-
Notifications
You must be signed in to change notification settings - Fork 160
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.
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
Rewrite processing shortcuts algorithm to be more precise #832
Conversation
Also creates a new array and returns it, rather than discarding the result.
Checks that name and url are present, URL is valid and within scope.
index.html
Outdated
<li>If <var>shortcut</var>["name"] or <var>shortcut</var>["url"] | ||
are undefined, <a>issue a developer warning</a> and | ||
<a>continue</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undefined or the empty string perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but for consistency we don't seem to check for empty string anywhere else in the file (e.g., for the name
field at the top level). Do you think it's worth inconsistent treatment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do check for empty strings where it makes sense, like the Web Share Target spec. I think it makes sense here.
We don't do it elsewhere because manifest properties are interpretable without name
, so we don't want to invalidate the whole manifest when its name
is empty. In this case, it's seems to be implied that if the name
of a ShortcutItem
is the empty string, it's invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on warnings for missing and empty. We were actually talking about this internally as we build out the implementation in Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Added "or if name is the empty string". Web Share Target is a reasonable precedent (even though it's not in the manifest spec yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Taking @aarongustafson 's LGTM as still valid since the only change I made he was in agreement. |
LGTM, thanks! |
Gentle reminder to please avoid using the (mostly) deprecated If you need to look up a term, you can do directly from ReSpec's Pill's "Search Definitions" option. Or, alternatively, head over to https://respec.org/xref/ to search for terms and "how to cite" them :) |
Normative changes: * shortcuts that meet certain failure conditions are ignored. * Checks that name and url are present, URL is valid and within scope of the manifest. Other changes: * Clarify that the ShortcutItem url member needs to be within scope. * Rewrite processing shortcuts algorithm as an ordered list. * Creates a new array and returns it, rather than discarding the result. * Explicitly says where developer warnings should be issued. Closes w3c#831
This change (choose one):
changes normative sections without changing behavior)
Implementation commitment (delete if not making normative changes):
To my knowledge, no vendor has implemented this yet, so it is safe to change without getting commitment. This is a very small change to add some new error cases which result in the shortcut items being ignored.
Commit message:
Normative changes:
Other changes:
Closes #831
Preview | Diff