Closed Bug 1276533 Opened 3 years ago Closed 3 years ago

Tests: Synthesizing mouse click may fail on smaller screens

Categories

(DevTools :: General, defect, trivial)

defect
Not set
trivial

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: steve.j.melia, Assigned: steve.j.melia)

Details

Attachments

(1 file, 1 obsolete file)

Happen to be using a laptop 1366x768 and encountered a test failure in devtools/client/inspector/test/browser_inspector_initialization.js when trying to simulate "right-click; inspect element" as the #closing tag was not in the visible area.
Attached patch 1276533-synthesizeMouse.patch (obsolete) — Splinter Review
Use scrollIntoView to make sure the element is in the visible area before simulating the mouse click.
Attachment #8757729 - Flags: review?(pbrosset)
Comment on attachment 8757729 [details] [diff] [review]
1276533-synthesizeMouse.patch

Review of attachment 8757729 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me, but I think this is something EventUtils should (and might) handle.
I don't remember, off the top of my head, cases where we had to scroll nodes into view before using EventUtils to click on them.
Alex might know more than me on this.
Attachment #8757729 - Flags: review?(pbrosset) → review?(poirot.alex)
Comment on attachment 8757729 [details] [diff] [review]
1276533-synthesizeMouse.patch

Review of attachment 8757729 [details] [diff] [review]:
-----------------------------------------------------------------

In theory, you may try to fire a click event on a hidden element.
A webpage, for example, may do element.click() or use dispatchEvent on some dom nodes that are out of the viewport.
Given that EventUtils are low lever helpers, it doesn't make sense to automagically scoll to the element.
I'm also wondering if we should do that in synthesizeMouseEvent.
We may keep that for clickOnInspectMenuItem function.

I'm wondering if any test break with your current patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ce88dfcfc5
Comment on attachment 8757729 [details] [diff] [review]
1276533-synthesizeMouse.patch

Review of attachment 8757729 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like it doesn't break any test and it seems like a good assumption to ensure nodes are in the viewport for inspector tests.

Could you just add a comment in synthesizeMouse comment about the automatic scroll if not in viewport?

Thanks.
Attachment #8757729 - Flags: review?(poirot.alex) → review+
Cool! Updated the comment, thanks Alex.
Attachment #8757729 - Attachment is obsolete: true
Attachment #8758432 - Flags: review?(poirot.alex)
Attachment #8758432 - Flags: review?(poirot.alex) → review+
Keywords: checkin-needed
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/cb0b25c9093f
Update test actor to scroll element into view before simulating mouse click;r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb0b25c9093f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.