Add allow-top-navigation-by-user-activation sandbox token #2292
Conversation
domenic
added
do not merge yet
|
My big concern around all this is the complete lack of clarity or interop around what "user activation" means. :( //cc @smaug---- @annevk |
|
I mean, it's well-defined in the spec: https://html.spec.whatwg.org/#triggered-by-user-activation But we know that e.g. Chrome has a more restrictive implementation than that: #1903. Are you saying that we should not build any future features on top of "triggered by user activation" until we achieve interop on its definition? That seems unfortunate, given what a useful primitive it is. |
Ah, I didn't know we had an existing definition for this. I do think that there is great danger in adding new features that see wide use that trigger off non-interoperable "triggered by user activation", because it runs the risk of causing a race to the bottom in terms of what constitutes "user activation", and possibly browsers actually using different definitions for different features. But it looks like the spec definition is already fairly raced to the bottom (in that I think it's looser than Gecko's as well)... We should probably up the priority of getting the spec on "triggered by user activation" sorted out and then I think I personally am OK with this proposal. |
domenic
removed
do not merge yet
|
Tests for this are landing in https://codereview.chromium.org/2711073002/ (i.e. they will be automatically upstreamed to WPT). So this needs editor review :). |
domenic
removed
the
needs tests
There was a problem hiding this comment.
I have reviewed the changes and would be OK with merging as is, but had an "Otherwise" preference that perhaps @domenic can decide on.
It seems that the chance of copy-paste errors and double negatives in this kind of change is pretty high, but I didn't spot any.
| data-x="attr-iframe-sandbox-allow-top-navigation-by-user-activation">allow-top-navigation-by-user-activation</code> | ||
| keywords must not both be specified, as doing so is redundant; only <code | ||
| data-x="attr-iframe-sandbox-allow-top-navigation">allow-top-navigation</code> will have an effect | ||
| in such non-conformant markup.</p> |
There was a problem hiding this comment.
Filed validator/validator#470 for this.
source
Outdated
| top-level navigation with user activation browsing context flag</span> set, then abort these | ||
| steps negatively.</p></li> | ||
|
|
||
| <li><p>If this algorithm is not <span>triggered by user activation</span> and <var>A</var>'s |
There was a problem hiding this comment.
Leading with "Otherwise, ..." here would be slightly better I think, but an editorial choice and I don't know if tastes have changed over the years.
There was a problem hiding this comment.
Seems reasonable, will change
|
(I didn't consider whether improvements in #1903 ought to block this or not, my review isn't a vote in favor of not blocking if y'all would like to.) |
domenic
force-pushed
the
new-sandbox-popups
branch
from
9f3f358 to
b841b31
Compare
Add `allow-top-navigation-by-user-activation` to the allowed values for `iframe[sandbox]`. See also whatwg/html#2292 (review) Fixes #470 Thanks @foolip
Fixes WICG/interventions#42.
The discussion had engagement from Mozilla and Microsoft but no confirmation of their support of the latest proposal, so tagging this as "needs implementor interest" and thus "do not merge yet" until then.
We'll also need tests before we can land this.