Implement MouseEvent buttons attribute #23249

georgeroman · 2019-04-23T16:51:25Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #22884

This change is

highfive · 2019-04-23T16:51:31Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webidls/MouseEvent.webidl, components/constellation/constellation.rs, components/script/dom/mouseevent.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: components/constellation/constellation.rs
@KiChjang: components/script/dom/webidls/MouseEvent.webidl, components/script_traits/lib.rs, components/script/dom/mouseevent.rs

highfive · 2019-04-23T16:51:34Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

georgeroman · 2019-04-23T17:12:23Z

I have a question regarding the next steps.
So the MouseEvent needs to have access to the bitmask representing the currently pressed buttons. I could send this information directly in the MouseButtonEvent and MouseMoveEvent but in this case the mouse event triggered by a synthetic click won't have access to it, as it's created on the spot. So the only way to access it is by communicating directly with the constellation. Is this the proper way to it?

jdm · 2019-04-23T19:04:48Z

@georgeroman I think synthetic clicks should be treated differently, like whatwg/html#4452 describes. I would expect a synthetic click to produce an event that only claims to have a left mouse button active.

jdm · 2019-04-30T16:05:18Z

r? @Manishearth

Manishearth

Looks good! I can't find how right/aux events get triggered, though. Do we just not support those yet?

Manishearth · 2019-04-30T17:09:00Z

components/script_traits/lib.rs

    ),
    /// The mouse was moved over a point (or was moved out of the recognizable region).
-    MouseMoveEvent(Option<Point2D<f32>>, Option<UntrustedNodeAddress>),
+    MouseMoveEvent(Option<Point2D<f32>>, Option<UntrustedNodeAddress>, u16),


leave an inline comment on the u16 saying it's a button mask of MouseButton values

(or mention this in the doc comment)

Manishearth · 2019-04-30T17:09:00Z

components/compositing/compositor.rs

