| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
libservo: Unify notifications to the embedder that input events are handled #39720
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.
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
Conversation
mrobinson
requested review from
atbrakhi,
gterzian and
yezhizhen
as code owners
servo-highfive
added
the
S-awaiting-review
|
🔨 Triggering try run (#18355023864) for Linux (WPT) |
|
Test results for linux-wpt from try job (#18355023864): Flaky unexpected result (27)
Stable unexpected results that are known to be intermittent (27)
|
|
✨ Try run (#18355023864) succeeded. |
mrobinson
added
T-android
github-actions
bot
removed
T-android
|
🔨 Triggering try run (#18355912591) for Android, OpenHarmony |
|
|
mrobinson
force-pushed
the
input-events
branch
from
479053f to
e8d44a6
Compare
|
🔨 Triggering try run (#18356576564) for Android |
|
|
mrobinson
force-pushed
the
input-events
branch
from
e8d44a6 to
32061ae
Compare
|
🔨 Triggering try run (#18359075796) for OpenHarmony |
|
|
mrobinson
force-pushed
the
input-events
branch
from
32061ae to
411fc6c
Compare
mrobinson
added
T-android
|
🔨 Triggering try run (#18367514688) for Android, OpenHarmony |
Bencher Report
|
✨ Try run (#18367514688) succeeded. |
mrobinson
force-pushed
the
input-events
branch
from
411fc6c to
c7c2577
Compare
| } | ||
|
|
||
| impl From<InputEvent> for InputEventAndId { | ||
| fn from(specific: InputEvent) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the parameter be called input_event or event? The latter would allow using the shorthand syntax in the body, but I'm fine with keeping it if as is if there is reason to call it specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this. It's called event now. This was left over from a previous version of the change. Thanks for spotting it.
| pub struct InputEventResult: u8 { | ||
| /// Whether or not this input event's default behavior was prevented via script. | ||
| const DefaultPrevented = 1 << 0; | ||
| /// Whether or not the WebView had a default handler for this event. This can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean by a default handler here (as opposed to just 'handler')?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to clarify this a bit:
/// Whether or not the WebView handled this event. Some events have default handlers in
/// Servo, such as keyboard events that insert characters in `<input>` areas. When these
/// handlers are triggered, this flag is included. This can be used to prevent triggering
/// behavior (such as keybindings) when the WebView has already consumed the event for its
/// own purpose.
|
|
||
| /// Returns a unique ID. | ||
| pub fn next(&self) -> WebDriverMessageId { | ||
| pub fn next(&self) -> WebDriverInputEventId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is implemented on a struct called WebDriverMessageIdGenerator. Perhaps the next method should instead be implemented as a static method of WebDriverInputEventId struct itself and WebDriverMessageIdGenerator needs to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to address refactoring of the WebDriver code in the followup that we discussed using crossbeam senders/receiver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Sounds good to me.
| id_generator: WebDriverMessageIdGenerator, | ||
|
|
||
| current_action_id: Cell<Option<WebDriverMessageId>>, | ||
| current_action_id: Cell<Option<WebDriverInputEventId>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the name current_action_id still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it has drifted from it's original meaning even before this change, but I would like to eliminate it completely in a followup PR. I'm not 100% certain that it always references an input event (just that the ids sent to the embedding layer identify them), but I would like to replace these with crossbeam senders/receivers in any case.
| /// for the `exit_after_stable_image` option. | ||
| achieved_stable_image: Rc<Cell<bool>>, | ||
|
|
||
| /// A [`HashMap`] of pending WebDriver events. It is the WebDriver embedder's respnosibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// A [`HashMap`] of pending WebDriver events. It is the WebDriver embedder's respnosibility | |
| /// A [`HashMap`] of pending WebDriver events. It is the WebDriver embedder's responsibility |
I'm also not sure if phrase the WebDriver embedder's responsibility is intentional. Is it supposed to be just the embedder's responsibility (which in this case is the servoshell layer)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo.
I did mean the "WebDriver embedder" here to indicate the person embedding a WebDriver server in their application and handling requests. Not sure if there's a better way to describe that as "WebDriver server" and "WebDriver client" refer to different things.
| Scroll(ScrollEvent), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Deserialize, Serialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it logically correct to be able to clone() an input event by duplicating the id? It looks like we clone them in constellation_webview.rs but don't change the id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's fine. Event ids should only be allocated by the embedding layer. When cloning an event you are essentially cloning a "handle" to the same original event in the embedding layer.
| // Step 5 | ||
| true | ||
|
|
||
| // Step 5: Return true from the action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is not just the Return true from the action. step, but also the return false step for a cancelled "paste" event, as we will fall through here now. Maybe this can be clarified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've added a comment above for the "paste" case.
components/script/dom/event.rs
Outdated
| /// <https://dom.spec.whatwg.org/#canceled-flag> | ||
| canceled: Cell<EventDefault>, | ||
| /// This contains the `canceled` flags defined at | ||
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as informationa about whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as informationa about whether | |
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as information about whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
components/script/dom/event.rs
Outdated
| canceled: Cell<EventDefault>, | ||
| /// This contains the `canceled` flags defined at | ||
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as informationa about whether | ||
| /// the event has been handled yet. This information is use to prevent double-handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// the event has been handled yet. This information is use to prevent double-handling | |
| /// the event has been handled yet. This information is used to prevent double-handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
components/script/dom/event.rs
Outdated
| /// | ||
| /// [msg]: https://doc.servo.org/compositing/enum.ConstellationMsg.html#variant.KeyEvent | ||
| /// | ||
| /// An enum to indicate whether the result of firing an event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// An enum to indicate whether the result of firing an event. | |
| /// An enum to indicate the result of firing an event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
mrobinson
force-pushed
the
input-events
branch
from
c7c2577 to
97fceee
Compare
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mukilan Thanks for the review. I think I've addressed all of your comments. PTAL. Thanks.
| // Step 5 | ||
| true | ||
|
|
||
| // Step 5: Return true from the action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I've added a comment above for the "paste" case.
components/script/dom/event.rs
Outdated
| /// <https://dom.spec.whatwg.org/#canceled-flag> | ||
| canceled: Cell<EventDefault>, | ||
| /// This contains the `canceled` flags defined at | ||
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as informationa about whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
components/script/dom/event.rs
Outdated
| canceled: Cell<EventDefault>, | ||
| /// This contains the `canceled` flags defined at | ||
| /// <https://dom.spec.whatwg.org/#canceled-flag> as well as informationa about whether | ||
| /// the event has been handled yet. This information is use to prevent double-handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
components/script/dom/event.rs
Outdated
| /// | ||
| /// [msg]: https://doc.servo.org/compositing/enum.ConstellationMsg.html#variant.KeyEvent | ||
| /// | ||
| /// An enum to indicate whether the result of firing an event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| pub struct InputEventResult: u8 { | ||
| /// Whether or not this input event's default behavior was prevented via script. | ||
| const DefaultPrevented = 1 << 0; | ||
| /// Whether or not the WebView had a default handler for this event. This can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to clarify this a bit:
/// Whether or not the WebView handled this event. Some events have default handlers in
/// Servo, such as keyboard events that insert characters in `<input>` areas. When these
/// handlers are triggered, this flag is included. This can be used to prevent triggering
/// behavior (such as keybindings) when the WebView has already consumed the event for its
/// own purpose.
| } | ||
|
|
||
| impl From<InputEvent> for InputEventAndId { | ||
| fn from(specific: InputEvent) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this. It's called event now. This was left over from a previous version of the change. Thanks for spotting it.
| Scroll(ScrollEvent), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Deserialize, Serialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's fine. Event ids should only be allocated by the embedding layer. When cloning an event you are essentially cloning a "handle" to the same original event in the embedding layer.
| id_generator: WebDriverMessageIdGenerator, | ||
|
|
||
| current_action_id: Cell<Option<WebDriverMessageId>>, | ||
| current_action_id: Cell<Option<WebDriverInputEventId>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it has drifted from it's original meaning even before this change, but I would like to eliminate it completely in a followup PR. I'm not 100% certain that it always references an input event (just that the ids sent to the embedding layer identify them), but I would like to replace these with crossbeam senders/receivers in any case.
| /// for the `exit_after_stable_image` option. | ||
| achieved_stable_image: Rc<Cell<bool>>, | ||
|
|
||
| /// A [`HashMap`] of pending WebDriver events. It is the WebDriver embedder's respnosibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo.
I did mean the "WebDriver embedder" here to indicate the person embedding a WebDriver server in their application and handling requests. Not sure if there's a better way to describe that as "WebDriver server" and "WebDriver client" refer to different things.
| webdriver_event_id: Option<WebDriverInputEventId>, | ||
| ) { | ||
| let Some(webview) = self.webview_by_id(webview_id) else { | ||
| error!("Could not find WebView for WebDriver event: {input_event:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I've included it now.
|
Btw there is some conflict with merged #39509 |
servo-highfive
removed
the
S-awaiting-review
…andled Servo currently has three ways of notifying the embedder that input events have been processed. 1. `WebViewDelegate::notify_keyboard_event`: This is used to implement overridable keybindings in servoshell. It notifies the embedder when a keyboard event has been processed, but not "handled" by the engine. 2. WebDriver's command message senders and receivers, this is used to let WebDriver wait until events have been handled to continue with script execution. 3. Touch event processing notifications: This is used to serialize touch event processing. This change unifies the first two things with a new `WebViewDelegate::notify_input_event_handled` API. This API informs the embedder (via a generated id) that an input event has finished processing and what the result of the processing was. This allows embedders to do other things events once they have been processed. For example, embedders might want to chain unprocessed events up to parent widgets or to add support for overridable keybindings. As part of this the `canceled` flag in script's `Event` data structure is turned into a `bitflags` mirror of the new API type to describe the result of event handling. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
mrobinson
force-pushed
the
input-events
branch
from
97fceee to
dcc6b8b
Compare
servo-highfive
added
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
Servo currently has three ways of notifying the embedder that input
events have been processed.
WebViewDelegate::notify_keyboard_event: This is used to implementoverridable keybindings in servoshell. It notifies the embedder when
a keyboard event has been processed, but not "handled" by the engine.
let WebDriver wait until events have been handled to continue with
script execution.
event processing.
This change unifies the first two things with a new
WebViewDelegate::notify_input_event_handledAPI. This API informs theembedder (via a generated id) that an input event has finished
processing and what the result of the processing was.
This allows embedders to do other things with events once they have been
processed. For example, embedders might want to chain unconsumed events
up to parent widgets or to add support for overridable keybindings.
As part of this the
canceledflag in script'sEventdata structureis turned into a
bitflagsmirror of the new API type to describe theresult of event handling.
Testing: This shouldn't change observable behavior and is thus covered by existing tests.
The new API is consumed by servoshell, which uses it for tests.