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)
Tracking
()
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)
Reporter | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
Updated•7 years ago
|
QA Contact: brindusa.tot
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Updated•7 years ago
|
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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.
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years 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•7 years 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)
Assignee | ||
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(philipp)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years 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•7 years 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 =
Assignee | ||
Comment 17•7 years ago
|
||
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) |
Assignee | ||
Comment 21•7 years ago
|
||
Joel, requesting your review for adding the synthesizeNativeTap function to EventUtils.js
Comment 22•7 years 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•7 years 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•7 years 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+
Assignee | ||
Comment 26•7 years ago
|
||
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.
Reporter | ||
Comment 27•7 years ago
|
||
(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)
Assignee | ||
Comment 28•7 years ago
|
||
(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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 31•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jhofmann)
Updated•7 years ago
|
QA Contact: brindusa.tot → ovidiu.boca
Comment 42•7 years ago
|
||
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)
Assignee | ||
Comment 43•7 years ago
|
||
(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)
Comment 44•7 years ago
|
||
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)
Assignee | ||
Comment 45•7 years ago
|
||
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)
Comment 46•7 years ago
|
||
(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)
Comment 47•7 years ago
|
||
Based on previous comments mark this verified on Windows 10 with latest Nightly 57.0a1.
You need to log in
before you can comment on or make changes to this bug.
Description
•