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.

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

<a>, <area>, <link>, and <form> behavior #2615

Closed
annevk opened this issue May 2, 2017 · 29 comments
Closed

<a>, <area>, <link>, and <form> behavior #2615

annevk opened this issue May 2, 2017 · 29 comments
Labels

Comments

@annevk
Copy link
Member

annevk commented May 2, 2017

In #1381 we made <form> check that it's connected to a document before it can navigate.

We don't have such a check for <a>, <area>, and <link>. (It does seem per tests below that Firefox requires these to be connected as well, or at least <a>.)

On the other hand, it seems we don't have a requirement for <form> to be in a document that's fully active. It just needs to have a browsing context.

This mismatch doesn't make sense. Ideally all these elements have the same checks (apart from the sandboxing check that only applies to <form>).

Related tests:

I still need to write tests for <form>.

cc @tkent-google @bzbarsky @cdumez

@annevk annevk added interop Implementations are not interoperable with each other topic: navigation labels May 2, 2017
@annevk
Copy link
Member Author

annevk commented May 2, 2017

The forms tests would end up closing web-platform-tests/wpt#3109 it looks like.

annevk added a commit to web-platform-tests/wpt that referenced this issue May 2, 2017
@annevk
Copy link
Member Author

annevk commented May 2, 2017

I created web-platform-tests/wpt#5761 to test <form> and it seems Chrome has an active document check, WebKit still doesn't require it to be connected, and Firefox throws some kind of test harness error it looks like that I'm not sure how to debug.

@bzbarsky
Copy link
Contributor

bzbarsky commented May 2, 2017

and Firefox throws some kind of test harness error it looks like that I'm not sure how to debug

What I'm seeing is that in Firefox the form.submit() call in the "in a navigated document cannot navigate" test throws an exception with an empty .message (and the test harness doesn't report the .name of exceptions). That exception ends up getting thrown because Gecko has a check on all <form>/<a>/<area>-triggered navigations that looks more or less like this (this check is done in the browsing context associated with the node document of the node):

  1. Let doc be the node document of the triggering node.
  2. Let win be the Window of that document
  3. If win is null (indicating some sort of torn-down state), throw. This is the exception the test is hitting.
  4. If the browsing context has no current window (a non-spec concept which more or less corresponds to "window corresponding to the active document at this moment"), throw.
  5. If win is not the current window, no-op and return.

We could, of course, convert those two "throw"s in steps 3 and 4 to the "no-op and return" behavior.

@domenic
Copy link
Member

domenic commented May 2, 2017

I just want to say I agree with the overall mission here of creating a uniform check that is reused across all these elements. Presumably via a shared algorithm.

@annevk
Copy link
Member Author

annevk commented May 3, 2017

Okay, so I think it's non-controversial to require their node document to be fully active (and we should probably not throw given that's what most do). The only other bit that only Firefox currently enforces is that the element is connected (inserted into the document or one of its shadow trees). Chrome enforces that just for <form> and WebKit doesn't enforce it at all.

In Edge (15) I get a whole bunch of exceptions leading me to suspect that whatever we do is compatible there.

My proposal is that we align with Firefox, except turning the exceptions into a no-op. Would be good to hear from Apple and Google on that though.

annevk added a commit that referenced this issue May 3, 2017
Require that they are all connected to a document that is fully
active. There is enough difference between implementations that this
appears to be web compatible.

Tests:

* web-platform-tests/wpt#5758
* web-platform-tests/wpt#5759
* web-platform-tests/wpt#5761

Fixes #2615.
@bzbarsky
Copy link
Contributor

bzbarsky commented May 3, 2017

The only other bit that only Firefox currently enforces is that the element is connected

Note https://bugzilla.mozilla.org/show_bug.cgi?id=1218456 which claims this is a somewhat annoying behavior on the part of Firefox, at least for some use cases.

I, too, would like to hear from Apple and Google here.

@annevk
Copy link
Member Author

annevk commented May 3, 2017

I added a test for <area>.click() by the way and there Chrome and Safari do seem to require it to be connected. I filed #2617 as a follow-up for <link> and #2616 on the popup blocking behavior of <a> and <area>.

@domenic
Copy link
Member

domenic commented May 3, 2017

I think the right people to ping are dtapuska and cdumez but I've kind of lost the thread on what the sum behavior changes being proposed are for each browser. and in what threads they've ended up in. So I'll hold off on pinging them myself and leave that to you, @annevk, so your ping message can include a summary for them.

@annevk
Copy link
Member Author

annevk commented May 3, 2017

That summary is in #2615 (comment).

@domenic
Copy link
Member

domenic commented May 3, 2017

But isn't there something about <link> now, and new data about <area>? I'd expect when we ping implementers asking for changes we tell them exactly what changes we're hoping their particular browser to make.

@annevk
Copy link
Member Author

annevk commented May 3, 2017

@dtapuska how would you feel about making Chrome enforce that <a> is connected (inserted into a document or one of its shadow trees) before it can navigate, just as you already do for <area> and <form>? @cdumez same question for you, except for both <a> and <form>, as you only enforce it for <area> currently.

@dtapuska
Copy link
Contributor

dtapuska commented May 3, 2017

I don't think Chrome has any notion of connected for <area>. See this example

I have encountered a few sites doing exact this download pattern in the past. I'd ideally like to gather some metrics of the impact of making the connected-ness change for area and anchor.

I can confirm chrome has the connected-ness check for <form>

@annevk
Copy link
Member Author

annevk commented May 3, 2017

@dtapuska sounds reasonable. When can you report back on those metrics? As for <area>, I used https://w3c-test.org/submissions/5759/html/semantics/links/following-hyperlinks/activation-behavior.window.html (from web-platform-tests/wpt#5759) to test, though I don't seem to get stable runs in Chrome (I just got all PASS).

@annevk
Copy link
Member Author

annevk commented May 3, 2017

(FWIW, @bzbarsky, if you don't care about the connectedness, I'd be happy with not requiring that too (though we need to decide if <form> should then also no longer require it). The "fully active" part is the best bit I think and what actually ends up simplifying things.)

@bzbarsky
Copy link
Contributor

bzbarsky commented May 3, 2017

Yes, agreed on the "fully active" bit.

Pinging @smaug---- for the connectedness issue.

@dtapuska
Copy link
Contributor

dtapuska commented May 3, 2017

@annevk I believe your test case might be flawed in that if it detects a policy violation it goes to about:blank maybe? See this example If the link click doesn't work then it should end up at a red background but it gets to a green background.

@annevk
Copy link
Member Author

annevk commented May 5, 2017

My test was flawed because I reused names. It's now fixed.

@smaug---- thoughts on https://bugzilla.mozilla.org/show_bug.cgi?id=1218456 and no longer requiring <a>, <area>, and <link> to be connected to a document before they can be followed?

I actually think I'd prefer not requiring being connected (though we should then probably not require it for <form> either) as that seems like the faster path towards interop and it doesn't really matter.

@annevk
Copy link
Member Author

annevk commented May 5, 2017

Per #whatwg IRC discussion @smaug---- would prefer it if @dtapuska gathered data on making the connected change in Chrome.

@dtapuska
Copy link
Contributor

dtapuska commented May 5, 2017

I've added the change today...
https://chromium.googlesource.com/chromium/src/+/f5baf6ee31f52f2a79925c7f03626c2f3d0b9b9f

It will be 6-8 weeks before we get actionable data though.

scheib pushed a commit to scheib/chromium that referenced this issue May 5, 2017
There is some debate whether area and anchors should be connected for
click to be handled. See whatwg/html#2615 (comment)

This is a data gathering attempt.

BUG=

