[WebDriver] Add synchronization for key action by PotatoCP · Pull Request #37403 · servo/servo

PotatoCP · 2025-06-12T02:54:57Z

Implement action synchronization for key event. Previously only done for pointer #36932 and wheel #37260.

PotatoCP · 2025-06-12T02:55:31Z

cc: @xiaochengh @longvatrong111 @yezhizhen

PotatoCP · 2025-06-12T04:34:59Z

For try, test for key, key_events, key_shortcuts, and key_special_keys seems stable here

https://github.com/PotatoCP/servo/actions/runs/15600770809
https://github.com/PotatoCP/servo/actions/runs/15600776568
https://github.com/PotatoCP/servo/actions/runs/15600920645

Note I run that try before the last commit for wpt expectation.

Also I update the expectation based on servodriver test here

https://github.com/PotatoCP/servo/actions/runs/15579793468
https://github.com/PotatoCP/servo/actions/runs/15581529630

Which is why I remove both failed expectation for key_shortcuts. And I am also reluctant to change the expectation of key_events since it is still timeout with servodriver.

The key_events is getting timeout since it sometimes failed the hit testing. In try, we run with headless browser, so maybe that's why it passes there.

longvatrong111

I have some suggestion about implementing count.
My main idea is we should follow the spec and avoid making differences in function signature.

longvatrong111 · 2025-06-12T05:05:12Z

components/webdriver_server/actions.rs


            // Step 1.3. Try to dispatch tick actions
-            self.dispatch_tick_actions(tick_actions, tick_duration)?;
+            let cnt = self.dispatch_tick_actions(tick_actions, tick_duration)?;


Is there any reason to return a count here?
If it's only a refactor so we should avoid changing function signature.
If it improves the couting logic, I recommend we can make a class variable and update anywhere in actions code.

Hmm, my concern is that sometimes dispatch_keyup sometimes don't dispatch any event

https://github.com/servo/servo/pull/37403/files#diff-5401b925eefc1f03be09174aea84fbfcbc5d8b85b559db101aa8d7dbef7b6745R328-R342

Hence, I try to return if it's counted or not. I also have concern with differing with spec, but I don't really know how to handle this one.

Actually not only in keyup, but also in the pointerdown and pointerup. Here

https://github.com/servo/servo/pull/37403/files#diff-5401b925eefc1f03be09174aea84fbfcbc5d8b85b559db101aa8d7dbef7b6745R360-R361

As discussed offline, I add sent_action_count item in Handler to avoid returning i32 when dispatching actions. @longvatrong111

components/shared/embedder/webdriver.rs

components/webdriver_server/lib.rs

xiaochengh · 2025-06-13T06:58:15Z

components/shared/embedder/input_events.rs