@@ -701,7 +702,7 @@ impl<Window: WindowMethods> IOCompositor<Window> {
        let results = self.hit_test_at_point(cursor);
        if let Some(item) = results.items.first() {
            let node_address = Some(UntrustedNodeAddress(item.tag.0 as *const c_void));
-            let event = MouseMoveEvent(Some(item.point_in_viewport.to_untyped()), node_address);
+            let event = MouseMoveEvent(Some(item.point_in_viewport.to_untyped()), node_address, 0);


Can you use MouseButton::Left as u16 so it's clear what the zero means?

Manishearth · 2019-04-30T17:09:00Z

components/compositing/compositor.rs

@@ -658,6 +658,7 @@ impl<Window: WindowMethods> IOCompositor<Window> {
            result.point_in_viewport.to_untyped(),
            Some(UntrustedNodeAddress(result.tag.0 as *const c_void)),
            Some(result.point_relative_to_item.to_untyped()),
+            0,


MouseButton::Left as u16

KwanEsq · 2019-04-30T18:38:18Z

components/script_traits/lib.rs

-    Right,
+    Right = 2,
+    /// The middle mouse button.
+    Middle = 4,


Shouldn't this use a new MouseButtonMask enum (or something), rather than mangling this one?

I could add a new enum for this purpose, but it would be exactly the same as this one since MouseButtonEvent (which I use for updating the buttons bitmask) stores the button that triggered the event as a MouseButton.

Then don't alter the values of this enum, just map them ala MouseButton::Left => 1 etc.

KwanEsq · 2019-04-30T18:46:42Z

Hmm, I guess to make comments on code show up in the conversation I have to "start a review" rather than "add single comment"? (Which I don't feel qualified for)
But I would like to highlight my comment. The existing MouseButton enum seems clearly to be for the button attribute (which has very different button->value mappings to the buttons attribute), so it seems like this PR should add and use a new enum.

Edit: okay, now my comment appears here, it just took a while.

georgeroman · 2019-05-03T13:20:23Z

@Manishearth I also noticed that all mouse events have MouseButton::Left as triggering button, so probably the other buttons are not supported yet.

KwanEsq · 2019-05-03T15:13:49Z

Ah yeah, I should have mentioned that as well. Middle-button isn't supported at all, and right-button is incorrectly reported as MouseButton::Left (zero). I was experimenting with implementing them, and the auxclick event. Though given none of the wpt mouse event tests run yet because testdriver isn't yet supported (Support Webdriver based tests project), I wasn't sure about submitting it yet.

Manishearth · 2019-05-09T20:01:13Z

@bors-servo r+

bors-servo · 2019-05-09T20:01:16Z

📌 Commit fd3b4e2 has been approved by Manishearth

bors-servo · 2019-05-10T05:53:09Z

⌛ Testing commit fd3b4e2 with merge 2872ffb...


        Auto merge of #23249 - georgeroman:implement_mouseevent_buttons, r=Ma…

…nishearth Implement MouseEvent buttons attribute  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22884    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23249)

bors-servo · 2019-05-10T06:16:27Z

💔 Test failed - linux-rel-css

CYBAI · 2019-05-10T06:28:34Z

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "MouseEvent interface: attribute buttons", 
    "test": "/uievents/idlharness.window.html", 
    "line": 257471, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "MouseEvent interface: new MouseEvent(\"event\") must inherit property \"buttons\" with the proper type", 
    "test": "/uievents/idlharness.window.html", 
    "line": 257485, 
    "action": "test_result", 
    "expected": "FAIL"
}


        Implement MouseEvent buttons attribute

ferjm · 2019-05-16T09:39:35Z

@bors-servo retry

CYBAI · 2019-05-16T10:43:25Z

Due to force-pushed, maybe we need to r+ (or maybe r=Manishearth) again 👀 (sorry I can only help to try 😭)

ferjm · 2019-05-16T10:54:15Z

@bors-servo r+

bors-servo · 2019-05-16T10:54:17Z

📌 Commit be3cb00 has been approved by ferjm

ferjm · 2019-05-16T10:54:37Z

@bors-servo r=Manishearth

bors-servo · 2019-05-16T10:54:38Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #23403

bors-servo · 2019-05-16T10:54:39Z

📌 Commit be3cb00 has been approved by Manishearth


        Auto merge of #23249 - georgeroman:implement_mouseevent_buttons, r=Ma…

…nishearth Implement MouseEvent buttons attribute  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #22884    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23249)

bors-servo · 2019-05-16T13:17:19Z

⌛ Testing commit be3cb00 with merge 3a5ebb1...

bors-servo · 2019-05-16T15:39:11Z

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: Manishearth
Pushing 3a5ebb1 to master...

highfive assigned emilio Apr 23, 2019

highfive added the S-awaiting-review label Apr 23, 2019

georgeroman changed the title ~~[WIP] Implement MouseEvent buttons attribute~~ Implement MouseEvent buttons attribute Apr 25, 2019

highfive assigned Manishearth and unassigned emilio Apr 30, 2019

Manishearth reviewed Apr 30, 2019

View changes

KwanEsq reviewed Apr 30, 2019

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels May 9, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels May 10, 2019

highfive added S-awaiting-review and removed S-tests-failed labels May 10, 2019

Implement MouseEvent buttons attribute

Loading status checks…

be3cb00

highfive assigned ferjm and unassigned Manishearth May 16, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels May 16, 2019

highfive assigned Manishearth and unassigned ferjm May 16, 2019

highfive removed the S-awaiting-merge label May 16, 2019

servo / servo

Implement MouseEvent buttons attribute #23249

Implement MouseEvent buttons attribute #23249

georgeroman commented Apr 23, 2019 •

edited by SimonSapin

highfive commented Apr 23, 2019

highfive commented Apr 23, 2019

georgeroman commented Apr 23, 2019

jdm commented Apr 23, 2019

jdm commented Apr 30, 2019

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KwanEsq commented Apr 30, 2019 •

edited

georgeroman commented May 3, 2019

KwanEsq commented May 3, 2019 •

edited

Manishearth commented May 9, 2019

bors-servo commented May 9, 2019

bors-servo commented May 10, 2019

bors-servo commented May 10, 2019

CYBAI commented May 10, 2019

ferjm commented May 16, 2019

CYBAI commented May 16, 2019 •

edited

ferjm commented May 16, 2019

bors-servo commented May 16, 2019

ferjm commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

servo / servo

Join GitHub today

Implement MouseEvent buttons attribute #23249

Implement MouseEvent buttons attribute #23249

Conversation

georgeroman commented Apr 23, 2019 • edited by SimonSapin

highfive commented Apr 23, 2019

highfive commented Apr 23, 2019

georgeroman commented Apr 23, 2019

jdm commented Apr 23, 2019

jdm commented Apr 30, 2019

Manishearth left a comment

This comment has been minimized.

Manishearth Apr 30, 2019

This comment has been minimized.

Manishearth Apr 30, 2019

This comment has been minimized.

Manishearth Apr 30, 2019

This comment has been minimized.

KwanEsq Apr 30, 2019 • edited

This comment has been minimized.

georgeroman May 2, 2019

This comment has been minimized.

KwanEsq May 2, 2019

KwanEsq commented Apr 30, 2019 • edited

georgeroman commented May 3, 2019

KwanEsq commented May 3, 2019 • edited

Manishearth commented May 9, 2019

bors-servo commented May 9, 2019

bors-servo commented May 10, 2019

bors-servo commented May 10, 2019

CYBAI commented May 10, 2019

ferjm commented May 16, 2019

CYBAI commented May 16, 2019 • edited

ferjm commented May 16, 2019

bors-servo commented May 16, 2019

ferjm commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

bors-servo commented May 16, 2019

georgeroman commented Apr 23, 2019 •

edited by SimonSapin

KwanEsq Apr 30, 2019 •

edited

KwanEsq commented Apr 30, 2019 •

edited

KwanEsq commented May 3, 2019 •

edited

CYBAI commented May 16, 2019 •

edited