Review-Url: https://codereview.chromium.org/2861633004
Cr-Commit-Position: refs/heads/master@{#469699}
annevk added a commit that referenced this issue May 9, 2017
Require that they are all connected to a document that is fully
active. There is enough difference between implementations that this
appears to be web compatible.

Tests:

* web-platform-tests/wpt#5758
* web-platform-tests/wpt#5759
* web-platform-tests/wpt#5761

Fixes #2615.
@domenic
Copy link
Member

domenic commented May 16, 2017

Chrome's connectedness check appears to happen in the wrong place, after the submit event is already dispatched.... I'm working on tests for that as part of web-platform-tests/wpt#5878 and will file issues.

domenic added a commit that referenced this issue May 31, 2017
#2708 (comment)
pointed out that this check was just broken, so this fixes it. But the
check is also under discussion in #2615 and #2708, pending compat data,
so this adds a warning in the meantime.
domenic added a commit that referenced this issue Jun 2, 2017
#2708 (comment)
pointed out that this check was just broken, so this fixes it. But the
check is also under discussion in #2615 and #2708, pending compat data,
so this adds a warning in the meantime.
@domenic
Copy link
Member

domenic commented Jun 13, 2017

@dtapuska I can't find AnchorClickDispatchForNonConnectedNode in chromestatus.com, 6 weeks later... any ideas?

@domenic
Copy link
Member

domenic commented Jun 15, 2017

Managed to find the data; it's still internal as it threads its way through canary/beta channels. It isn't looking promising, i.e. a decent portion of people are triggering clicks on disconnected anchors. But we want to wait on making a final determination until we get broader visibility of the metric on the stable channel, which will occur in early August.

Re-setting my calendar reminder for then :)

@zcorpan
Copy link
Member

zcorpan commented Aug 17, 2017

I don't know if the counter has reached stable yet, but https://www.chromestatus.com/metrics/feature/timeline/popularity/1971 exists and the latest is:
15 aug. 2017
Percentage: 0,024169

@domenic
Copy link
Member

domenic commented Aug 18, 2017

That doesn't look good at all. So I think a elements should not get the connected check.

So per the general idea of this thread, maybe we should remove it from form and area as well?

@zcorpan
Copy link
Member

zcorpan commented Aug 21, 2017

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5311

Gecko/Chromium don't navigate, WebKit and EdgeHTML do.

Is the connectedness check for form in Gecko/Chromium deliberate? Was it something that changed in Chromium post-fork, or did WebKit change post-fork?

@annevk
Copy link
Member Author

annevk commented Aug 21, 2017

Chromium changed to align with Gecko iirc.

@tkent-google
Copy link
Collaborator

tkent-google commented Aug 23, 2017

https://www.chromestatus.com/metrics/feature/timeline/popularity/1971 exists and the latest is:
15 aug. 2017
Percentage: 0,024169

This comes from from Google Chrome Stable channel, and we can evaluate it.

Chromium changed to align with Gecko iirc.

That's right.

IMO, navigation by disconnected elements is weird. So I prefer limiting the weird behavior only to <a href>.

@annevk
Copy link
Member Author

annevk commented Aug 23, 2017

I'm happy with just allowing it on <a> and calling that out. I'll see about updating all the things.

@domenic
Copy link
Member

domenic commented Aug 23, 2017

Hmm, I liked the idea of everything being consistent, even if it means allowing navigation everywhere. But I don't care much.

Let's also remember to fix #2708, one way or another.

annevk added a commit that referenced this issue Aug 23, 2017
Require that they are all connected to a document that is fully
active. There is enough difference between implementations that this
appears to be web compatible.

Tests:

* web-platform-tests/wpt#5758
* web-platform-tests/wpt#5759
* web-platform-tests/wpt#5761

Fixes #2615.
annevk added a commit to web-platform-tests/wpt that referenced this issue Sep 22, 2017
@annevk annevk closed this as completed in f3c354a Sep 22, 2017
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this issue Nov 8, 2017
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this issue Nov 16, 2017
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
whatwg#2708 (comment)
pointed out that this check was just broken, so this fixes it. But the
check is also under discussion in whatwg#2615 and whatwg#2708, pending compat data,
so this adds a warning in the meantime.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Require that they are all connected to a document (except for <a>) that is fully active (including <a>). There is enough difference between implementations that this appears to be web compatible.

Also perform a second connected check for <form>.

Tests:

* web-platform-tests/wpt#5758
* web-platform-tests/wpt#5759
* web-platform-tests/wpt#5761

Fixes whatwg#2615 and fixes whatwg#2708.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants