Skip to content

[webdriver] Add synchronization for wheel action#37260

Merged
xiaochengh merged 2 commits intoservo:mainfrom
PotatoCP:sync-wheel
Jun 11, 2025
Merged

[webdriver] Add synchronization for wheel action#37260
xiaochengh merged 2 commits intoservo:mainfrom
PotatoCP:sync-wheel

Conversation

@PotatoCP
Copy link
Copy Markdown
Contributor

@PotatoCP PotatoCP commented Jun 5, 2025

Implement action synchronization for wheel event. Previously only done for pointer here #36932.

Testing: tests/wpt/meta/webdriver/tests/classic/perform_actions/wheel.py

@PotatoCP
Copy link
Copy Markdown
Contributor Author

PotatoCP commented Jun 5, 2025

@longvatrong111
Copy link
Copy Markdown
Contributor

Nice change. Thank for updating webdriver_id to Option in MouseButtonAction and MouseMoveAction, it fixes some issues in mouse move command as well.
I don't see the part WebDriverCommandResponse is sent back to webdriver, maybe this is a draft PR and you are implementing it?

@PotatoCP
Copy link
Copy Markdown
Contributor Author

PotatoCP commented Jun 6, 2025

Nice change. Thank for updating webdriver_id to Option in MouseButtonAction and MouseMoveAction, it fixes some issues in mouse move command as well. I don't see the part WebDriverCommandResponse is sent back to webdriver, maybe this is a draft PR and you are implementing it?

Since we already add webdriver_id on WheelEvent, it should be sent by

fn notify_webdriver_input_event_completed(&self, pipeline_id: PipelineId, event: &InputEvent) {
let Some(id) = event.webdriver_message_id() else {
return;
};
if let Err(error) = self.senders.pipeline_to_constellation_sender.send((
pipeline_id,
ScriptToConstellationMessage::WebDriverInputComplete(id),
)) {
warn!("ScriptThread failed to send WebDriverInputComplete {id:?}: {error:?}",);
}
}

The function is called on process_pending_input_events

self.notify_webdriver_input_event_completed(pipeline_id, &event.event);

@PotatoCP PotatoCP marked this pull request as ready for review June 6, 2025 02:34
@PotatoCP PotatoCP force-pushed the sync-wheel branch 2 times, most recently from 22dd140 to ee1b92b Compare June 7, 2025 04:28
@PotatoCP PotatoCP changed the title Add synchronization for webdriver wheel action [webdriver] Add synchronization for wheel action Jun 9, 2025
@PotatoCP
Copy link
Copy Markdown
Contributor Author

Have updated expectation after #37385

f32,
f64,
f64,
Option<WebDriverMessageId>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this is an option.

Is there any place where webdriver does not set message id on a MouseButton / MouseMove / WheelScroll action?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 command may trigger multiple events, for current solution, we only want to add webdriverid to the last event.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, with some fresh thoughts, does that mean that if we want thing to be semantically correct, message ids should be set on something higher-level, instead of just actions? But we have the problem of matching webdriver actions and input events on the UA, which seems tough...

So I think this PR is fine on its own, but we should probably leave a note here (and probably also file an issue) about:

  1. When will this message ID be None and why
  2. Think about ways to improve the design

Comment on lines +644 to +648
let msg_id = if last {
self.current_action_id.get()
} else {
None
};
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaochengh Just like Trong said, we set the id to None if it's not the last action sent here.

f32,
f64,
f64,
Option<WebDriverMessageId>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, with some fresh thoughts, does that mean that if we want thing to be semantically correct, message ids should be set on something higher-level, instead of just actions? But we have the problem of matching webdriver actions and input events on the UA, which seems tough...

So I think this PR is fine on its own, but we should probably leave a note here (and probably also file an issue) about:

  1. When will this message ID be None and why
  2. Think about ways to improve the design

@PotatoCP
Copy link
Copy Markdown
Contributor Author

Hi @xiaochengh, I have added comments for the option.

@xiaochengh xiaochengh enabled auto-merge June 11, 2025 08:42
@xiaochengh xiaochengh added this pull request to the merge queue Jun 11, 2025
Merged via the queue into servo:main with commit 15eadb5 Jun 11, 2025
21 checks passed
@PotatoCP PotatoCP deleted the sync-wheel branch June 18, 2025 06:54
github-merge-queue bot pushed a commit that referenced this pull request Jun 18, 2025
Implement action synchronization for key event. Previously only done for
pointer #36932 and wheel
#37260.

---------

Signed-off-by: PotatoCP <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants