Browser chrome: increase margin of context menus when they are triggered with touch (as we do with context menus in content)

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
3 months ago
8 days ago

People

(Reporter: phlsa, Assigned: johannh)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 56
Unspecified
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-visual][p3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Bug 1256754 gave us touch-friendly context menus in the content viewport.
We should extend this behavior to context menus that come from the browser chrome (from long-press on a tab, for example)

Comment 1

3 months ago
Is this supposed to be tracked for Photon?
Flags: needinfo?(philipp)
Hm... I thought that bug 1256754 was tracked for Photon, but it looks like it isn't.
But since we are explicitly creating a touch mode, I would say yes, it should be tracked.
Flags: needinfo?(philipp)

Updated

3 months ago
Blocks: 1352356
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [photon-visual][p3]

Updated

3 months ago
QA Contact: brindusa.tot
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

Updated

3 months ago
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
There isn't really a nice solution for this. I could listen for the "contextmenu" event but I'm struggling to connect the elements that get the "contextmenu" event with their corresponding contextmenus (unless I add them all to a big map which doesn't scale for obvious reasons). So instead, I propagate the inputSource of the contextmenu event to the popupshowing event.

I'll add a test if you're telling me that it's the right approach to the problem.
Comment on attachment 8877141 [details]
Bug 1364896 - Part 1 - Propagate trigger event inputSource to XUL popupshowing events.

Masayuki, it seems Neil is away, so would you mind taking a look at this patch or redirecting me to someone else?

Thank you!
Attachment #8877141 - Flags: review?(masayuki)

Comment 7

2 months ago
mozreview-review
Comment on attachment 8877141 [details]
Bug 1364896 - Part 1 - Propagate trigger event inputSource to XUL popupshowing events.

https://reviewboard.mozilla.org/r/148474/#review153276

I'm not so familiar with this class, but I have no idea who is familiar with this class except Enn. I think that this patch must be fine.

::: layout/xul/nsXULPopupManager.h:724
(Diff revision 1)
>    /**
>     * Fire a popupshowing event on the popup and then open the popup.
>     *
>     * aPopup - the popup to open
> +   * aTriggerEvent - the event that triggered the showing event.
> +   *                 This is currently used to propagate the
> +   *                 inputSource attribute. May be null.
>     * aIsContextMenu - true for context menus
>     * aSelectFirstItem - true to select the first item in the menu
>     */
>    void FirePopupShowingEvent(nsIContent* aPopup,
> +                             nsIDOMEvent* aTriggerEvent,
>                               bool aIsContextMenu,
>                               bool aSelectFirstItem);

Although, I like this argument order, other methods take aTriggerEvent at the last. Up to you.

::: layout/xul/nsXULPopupManager.cpp:1480
(Diff revision 1)
> +    nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aTriggerEvent);
> +    if (mouseEvent) {
> +      uint16_t inputSource = nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
> +      mouseEvent->GetMozInputSource(&inputSource);

Use WidgetMouseEvent directly. Then,

WidgetMouseEvent mouseEvent =
  aTriggerEvent->WidgetEventPtr()->AsMouseEventBase();
if (mouseEvent) {
  event.inputSource = mouseEvent.inputSource;
}
Attachment #8877141 - Flags: review?(masayuki) → review+

Comment 8

2 months ago
mozreview-review
Comment on attachment 8877142 [details]
Bug 1364896 - Part 2 - Increase margin of all context menus that are triggered through touch.

https://reviewboard.mozilla.org/r/148476/#review153420

::: browser/base/content/browser.js:1636
(Diff revision 1)
>        Win7Features.onOpenWindow();
>  
>      FullScreen.init();
>      PointerLock.init();
>  
> +    if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {

Why is this limited to Windows 10?

::: browser/base/content/browser.js:8215
(Diff revision 1)
> +  },
> +
> +  handleEvent(event) {
> +    let target = event.target;
> +
> +    // The contentAreaContextMenu is assigned touchmode in nsContextMenu.js.

Why, though? Can we get rid of this code in nsContextMenu.js?
Attachment #8877142 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #8)
> Comment on attachment 8877142 [details]
> Bug 1364896 - Increase margin of all context menus that are triggered
> through touch.
> 
> https://reviewboard.mozilla.org/r/148476/#review153420
> 
> ::: browser/base/content/browser.js:1636
> (Diff revision 1)
> >        Win7Features.onOpenWindow();
> >  
> >      FullScreen.init();
> >      PointerLock.init();
> >  
> > +    if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {
> 
> Why is this limited to Windows 10?

I think this behavior (resizing the context menu on touch) is only natively happening on Win 10, though I could be wrong. We could extend it to Win 8 (or Linux)? Philipp, do you have a suggestion on where we should enable this?

> ::: browser/base/content/browser.js:8215
> (Diff revision 1)
> > +  },
> > +
> > +  handleEvent(event) {
> > +    let target = event.target;
> > +
> > +    // The contentAreaContextMenu is assigned touchmode in nsContextMenu.js.
> 
> Why, though? Can we get rid of this code in nsContextMenu.js?

The triggerEvent (from which I get the sourceInput) isn't passed for content contextmenu events (maybe because it's happening in a different process?). I wasn't able to trivially solve this (especially since Enn isn't available), hence I chose to not spend more time on it and leave it working as it is. I can look into it at least a bit more if you're concerned about this.
Flags: needinfo?(philipp)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
As I suspected, this had something to do with the contextmenu event coming from the content process. nsContextMenu.js uses a CPOW contextmenu event to extract the position it should place the menu at. Because it can't pass the a CPOW to the native code it just doesn't pass it. That's why our native code can't extract the inputSource from that event. My fix is to just synthesize a new event with the values we need. It's a little hacky, but I much prefer that to setting the touchmode in two completely different places. Also, in the future, other code could theoretically add more attributes to that synthetic event as needed.

Comment 13

2 months ago
mozreview-review
Comment on attachment 8877142 [details]
Bug 1364896 - Part 2 - Increase margin of all context menus that are triggered through touch.

https://reviewboard.mozilla.org/r/148476/#review154486
Attachment #8877142 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 months ago
mozreview-review-reply
Comment on attachment 8877141 [details]
Bug 1364896 - Part 1 - Propagate trigger event inputSource to XUL popupshowing events.

https://reviewboard.mozilla.org/r/148474/#review153276

> Although, I like this argument order, other methods take aTriggerEvent at the last. Up to you.

Yeah, you're right, I also like it better but we should probably stay consistent (I guess it's last because it's optional).

> Use WidgetMouseEvent directly. Then,
> 
> WidgetMouseEvent mouseEvent =
>   aTriggerEvent->WidgetEventPtr()->AsMouseEventBase();
> if (mouseEvent) {
>   event.inputSource = mouseEvent.inputSource;
> }

Good idea, I assume you meant WidgetMouseEventBase* mouseEvent =
Not landing since we're waiting for Philipp's input. I'm writing a test for this in the meantime.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Joel, requesting your review for adding the synthesizeNativeTap function to EventUtils.js

Comment 22

2 months ago
mozreview-review
Comment on attachment 8880769 [details]
Bug 1364896 - Part 3 - Add a test for contextmenu touchmode.

https://reviewboard.mozilla.org/r/152142/#review157150

2 questions/concerns in EventUtils.js- please test carefully on all platforms.

::: testing/mochitest/tests/SimpleTest/EventUtils.js:639
(Diff revision 1)
>  }
>  
> +// Convert (aX, aY), in CSS pixels relative to aElement's bounding rect,
> +// to device pixels relative to the screen.
> +function _getElementCoordinatesWithOffset(aX, aY, aElement) {
> +  let targetWindow = aElement.ownerDocument.defaultView;

in synthesizeNativeMouseMove we had previously used window.mozInnerScreenX whereas now we do aElement.ownerDocument.defaultView.mozInnerScreenX.

window was not well defined previously- do you believe that aElement.ownerDocument.defaultView will work properly?

::: testing/mochitest/tests/SimpleTest/EventUtils.js:640
(Diff revision 1)
>  
> +// Convert (aX, aY), in CSS pixels relative to aElement's bounding rect,
> +// to device pixels relative to the screen.
> +function _getElementCoordinatesWithOffset(aX, aY, aElement) {
> +  let targetWindow = aElement.ownerDocument.defaultView;
> +  let scale = targetWindow.devicePixelRatio;

previously we had scale=utils.screenPixelsPerCSSPixel, does window.devicePixelRatio do the same thing?  This could change a lot of tests
Attachment #8880769 - Flags: review?(jmaher) → review-

Comment 23

2 months ago
mozreview-review
Comment on attachment 8880769 [details]
Bug 1364896 - Part 3 - Add a test for contextmenu touchmode.

https://reviewboard.mozilla.org/r/152142/#review157486

::: browser/base/content/test/contextMenu/browser_contextmenu_touch.js:72
(Diff revision 1)
> +  let urlbar = document.getElementById("urlbar");
> +  let textBox = document.getAnonymousElementByAttribute(urlbar,
> +                                      "anonid", "textbox-input-box");
> +  let menu = document.getAnonymousElementByAttribute(textBox,
> +                                      "anonid", "input-box-contextmenu");
> +  await openAndCheckContextMenu(menu, textBox, 32, 8);

What are these magic numbers? How about implementing synthesizeNativeTapAtCenter?
Attachment #8880769 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)

Comment 25

2 months ago
mozreview-review
Comment on attachment 8880769 [details]
Bug 1364896 - Part 3 - Add a test for contextmenu touchmode.

https://reviewboard.mozilla.org/r/152142/#review157528

the eventutils.js code looks good
Attachment #8880769 - Flags: review?(jmaher) → review+
Thanks for the quick review!

I think the changes window => ownerDocument.defaultView and screenPixelsPerCSSPixel => devicePixelRatio _should_ not have an impact on the other tests, but I agree that we shouldn't take chances (I certainly don't want to have to clean it up). After the changes it didn't really make sense to have a shared function for getting the element position anymore.
(In reply to Johann Hofmann [:johannh] from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > Why is this limited to Windows 10?
> 
> I think this behavior (resizing the context menu on touch) is only natively
> happening on Win 10, though I could be wrong. We could extend it to Win 8
> (or Linux)? Philipp, do you have a suggestion on where we should enable this?

If we can (easily) enable it on Win 8 and Linux as well, then I think we should do it. But Win10 is by far the most high-value target and we don't need to jump through any extra hoops right now to get it enabled on other platforms (esp. Win8)
(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #27)
> (In reply to Johann Hofmann [:johannh] from comment #9)
> > (In reply to Dão Gottwald [::dao] from comment #8)
> > > Why is this limited to Windows 10?
> > 
> > I think this behavior (resizing the context menu on touch) is only natively
> > happening on Win 10, though I could be wrong. We could extend it to Win 8
> > (or Linux)? Philipp, do you have a suggestion on where we should enable this?
> 
> If we can (easily) enable it on Win 8 and Linux as well, then I think we
> should do it. But Win10 is by far the most high-value target and we don't
> need to jump through any extra hoops right now to get it enabled on other
> platforms (esp. Win8)

Ok, thanks! I think I'd rather land this right now with, Win 10 support, and make a new bug for enabling on Win 8 and Linux. Testing this on different platforms is much easier once it's in the tree and can be build using artifact mode (and as you say Win 10 is the most valuable target).
Flags: needinfo?(philipp)

Comment 29

2 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f52f28a9eaa
Part 1 - Propagate trigger event inputSource to XUL popupshowing events. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/134d9ee1e545
Part 2 - Increase margin of all context menus that are triggered through touch. r=dao
https://hg.mozilla.org/integration/autoland/rev/5c870a786e94
Part 3 - Add a test for contextmenu touchmode. r=dao,jmaher

Comment 30

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f52f28a9eaa
https://hg.mozilla.org/mozilla-central/rev/134d9ee1e545
https://hg.mozilla.org/mozilla-central/rev/5c870a786e94
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

2 months ago
Depends on: 1376466
Backed out for causing bug 1376466.
https://hg.mozilla.org/mozilla-central/rev/8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
Status: RESOLVED → REOPENED
status-firefox56: fixed → ---
Flags: needinfo?(jhofmann)
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 months ago
mozreview-review
Comment on attachment 8881522 [details]
Bug 1364896 - Part 4 - Add a popupshowing listener to the entire document instead of individual elements.

https://reviewboard.mozilla.org/r/152678/#review157896

LGTM
Attachment #8881522 - Flags: review?(nhnt11) → review+

Comment 39

2 months ago
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 40

2 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1688d461e24e
Part 1 - Propagate trigger event inputSource to XUL popupshowing events. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/d872a70d8962
Part 2 - Increase margin of all context menus that are triggered through touch. r=dao
https://hg.mozilla.org/integration/autoland/rev/686ac124663b
Part 3 - Add a test for contextmenu touchmode. r=dao,jmaher
https://hg.mozilla.org/integration/autoland/rev/3b533f122a59
Part 4 - Add a popupshowing listener to the entire document instead of individual elements. r=nhnt11

Comment 41

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1688d461e24e
https://hg.mozilla.org/mozilla-central/rev/d872a70d8962
https://hg.mozilla.org/mozilla-central/rev/686ac124663b
https://hg.mozilla.org/mozilla-central/rev/3b533f122a59
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: needinfo?(jhofmann)
Blocks: 1383003
QA Contact: brindusa.tot → ovidiu.boca
You need to log in before you can comment on or make changes to this bug.