Move handling of actions input states to WebDriverSession
Categories
(Remote Protocol :: Marionette, task, P2)
Tracking
(Not tracked)
People
(Reporter: whimboo, Unassigned)
References
(Depends on 1 open bug)
Details
(Whiteboard: [marionette-fission-reserve])
Right now the action input state is stored in each and every process (including different content processes) separately. This is because it's state is hold in action.js., which gets loaded by the MarionetteCommandsActor
child.
It means that different processes have their own state, and it cannot be shared.
This is especially important for Fission, when sub-frames could be out of process, and won't have the state from eg. the parent.
I can see especially the pointerevents web-platform tests failing a lot with invalid input states, which cause harness errors and as such most of the tests are marked as expected Error
.
James, am I reading the WebDriver spec correctly? Would be great to get your feedback before we actually get this changed. Thanks.
Comment 1•4 years ago
|
||
I think this should be per session rather than per window and afaict that's what ChromeDriver does.
Comment 2•4 years ago
|
||
https://source.chromium.org/chromium/chromium/src/+/master:chrome/test/chromedriver/window_commands.cc;l=1880?q=actions&ss=chromium%2Fchromium%2Fsrc:chrome%2Ftest%2Fchromedriver%2F is ChromeDriver's release actions implementation, which refers to a per-session list.
Comment 3•4 years ago
|
||
Tracking marionette-fission-reserve bugs for Fission MVP
Reporter | ||
Comment 4•4 years ago
|
||
Julien, I might not be able to get to this bug this week. Would it be possible for you to investigate / fix that in January? It's the last blocking bug for the framescript code removal. We need some similar logic as for the element reference store.
I'm happy to chat about details this week, so it's clear what needs to be done here.
Comment 5•4 years ago
|
||
Sure, I can take a look this week.
Comment 6•4 years ago
|
||
I have a basic implementation working at https://treeherder.mozilla.org/jobs?repo=try&revision=330cc3ad37b0844ab4b7cc35d4e3a653dda60fa9 but it needs a lot of cleanup.
The general idea was to copy action.js to an action-parent.js module. action-parent.js is almost identical to action.js, except that it relies on JSWindowActors to create/synthesize events.
We can share a lot of code between action & action-parent, my current patch was just a WIP to get an idea of what kind of issues could occur.
Some interesting bits:
- the DoubleClick state stays in the content process for now. We might move it to the parent process later, but it is updated outside of the scope of actions, so I would prefer not to update it here
- the PointerMove was a bit challenging here. Initially I imagined that everything except event creation would be done in parent. But I'm afraid that adding async roundtrips between parent and child for PointerMove could create issues. So for now, the child process still handles the timer & creation of pointer move events (https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/testing/marionette/action.js#1406-1436)
- for now I am sending keyEvent and mouseEvent objects from parent to child. This created an issue with mouseEvents because the serialization turns
undefined
properties intonull
. This means that a mouseEvent withcode: undefined
on the parent becamecode: null
in the child, which confused the event creation. As a workaround, I perform a special cleanup on those parameters. As a final solution, we can either try to pass "safer" arguments (it was just easier to pass the keyEvent/mouseEvent object, but in some cases, we could get away with smaller payloads). Or we could also change the logic that turns undefined to null (https://searchfox.org/mozilla-central/rev/014fe72eaba26dcf6082fb9bbaf208f97a38594e/testing/marionette/evaluate.js#301-303)
Comment 7•4 years ago
|
||
Does this marionette-fission-reserve
bug need to block shipping Fission MVP?
Comment 8•4 years ago
|
||
For now, I am assuming marionette-fission-reserve
bugs don't need to block shipping Fission MVP.
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Comment 9•4 years ago
|
||
As per spec the WebDriver session has to hold these values. As such we should wait for bug 1683193.
A session has an associated list of active input sources.
A session has an associated input state table.
A session has an associated input cancel list.
Reporter | ||
Comment 10•4 years ago
|
||
This bug is not a priority right now but I want to mention that it is unblocked now. So whenever there is a free slot feel free to continue.
Reporter | ||
Comment 11•3 years ago
|
||
Lets wait at least until the refactoring of the WebDriver session has been done via bug 1720707.
Updated•2 years ago
|
Description
•