The spec doesn't say we should focus the main frame. This is confusing WPT test infrastructure/testdriver/actions/crossOrigin.sub.html that first focuses an input in an iframe and then it send keyboard events to the window (after switching to the window again even when it's already the current one, that causes the iframe focus to be lost).
Created attachment 412738 [details] Patch
Comment on attachment 412738 [details] Patch r=me, great investigation!
So. We recently refactored the spec here and afaict per the current text, switch to window does reset the frame; it calls [1] which unconditionally sets the current browsing context. But I can't tell you if that's actually the correct behaviour here; if it doesn't match browsers we should possibly change it to be a no-op if the window doesn't change (the refactor was intended to ensure we didn't fail commands in lots of cases where a frame was closed and the top level context was still open). The infra test could well be wrong. [1] https://w3c.github.io/webdriver/#dfn-set-the-current-top-level-browsing-context
Oh, right so I was confused. Focus doesn't mean the same as the current browsing context. But exactly what should happen in step 5 is somewhat unclear. It seems non-obvious to me that it should work to perform key actions on an iframe that has the OS-level focus when the command is being processed in the top-level frame. Especially in the context of e.g. an out of process iframe that seems difficult to implement.
(In reply to James Graham from comment #4) > Oh, right so I was confused. Focus doesn't mean the same as the current > browsing context. But exactly what should happen in step 5 is somewhat > unclear. It seems non-obvious to me that it should work to perform key > actions on an iframe that has the OS-level focus when the command is being > processed in the top-level frame. Especially in the context of e.g. an out > of process iframe that seems difficult to implement. Caret position (which determines where the keystrokes are dispatched to when a window is active) is different from window focus. I don't think WebDriver has a distinction.
(In reply to James Graham from comment #3) > So. We recently refactored the spec here and afaict per the current text, > switch to window does reset the frame; it calls [1] which unconditionally > sets the current browsing context. > > But I can't tell you if that's actually the correct behaviour here; if it > doesn't match browsers we should possibly change it to be a no-op if the > window doesn't change (the refactor was intended to ensure we didn't fail > commands in lots of cases where a frame was closed and the top level context > was still open). > > The infra test could well be wrong. > > [1] > https://w3c.github.io/webdriver/#dfn-set-the-current-top-level-browsing- > context As I read the spec, changing the current window changes the "current browsing context" (and more obviously the "current top-level browsing context"), and send keys should fail to send keys to an element within a frame as it looks up the element by id within the "current browsing context", and the element shouldn't exist within the top-level browsing context, only its child. So per spec send keys should fail due to not finding the element?
The spec says: "Update any implementation-specific state that would result from the user selecting the current browsing context for interaction, without altering OS-level focus." So, I'm not sure how to interpret that. The current browsing context after setting the toplevel browsing context is the current window main frame or the window itself?
(In reply to Sam Sneddon [:gsnedders] from comment #6) > (In reply to James Graham from comment #3) > > So. We recently refactored the spec here and afaict per the current text, > > switch to window does reset the frame; it calls [1] which unconditionally > > sets the current browsing context. > > > > But I can't tell you if that's actually the correct behaviour here; if it > > doesn't match browsers we should possibly change it to be a no-op if the > > window doesn't change (the refactor was intended to ensure we didn't fail > > commands in lots of cases where a frame was closed and the top level context > > was still open). > > > > The infra test could well be wrong. > > > > [1] > > https://w3c.github.io/webdriver/#dfn-set-the-current-top-level-browsing- > > context > > As I read the spec, changing the current window changes the "current > browsing context" (and more obviously the "current top-level browsing > context"), and send keys should fail to send keys to an element within a > frame as it looks up the element by id within the "current browsing > context", and the element shouldn't exist within the top-level browsing > context, only its child. So per spec send keys should fail due to not > finding the element? It's not send keys command, but an action sequence. Two action sequences actually, on to click on the input element and another one to send "PASS" to the input filed. Keyboard actions don't have an element as target, they are equivalent to press/release they keys, so the current focused element will handle them. In this case, the window we are switching to is the one containing the iframe, so there isn't a toplevel browsing context change in the end. test_driver.set_test_context(parent); await new test_driver.Actions() .pointerMove(0, 0, {origin: input}) .pointerDown() .pointerUp() .send(); await new test_driver.Actions() .keyDown("P") .keyUp("P") .keyDown("A") .keyUp("A") .keyDown("S") .keyUp("S") .keyDown("S") .keyUp("S") .send(); if (input.value === "PASS") { test_driver.message_test("PASS", "*") } else { test_driver.message_test("FAIL", "*") } For some reason between the first action sequence and the second there's a window switch.
The test is definitely broken. It ought to switch back to the iframe for the second action sequence; I'm going to look into that now. I think it's unclear from the spec whether sending key events to a browsing context when the caret is in a child of that browsing context is expected to work. It seems like in Firefox this does work except when we have site isolation enabled when it doesn't. I guess it works in Chrome too if this test passes there. So we might need to change the spec so it's explicitly supported. Basically: I think if you take this patch that's going to make WebKit more like blink and pre-site-isolation gecko, so it's probably the right thing to do. But I'm also going to fix the test, and I don't think the spec is clear one way or another.
<rdar://problem/71117298>
Brian, should I land this patch in any case?
Carlos, I'm not Brian [citation needed], but if James says it makes us more like Blink and non-site-isolation Gecko (which hasn't shipped yet), I'd say we should land it.
Committed r269851: <https://trac.webkit.org/changeset/269851> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412738 [details].