Bug 218383 - WebDriver: do not focus the main frame when switching to a window
Summary: WebDriver: do not focus the main frame when switching to a window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 166679
  Show dependency treegraph
 
Reported: 2020-10-30 07:08 PDT by Carlos Garcia Campos
Modified: 2020-11-16 06:06 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.16 KB, patch)
2020-10-30 07:10 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-10-30 07:08:20 PDT
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).
Comment 1 Carlos Garcia Campos 2020-10-30 07:10:25 PDT
Created attachment 412738 [details]
Patch
Comment 2 BJ Burg 2020-11-04 11:43:25 PST
Comment on attachment 412738 [details]
Patch

r=me, great investigation!
Comment 3 James Graham 2020-11-04 12:02:07 PST
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
Comment 4 James Graham 2020-11-04 12:27:55 PST
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.
Comment 5 BJ Burg 2020-11-04 12:39:44 PST
(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.
Comment 6 Sam Sneddon [:gsnedders] 2020-11-04 13:22:37 PST
(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?
Comment 7 Carlos Garcia Campos 2020-11-05 00:55:03 PST
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?
Comment 8 Carlos Garcia Campos 2020-11-05 01:02:27 PST
(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.
Comment 9 James Graham 2020-11-05 01:11:23 PST
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.
Comment 10 Radar WebKit Bug Importer 2020-11-06 06:09:16 PST
<rdar://problem/71117298>
Comment 11 Carlos Garcia Campos 2020-11-13 04:58:47 PST
Brian, should I land this patch in any case?
Comment 12 Sam Sneddon [:gsnedders] 2020-11-13 06:01:50 PST
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.
Comment 13 EWS 2020-11-16 06:06:22 PST
Committed r269851: <https://trac.webkit.org/changeset/269851>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412738 [details].