8000 Webdriver: Keyboard Action use `webview::notify_input_event` instead of directly sent to constellation by PotatoCP · Pull Request #37908 · servo/servo · GitHub
[go: up one dir, main page]
Skip to content

Webdriver: Keyboard Action use webview::notify_input_event instead of directly sent to constellation #37908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 8, 2025
Merged

Webdriver: Keyboard Action use webview::notify_input_event instead of directly sent to constellation #37908

merged 3 commits into from
Jul 8, 2025

Conversation

PotatoCP
Copy link
Contributor
@PotatoCP PotatoCP commented Jul 7, 2025 •
edited
Loading

Previously, we KeyboardAction will be forwarded to constellation by the embedder. Now we use webview.notify_input_event, which will send WebDriverCommandMsg::ForwardInputEvent for KeyboardAction

Fixes: part of #37370

@PotatoCP
Keyboard Action use notify_input_event
6a8472e 
Signed-off-by: PotatoCP <kenzieradityatirtarahardja18@gmail.com>
@PotatoCP PotatoCP force-pushed the keyboard-action-move branch from 497aab1 to 6a8472e Compare July 7, 2025 02:21
@PotatoCP
Copy link
Contributor Author
PotatoCP commented Jul 7, 2025 •
edited
Loading

Ideally, we should use this function, since it already handled many pre-processing steps like capitalizing the letter when pressing shift.

servo/ports/servoshell/desktop/headed_window.rs

Lines 210 to 257 in ee8bd14

fn handle_keyboard_input(&self, state: Rc<RunningAppState>, winit_event: KeyEvent) {
// First, handle servoshell key bindings that are not overridable by, or visible to, the page.
let mut keyboard_event =
keyboard_event_from_winit(&winit_event, self.modifiers_state.get());
if self.handle_intercepted_key_bindings(state.clone(), &keyboard_event) {
return;
}
// Then we deliver character and keyboard events to the page in the focused webview.
let Some(webview) = state.focused_webview() else {
return;
};
if let Some(input_text) = &winit_event.text {
for character in input_text.chars() {
self.handle_received_character(&webview, character);
}
}
if keyboard_event.event.state == KeyState::Down &&
keyboard_event.event.key == Key::Unidentified
{
// If pressed and probably printable, we expect a ReceivedCharacter event.
// Wait for that to be received and don't queue any event right now.
self.last_pressed
.set(Some((keyboard_event, Some(winit_event.logical_key))));
return;
} else if keyboard_event.event.state == KeyState::Up &&
keyboard_event.event.key == Key::Unidentified
{
// If release and probably printable, this is following a ReceiverCharacter event.
if let Some(key) = self.keys_down.borrow_mut().remove(&winit_event.logical_key) {
keyboard_event.event.key = key;
}
}
if keyboard_event.event.key != Key::Unidentified {
self.last_pressed.set(None);
let xr_poses = self.xr_window_poses.borrow();
for xr_window_pose in &*xr_poses {
xr_window_pose.handle_xr_rotation(&winit_event, self.modifiers_state.get());
}
webview.notify_input_event(InputEvent::Keyboard(keyboard_event));
}
// servoshell also has key bindings that are visible to, and overridable by, the page.
// See the handler for EmbedderMsg::Keyboard in webview.rs for those.
}

However, this function is only available on headed_windows. Also, we need to send the keyboard action based on the webviewid that is sent from the webdriver.

@PotatoCP
Copy link
Contributor Author
PotatoCP commented Jul 7, 2025

Try: https://github.com/PotatoCP/servo/actions/runs/16105983287
cc: @xiaochengh @longvatrong111 @yezhizhen

@PotatoCP PotatoCP marked this pull request as ready for review July 7, 2025 02:55
@PotatoCP PotatoCP requested a review from atbrakhi as a code owner July 7, 2025 02:55
PotatoCP added 2 commits July 7, 2025 11:04
@PotatoCP
Update wpt expectation
7ac7dd4 
Signed-off-by: PotatoCP <kenzieradityatirtarahardja18@gmail.com>
@PotatoCP
add todo
2670462 
Signed-off-by: PotatoCP <kenzieradityatirtarahardja18@gmail.com>
@mrobinson
Copy link
Member

Ideally, we should use this function, since it already handled many things like capitalizing the letter when pressing shift.

Do you mind linking to the code that is capitalizing letters when shift is pressed just so I understand this issue a bit better?

@PotatoCP
Copy link
Contributor Author
PotatoCP commented Jul 7, 2025

Ideally, we should use this function, since it already handled many things like capitalizing the letter when pressing shift.

Do you mind linking to the code that is capitalizing letters when shift is pressed just so I understand this issue a bit better?

Sorry, I think I misunderstood something, it should be already capitalized before. But I still think the behavior of both user input in servoshell and webdriver should be similar (right?). For example, we have this handling of key binding

servo/ports/servoshell/desktop/headed_window.rs

Lines 284 to 402 in ee8bd14

fn handle_intercepted_key_bindings(
&self,
state: Rc<RunningAppState>,
key_event: &KeyboardEvent,
) -> bool {
let Some(focused_webview) = state.focused_webview() else {
return false;
};
let mut handled = true;
ShortcutMatcher::from_event(key_event.event.clone())
.shortcut(CMD_OR_CONTROL, 'R', || focused_webview.reload())
.shortcut(CMD_OR_CONTROL, 'W', || {
state.close_webview(focused_webview.id());
})
.shortcut(CMD_OR_CONTROL, 'P', || {
let rate = env::var("SAMPLING_RATE")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(10);
let duration = env::var("SAMPLING_DURATION")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(10);
focused_webview.toggle_sampling_profiler(
Duration::from_millis(rate),
Duration::from_secs(duration),
);
})
.shortcut(CMD_OR_CONTROL, 'X', || {
focused_webview
.notify_input_event(InputEvent::EditingAction(servo::EditingActionEvent::Cut))
})
.shortcut(CMD_OR_CONTROL, 'C', || {
focused_webview
.notify_input_event(InputEvent::EditingAction(servo::EditingActionEvent::Copy))
})
.shortcut(CMD_OR_CONTROL, 'V', || {
focused_webview
.notify_input_event(InputEvent::EditingAction(servo::EditingActionEvent::Paste))
})
.shortcut(Modifiers::CONTROL, Key::F9, || {
focused_webview.capture_webrender();
})
.shortcut(Modifiers::CONTROL, Key::F10, || {
focused_webview.toggle_webrender_debugging(WebRenderDebugOption::RenderTargetDebug);
})
.shortcut(Modifiers::CONTROL, Key::F11, || {
focused_webview.toggle_webrender_debugging(WebRenderDebugOption::TextureCacheDebug);
})
.shortcut(Modifiers::CONTROL, Key::F12, || {
focused_webview.toggle_webrender_debugging(WebRenderDebugOption::Profiler);
})
.shortcut(CMD_OR_ALT, Key::ArrowRight, || {
focused_webview.go_forward(1);
})
.optional_shortcut(
cfg!(not(target_os = "windows")),
CMD_OR_CONTROL,
']',
|| {
focused_webview.go_forward(1);
},
)
.shortcut(CMD_OR_ALT, Key::ArrowLeft, || {
focused_webview.go_back(1);
})
.optional_shortcut(
cfg!(not(target_os = "windows")),
CMD_OR_CONTROL,
'[',
|| {
focused_webview.go_back(1);
},
)
.optional_shortcut(
self.get_fullscreen(),
Modifiers::empty(),
Key::Escape,
|| focused_webview.exit_fullscreen(),
)
// Select the first 8 tabs via shortcuts
.shortcut(CMD_OR_CONTROL, '1', || state.focus_webview_by_index(0))
.shortcut(CMD_OR_CONTROL, '2', || state.focus_webview_by_index(1))
.shortcut(CMD_OR_CONTROL, '3', || state.focus_webview_by_index(2))
.shortcut(CMD_OR_CONTROL, '4', || state.focus_webview_by_index(3))
.shortcut(CMD_OR_CONTROL, '5', || state.focus_webview_by_index(4))
.shortcut(CMD_OR_CONTROL, '6', || state.focus_webview_by_index(5))
.shortcut(CMD_OR_CONTROL, '7', || state.focus_webview_by_index(6))
.shortcut(CMD_OR_CONTROL, '8', || state.focus_webview_by_index(7))
// Cmd/Ctrl 9 is a bit different in that it focuses the last tab instead of the 9th
.shortcut(CMD_OR_CONTROL, '9', || {
let len = state.webviews().len();
if len > 0 {
state.focus_webview_by_index(len - 1)
}
})
.shortcut(Modifiers::CONTROL, Key::PageDown, || {
if let Some(index) = state.get_focused_webview_index() {
state.focus_webview_by_index((index + 1) % state.webviews().len())
}
})
.shortcut(Modifiers::CONTROL, Key::PageUp, || {
if let Some(index) = state.get_focused_webview_index() {
let new_index = if index == 0 {
state.webviews().len() - 1
} else {
index - 1
};
state.focus_webview_by_index(new_index)
}
})
.shortcut(CMD_OR_CONTROL, 'T', || {
state.create_and_focus_toplevel_webview(Url::parse("servo:newtab").unwrap());
})
.shortcut(CMD_OR_CONTROL, 'Q', || state.servo().start_shutting_down())
.otherwise(|| handled = false);
handled
}

xiaochengh
xiaochengh approved these changes Jul 8, 2025
View reviewed changes
Copy link
Contributor
@xiaochengh xiaochengh left a comment

Choose a reason for hiding this comment

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

This PR on its own makes a lot of sense to me.

Regarding the modifier keys, I think the modifier-related mapping should be done inside Servo. The embedders may or may not handle modifiers, but Servo should not rely on embedders for handling the modifiers.

@xiaochengh xiaochengh added this pull request to the merge queue Jul 8, 2025
Merged via the queue into servo:main with commit 1773ea4 Jul 8, 2025
22 checks passed
@PotatoCP PotatoCP deleted the keyboard-action-move branch July 8, 2025 09:34
@mrobinson
Copy link
Member

Regarding the modifier keys, I think the modifier-related mapping should be done inside Servo. The embedders may or may not handle modifiers, but Servo should not rely on embedders for handling the modifiers.

I agree with this. In general, I think we should move more key handling into the script itself. This includes keys for things like scrolling, etc.

@PotatoCP PotatoCP mentioned this pull request Jul 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaochengh xiaochengh xiaochengh approved these changes

@atbrakhi atbrakhi Awaiting requested review from atbrakhi atbrakhi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PotatoCP @mrobinson @xiaochengh
0