Closed Bug 1364896 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, enhancement, P1)

Unspecified
Windows
enhancement

Tracking

()

VERIFIED FIXED
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: phlsa, Assigned: johannh)

References

(Blocks 1 open bug)

Details

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

Attachments

(4 files)

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)
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)
Blocks: photon-touch
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [photon-visual][p3]
QA Contact: brindusa.tot
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
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 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 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)
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 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 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.
Joel, requesting your review for adding the synthesizeNativeTap function to EventUtils.js
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 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 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)
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
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1376466
Backed out for causing bug 1376466.
https://hg.mozilla.org/mozilla-central/rev/8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
Status: RESOLVED → REOPENED
Flags: needinfo?(jhofmann)
Resolution: FIXED → ---
Target Milestone: Firefox 56 → ---
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
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+
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.
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
Flags: needinfo?(jhofmann)
Blocks: 1383003
QA Contact: brindusa.tot → ovidiu.boca
Johann, could you please advise us what exactly we should verify in this bug? I checked context menus on Windows 10 with touch and without touch and I did not observe any difference between them.
Flags: needinfo?(jhofmann)
(In reply to Brindusa Tot[:brindusat] from comment #42)
> Johann, could you please advise us what exactly we should verify in this
> bug? I checked context menus on Windows 10 with touch and without touch and
> I did not observe any difference between them.

Uhm, that's bad, it should be much larger when opened with a touch gesture (long tap). That's also covered by automated tests, are you sure you opened the context menu with a long tap?
Flags: needinfo?(jhofmann)
My mistake here. Verified again with a long tap, not with a long click or with a Stylus Pen, and the context menu is much larger in this case. I verified this on Windows 10 with touch and as I understand from bug this is fixed only on Windows 10. Is this correct?
Also, do we have some specific documentation with dimensions for touch? Or is it enough just to verify that the context menu is larger for touch?
Flags: needinfo?(jhofmann)
I don't think there are specific specs for context menus on touch, it was quite literally me and shorlander in Toronto adding pixels until we thought it felt decent.

I'll needinfo shorlander to be sure, but I'd say "it's a lot larger" is already a good verification ;)
Flags: needinfo?(jhofmann) → needinfo?(shorlander)
(In reply to Johann Hofmann [:johannh] from comment #45)
> I don't think there are specific specs for context menus on touch, it was
> quite literally me and shorlander in Toronto adding pixels until we thought
> it felt decent.
> 
> I'll needinfo shorlander to be sure, but I'd say "it's a lot larger" is
> already a good verification ;)

Hi! Yeah, we just kind of eyeballed this to roughly match the panel menu item height. So no specific height, just "taller".
Flags: needinfo?(shorlander)
Based on previous comments mark this verified on Windows 10 with latest Nightly 57.0a1.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1628171
See Also: 1628171
See Also: → 1397755
You need to log in before you can comment on or make changes to this bug.