Re: [w3c/manifest] editorial: shortcut compares wrong scope (#886)

@mgiuca commented on this pull request.

This is a tricky PR to review ... it's making two separate changes (one to the definition of the "within scope" algorithm, which should have no behavioural changes, and one to fix the bug in shortcuts), as well as pulling in the name change to the within scope algorithm from the other PR, and changing a bunch of HTML syntax to Respec.

Could you separate some of these out? I suggest only fixing the shortcut bug here, and delaying the other changes to the scope algorithm until we've sorted out the naming on the other PR.

>        </p>
-      <ol>
-        <li>Let <var>scopePath</var> be the [=string/concatenation=] of
-        <var>scopes</var>'s <a data-cite="URL#concept-url-path">path</a>, using
-        U+002F (/).
+      <ol class="algorithm">
+        <li>If |target| and |scope| are not [=same origin=], return `false`.
+        </li>
+        <li>Let |scopePath:string| be the [=string/concatenation=] of

"using" is correct here. See the definition of [concatenation](https://infra.spec.whatwg.org/#string-concatenate). This is supposed to place "/" in between each piece of the path, not at the end of it.

Perhaps you could clarify it by writing "using the separator "/"" or "using "/" as a separator".

>          </li>
-        <li>If <var>target</var> is <a>same origin</a> as <var>scope</var> and
-        <var>targetPath</var> starts with <var>scopePath</var>, return
-        <code>true</code>.
+        <li>If |targetPath| starts with |scopePath:string|, return `true`.

Maybe this and the next line could be combined by saying "Return a Boolean indicating whether |targetPath| starts with |scopePath|." Or even just "Return true if |targetPath| starts with |scopePath|, or false otherwise."

Since if I were writing this in C++, I would write:

```c++
return targetPath.starts_with(scopePath);
```

not

```c++
if (targetPath.starts_with(scopePath))
  return true;
else
  return false;
```

>          </li>
       </ol>
       <p>
-        A <a>URL</a> <var>target</var> is said to be <dfn data-lt=
-        "within-scope-manifest">within scope of a manifest</dfn>
-        <var>manifest</var> if <var>target</var> is <a>within scope</a> of the
-        navigation scope of <var>manifest</var>.
+        A [=URL=] |target:URL| is said to be <dfn data-export="" data-local-lt=

This seems to have stuff leaking into it from the other PR? (I hope you don't change this definition in the same PR as making the other changes.)

>          </li>
       </ol>
       <p>
-        A <a>URL</a> <var>target</var> is said to be <dfn data-lt=
-        "within-scope-manifest">within scope of a manifest</dfn>
-        <var>manifest</var> if <var>target</var> is <a>within scope</a> of the
-        navigation scope of <var>manifest</var>.
+        A [=URL=] |target:URL| is said to be <dfn data-export="" data-local-lt=

The two PRs seem very intertwined. Maybe hold off on this one until that one is resolved.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/manifest/pull/886#pullrequestreview-420690807