Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

editorial: shortcut compares wrong scope #886

Closed
wants to merge 4 commits into from
Closed

editorial: shortcut compares wrong scope #886

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b49bab
refactor: export scope terms
marcoscaceres May 29, 2020
1222c50
fix: shortcut compares wrong scope
marcoscaceres May 29, 2020
7574af0
define scopes for manifest/url
marcoscaceres May 29, 2020
39ca0c8
fixup
marcoscaceres May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 42 additions & 41 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,31 +460,30 @@ <h2>
</p>
</div>
<p>
A <a>URL</a> <var>target</var> is said to be <dfn>within scope</dfn> of
<a>navigation scope</a> <var>scope</var> if the following algorithm
returns <code>true</code>:
A [=URL=] |target:URL| is said to be <dfn data-export="">within the
navigation scope of</dfn> [=URL=] |scope:URL| if the following
algorithm returns `true`:
</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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"using" is correct here. See the definition of concatenation. 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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought that was weird.... was going to ask about that.

|scopes|'s [=URL/path=] and U+002F (/).
</li>
<li>Let <var>targetPath</var> be the [=string/concatenation=] of <var>
target</var>'s <a data-cite="URL#concept-url-path">path</a>, using
U+002F (/).
<li>Let |targetPath:string| be the [=string/concatenation=] of
|target|'s [=URL/path=] and U+002F (/).
</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`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

return targetPath.starts_with(scopePath);

not

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree... much nicer.

</li>
<li>Otherwise, return <code>false</code>.
<li>Otherwise, 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=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... sorry, "Will need to merge #882 first... as this depends on that."

"within manifest scope">within the navigation scope of a web
manifest</dfn> {{WebAppManifest}} |manifest:WebAppManifest| if [=URL=]
|target:URL| is [=within the navigation scope of=] the |manifest|'s
{{WebAppManifest/scope}} member (once resolved).
</p>
<div class="note" title="Scope is a simple string match">
The URL string matching in this algorithm is prefix-based rather than
Expand All @@ -497,13 +496,13 @@ <h2>
</div>
<p>
If the <a>application context</a>'s <a>active document</a>'s
[=Document/URL=] is not <a data-lt="within-scope-manifest">within
scope</a> of the <a>application context</a>'s manifest, the user agent
SHOULD show a prominent UI element indicating the [=Document/URL=] or
at least its <a>origin</a>, including whether it is served over a
secure connection. This UI SHOULD differ from any UI used when the
[=Document/URL=] is <a>within scope</a>, in order to make it obvious
that the user is navigating off scope.
[=Document/URL=] is not [=within manifest scope=] of the <a>application
context</a>'s manifest, the user agent SHOULD show a prominent UI
element indicating the [=Document/URL=] or at least its <a>origin</a>,
including whether it is served over a secure connection. This UI SHOULD
differ from any UI used when the [=Document/URL=] is [=within manifest
scope=], in order to make it obvious that the user is navigating off
scope.
</p>
<div class="note">
<p>
Expand Down Expand Up @@ -537,9 +536,8 @@ <h3>
Deep links
</h3>
<p>
A <dfn>deep link</dfn> is a URL that is <a data-lt=
"within-scope-manifest">within scope</a> of an <a>installed web
application</a>'s manifest.
A <dfn>deep link</dfn> is a URL that is [=within manifest scope=] of
an <a>installed web application</a>'s manifest.
</p>
<p>
An <a>application context</a> can be instantiated through a <a>deep
Expand Down Expand Up @@ -1140,8 +1138,9 @@ <h3>
member</a> given <var>manifest</var>["<a>related_applications</a>"].
</li>
<li>Set <var>manifest</var>["<a>shortcuts</a>"] to the result of
running <a>processing the <code>shortcuts</code> member</a> given
<var>manifest</var>["<a>shortcuts</a>"] and <var>manifest URL</var>.
running <a>processing the `shortcuts` member</a> given
<var>manifest</var>["<a>shortcuts</a>"], <var>manifest URL</var>, and
<var>scope URL</var>.
</li>
<li>
<a>Extension point</a>: process any proprietary and/or other
Expand Down Expand Up @@ -1428,11 +1427,12 @@ <h3>
</li>
</ul>
</li>
<li>If <var>start URL</var> is not <a>within scope</a> of scope URL:
<li>If <var>start URL</var> is not [=within the navigation scope of=]
|scope URL|:
<ol>
<li>
<a>Issue a developer warning</a> that the start URL is not
<a>within scope</a> of the scope URL.
[=within the navigation scope of=] the scope URL.
</li>
<li>Return <var>default</var>.
</li>
Expand Down Expand Up @@ -1951,11 +1951,11 @@ <h3>
the most critical shortcuts appearing first in the array.
</p>
<p>
The steps for <dfn>processing the <code>shortcuts</code> member</dfn>
are given by the following algorithm. The algorithm takes a
The steps for <dfn>processing the `shortcuts` member</dfn> are given
by the following algorithm. The algorithm takes a
<a>sequence</a>&lt;<a>ShortcutItem</a>&gt; <var>shortcuts</var> and a
<a>URL</a> <var>manifest URL</var>. This algorithm returns a
<a>sequence</a>&lt;<a>ShortcutItem</a>&gt;.
<a>URL</a> |manifest URL:URL|, and [=URL=] <var>scope URL</var>. This
algorithm returns a <a>sequence</a>&lt;<a>ShortcutItem</a>&gt;.
</p>
<ol>
<li>Let <var>processedShortcuts</var> be a new Array object created
Expand All @@ -1978,9 +1978,9 @@ <h3>
URL</var> as the base URL. If the result is failure, <a>issue a
developer warning</a> and [=iteration/continue=].
</li>
<li>If <var>shortcut</var>["url"] is not <a>within scope</a> of
<var>manifest URL</var>, <a>issue a developer warning</a> and
[=iteration/continue=].
<li>If <var>shortcut</var>["url"] is not [=within the navigation
scope of=] <var>scope URL</var>, <a>issue a developer warning</a>
and [=iteration/continue=].
</li>
<li>
<a>Append</a> <var>shortcut</var> to
Expand Down Expand Up @@ -2642,8 +2642,9 @@ <h3>
</h3>
<p>
The <dfn>url</dfn> member of a <a>ShortcutItem</a> is the <a>URL</a>
<a data-lt="within scope of a manifest">within the application's
scope</a> that opens when the associated shortcut is activated.
that opens when the associated shortcut is activated. When resolved,
the {{ShortcutItem/url}} must be [=within manifest scope=], otherwise
it gets ignored.
</p>
</section>
<section data-dfn-for="ShortcutItem" data-link-for="ShortcutItem">
Expand Down