+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct ServoKeyEvent {
+    pub event: KeyboardEvent,


This pattern is weird and also inconsistent with the other types of events.

Is it possible to add message id to the existing KeyboardEvent struct?

Unfortunately KeyboardEvent is taken from keyboard_types crate. I don't know how to add the id unless we wrap the event into another struct.

I see, thanks. Then wrapping it seems to be the only way to go.

I'm still concerned about the inconsistent approaches between keyboard events and other events. But maybe I'm overthinking...

ServoKeyEvent is still a weird name. Any better idea?

For number one, I also think that the approach is weird, but I think we have to do it like this until in the future, if the design for waiting the action is changed for the better.

For number two, how about WrappedKeyEvent?

I don't have a good idea. Maybe just call it KeyboardEventWithWebDriverMsgId, plus some comment what this is for and why, and a TODO for thinking about a better name.

A name should explain its role and usage. WrappedKeyEvent is therefore not informative enough, as readers still can't tell why it's wrapped.

How about keep it KeyboardEvent? Because it still appears everywhere in Servo. Other parts don't need to care about webdriver_id we add inside.

I think calling it KeyboardEvent makes sense.

The problem with calling it KeyboardEvent is that in headed_window.rs, we already have KeyboardEvent from Servo. So I think there will be conflict of name.

I'll go with Xiaocheng's suggestion for now.

Signed-off-by: PotatoCP <[email protected]>

longvatrong111

The purpose of wrapping a type is we pretend the wrapper type is the internal type.
We can expose everything of keyboard_types::KeyboardEvent in our embedder::KeyboardEvent, it seems a bit verbose here but help keeping the API input_events provides.
Or can we fix it in keyboard_types crate?

longvatrong111 · 2025-06-14T13:06:39Z

components/shared/embedder/input_events.rs

+/// Wrap the KeyboardEvent from crate to pair it with webdriver_id,
+/// which is used for webdriver action synchronization.
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub struct KeyboardEventWithWebDriverId {


Can we do something like this? I believe it can minimize the change outside of this file.

Suggested change

pub struct KeyboardEventWithWebDriverId {

use keyboard_types::{CompositionEvent, KeyboardEvent as ExternalKeyboardEvent, Modifier};

pub struct KeyboardEvent {

pub event: ExternalKeyboardEvent,

pub modifiers: Modifier,

...

}

impl KeyboardEvent {

pub fn new(event: ExternalKeyboardEvent) -> Self {

Self {

event: keyboard_event,

modifiers: event.modifiers,

webdriver_id: None,

}

}

fn foo(&self) {

self.event.foo()

}

}

This looks good. ~~I think the event item is unneeded, since keyboard_types:KeyboardEvent has no method~~

The event field is still needed I guess, see below comment.

PotatoCP · 2025-06-16T07:00:34Z

I have tried to disguise the wrapper struct. Maybe you can check later when you have time @longvatrong111.

PotatoCP · 2025-06-16T07:03:19Z

components/shared/embedder/input_events.rs

+pub struct ShortcutMatcher;
+impl ShortcutMatcher {
+    pub fn from_event<T>(event: KeyboardEvent) -> ExternalShortcutMatcher<T> {
+        ExternalShortcutMatcher::from_event(event.event)
+    }
+}
+


This is for

servo/ports/servoshell/desktop/app_state.rs

Lines 315 to 323 in e69962e

fn handle_overridable_key_bindings(&self, webview: ::servo::WebView, event: KeyboardEvent) {

let origin = webview.rect().min.ceil().to_i32();

ShortcutMatcher::from_event(event)

.shortcut(CMD_OR_CONTROL, '=', || {

webview.set_zoom(1.1);

})

.shortcut(CMD_OR_CONTROL, '+', || {

webview.set_zoom(1.1);

})

Let me know if anyone has any better suggestion about this, since I has some doubts.

If we can't totally remove keyboard_types::KeyboardEvent, I think it's good to convert between 2 events sometimes.

longvatrong111

Look mostly good to me. I have some more suggestions.

components/webdriver_server/lib.rs

ports/servoshell/desktop/headed_window.rs

components/shared/embedder/input_events.rs

Signed-off-by: PotatoCP <[email protected]>

xiaochengh · 2025-06-17T07:08:51Z

components/shared/embedder/input_events.rs

+/// which is used for webdriver action synchronization.
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+pub struct KeyboardEvent {
+    pub state: KeyState,


We can either wrap or unpack the external KeyboardEvent, but doing both at the same time looks really bad to me.

Let's go back to simply wrapping it so that we can reuse the abilities of the external KeyboardEvent.

Thanks. I have made so that we only wrap the event but still making sure that code other than input_event only use input_event::KeyboardEvent instead of keyboard_types::KeyboardEvent

Signed-off-by: PotatoCP <[email protected]>

xiaochengh · 2025-06-18T03:18:21Z

Build on OH failed. Could you fix the usage of KeyboardEvent in ports/servoshell/egl/app_state.rs?

Signed-off-by: PotatoCP <[email protected]>

PotatoCP · 2025-06-18T06:40:38Z

Thanks for the notice. It should be good now @xiaochengh

github-actions · 2025-06-18T07:16:07Z

🔨 Triggering try run (#15726388444) for OpenHarmony

github-actions · 2025-06-18T07:23:44Z

Bencher Report

Branch	main
Testbed	HUAWEI Mate 60 Pro

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.
Memory (Bytes)
Latency (nanoseconds (ns))
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results

Benchmark	Latency	milliseconds (ms)	Memory	Bytes
E2E/https://www.google.com/JS/gc-heap/admin			📈 view plot ⚠️ NO THRESHOLD	23,840.00
E2E/https://www.google.com/JS/gc-heap/decommitted			📈 view plot ⚠️ NO THRESHOLD	573,440.00
E2E/https://www.google.com/JS/gc-heap/unused			📈 view plot ⚠️ NO THRESHOLD	49,272.00
E2E/https://www.google.com/JS/gc-heap/used			📈 view plot ⚠️ NO THRESHOLD	402,024.00
E2E/https://www.google.com/JS/malloc-heap			📈 view plot ⚠️ NO THRESHOLD	2,862,067.00
E2E/https://www.google.com/JS/non-heap			📈 view plot ⚠️ NO THRESHOLD	262,144.00
E2E/https://www.google.com/LayoutThread/box-tree			📈 view plot ⚠️ NO THRESHOLD	101,488.00
E2E/https://www.google.com/LayoutThread/display-list			📈 view plot ⚠️ NO THRESHOLD	0.00
E2E/https://www.google.com/LayoutThread/font-context			📈 view plot ⚠️ NO THRESHOLD	4,040.00
E2E/https://www.google.com/LayoutThread/fragment-tree			📈 view plot ⚠️ NO THRESHOLD	112.00
E2E/https://www.google.com/LayoutThread/stylist			📈 view plot ⚠️ NO THRESHOLD	5,264.00
E2E/https://www.google.com/Load	📈 view plot ⚠️ NO THRESHOLD	444.59 ms
E2E/https://www.google.com/Resident			📈 view plot ⚠️ NO THRESHOLD	184,971,264.00
E2E/https://www.google.com/image-cache			📈 view plot ⚠️ NO THRESHOLD	35,328.00
E2E/https://www.servo.org/Load	📈 view plot ⚠️ NO THRESHOLD	1,463.39 ms
E2E/https://www.servo.org/Resident			📈 view plot ⚠️ NO THRESHOLD	265,460,121.00

🐰 View full continuous benchmarking report in Bencher

github-actions · 2025-06-18T07:34:16Z

✨ Try run (#15726388444) succeeded.

longvatrong111 reviewed Jun 12, 2025

View reviewed changes

PotatoCP changed the title ~~[Webdriver] Add synchronization for key action~~ [WebDriver] Add synchronization for key action Jun 12, 2025

PotatoCP marked this pull request as ready for review June 12, 2025 09:27

PotatoCP requested review from atbrakhi and gterzian as code owners June 12, 2025 09:27

longvatrong111 mentioned this pull request Jun 13, 2025

Too many intermittent failures for webdriver conformance tests #37387

Closed

xiaochengh reviewed Jun 13, 2025

View reviewed changes

Add synchronization for key action

5d625e2

Signed-off-by: PotatoCP <[email protected]>

PotatoCP force-pushed the add-sync-key branch from 6a171a0 to 5d625e2 Compare June 14, 2025 02:55

longvatrong111 reviewed Jun 14, 2025

View reviewed changes

PotatoCP force-pushed the add-sync-key branch from 3445e5d to 4cae02f Compare June 16, 2025 06:51

PotatoCP commented Jun 16, 2025

View reviewed changes

longvatrong111 reviewed Jun 16, 2025

View reviewed changes

components/webdriver_server/lib.rs Outdated Show resolved Hide resolved

ports/servoshell/desktop/headed_window.rs Outdated Show resolved Hide resolved

components/shared/embedder/input_events.rs Outdated Show resolved Hide resolved

PotatoCP force-pushed the add-sync-key branch 3 times, most recently from 7d1c871 to 12080fd Compare June 17, 2025 01:54

Let the event wrapper name be KeyboardEvent

136d73f

Signed-off-by: PotatoCP <[email protected]>

PotatoCP force-pushed the add-sync-key branch from 12080fd to 136d73f Compare June 17, 2025 01:56

PotatoCP requested a review from xiaochengh June 17, 2025 02:00

xiaochengh reviewed Jun 17, 2025

View reviewed changes

Only wrap and not extract keyboard::types::KeyboardEvent

651d757

Signed-off-by: PotatoCP <[email protected]>

PotatoCP force-pushed the add-sync-key branch from 1ec9f4c to 651d757 Compare June 17, 2025 08:10

xiaochengh approved these changes Jun 17, 2025

View reviewed changes

xiaochengh added this pull request to the merge queue Jun 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 17, 2025

Fix KeyboardEvent on OpenHarmony app_state

fc9e5c2

Signed-off-by: PotatoCP <[email protected]>

PotatoCP requested a review from jschwe as a code owner June 18, 2025 06:06

jschwe added the T-ohos Do a try run on OpenHarmony label Jun 18, 2025

github-actions bot removed the T-ohos Do a try run on OpenHarmony label Jun 18, 2025

xiaochengh added this pull request to the merge queue Jun 18, 2025

Merged via the queue into servo:main with commit cdc8b45 Jun 18, 2025
33 checks passed

-pub struct KeyboardEventWithWebDriverId {
+use keyboard_types::{CompositionEvent, KeyboardEvent as ExternalKeyboardEvent, Modifier};
+pub struct KeyboardEvent {
+   pub event: ExternalKeyboardEvent,
+   pub modifiers: Modifier,
+   ...
+}
+impl KeyboardEvent {
+    pub fn new(event: ExternalKeyboardEvent) -> Self {
+        Self {
+            event: keyboard_event,
+            modifiers: event.modifiers,
+            webdriver_id: None,
+        }
+    }
+   fn foo(&self) {
+      self.event.foo()
+   }
+}

	fn handle_overridable_key_bindings(&self, webview: ::servo::WebView, event: KeyboardEvent) {
	let origin = webview.rect().min.ceil().to_i32();
	ShortcutMatcher::from_event(event)
	.shortcut(CMD_OR_CONTROL, '=', \|\| {
	webview.set_zoom(1.1);
	})
	.shortcut(CMD_OR_CONTROL, '+', \|\| {
	webview.set_zoom(1.1);
	})

Uh oh!

Conversation

PotatoCP commented Jun 12, 2025

Uh oh!

PotatoCP commented Jun 12, 2025

Uh oh!

PotatoCP commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longvatrong111 left a comment

Choose a reason for hiding this comment

Uh oh!

longvatrong111 Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PotatoCP Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longvatrong111 Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PotatoCP Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longvatrong111 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PotatoCP Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PotatoCP commented Jun 16, 2025

Uh oh!

PotatoCP Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longvatrong111 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaochengh commented Jun 18, 2025

Uh oh!

PotatoCP commented Jun 12, 2025 •

edited

Loading

longvatrong111 Jun 12, 2025 •

edited

Loading

PotatoCP Jun 12, 2025 •

edited

Loading

longvatrong111 Jun 13, 2025 •

edited

Loading

jdm Jun 13, 2025 •

edited

Loading

PotatoCP Jun 14, 2025 •

edited

Loading

PotatoCP Jun 16, 2025 •

edited

Loading

PotatoCP Jun 16, 2025 •

edited

Loading