Increase size of main menu items when accessed through touch

VERIFIED FIXED in Firefox 56

Status

()

P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

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

Attachments

(3 attachments)

Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]
Whiteboard: [photon-visual] → [photon-visual][p2]
Duplicate of this bug: 1373441
(Assignee)

Updated

2 years ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
The patch includes all toolbar menu panels that are part of (what I perceive to be) the main Photon menu structure. The hamburger menu, the page action meatball menu and the library menu and its more specific cousins (such as the history or developer menu).

Comment 6

2 years ago
mozreview-review
Comment on attachment 8887086 [details]
Bug 1371219 - Add an inputSource attribute to XULCommandEvent.

https://reviewboard.mozilla.org/r/157820/#review163146

Need to be consistent and update all the places where command event is dispatched to pass input source, not just xul button frame.
Attachment #8887086 - Flags: review?(bugs) → review-

Comment 7

2 years ago
mozreview-review
Comment on attachment 8887087 [details]
Bug 1371219 - Increase margin of main menu items when accessed through touch.

https://reviewboard.mozilla.org/r/157822/#review163536

Thanks Johan, this is getting close. I'd like to talk about a few things below that don't sit quite right with me. Curious for your thoughts!

::: browser/components/customizableui/PanelMultiView.jsm:929
(Diff revision 1)
> -          this._mainViewHeight = 0;
>          }
> +
> +        // Clear the main view size caches. The dimensions could be different
> +        // when the popup is opened again, e.g. through touch mode sizing.
> +        this._mainViewHeight = 0;

Can you move all these inside the `if (this.panelViews) {` conditional above?

::: browser/components/customizableui/content/panelUI.js:288
(Diff revision 1)
>          this.panel.addEventListener("popupshown", function() {
>            resolve();
>          }, {once: true});
>  
>          anchor = this._getPanelAnchor(anchor);
> -        this.panel.openPopup(anchor);
> +        this.panel.openPopup(anchor, undefined, undefined, undefined,

I think you agree with me that this looks very awkward and non-Javascript-y. Is there no other way to do this?

::: browser/components/customizableui/content/panelUI.js:485
(Diff revision 1)
>     */
> -  async showSubView(aViewId, aAnchor, aPlacementArea) {
> +  async showSubView(aViewId, aAnchor, aPlacementArea, aEvent) {
> +
> +    let domEvent = null;
> +    if (aEvent) {
> +      if (aEvent.type == "command" && aEvent.inputSource) {

Just checking... you're specifically checking for inputSource != 0, right?

::: browser/components/customizableui/content/panelUI.js:487
(Diff revision 1)
> +
> +    let domEvent = null;
> +    if (aEvent) {
> +      if (aEvent.type == "command" && aEvent.inputSource) {
> +        // Synthesize a new DOM mouse event to pass on the inputSource.
> +        domEvent = document.createEvent("MouseEvent");

I wish you could simply use:

```js
domEvent = new MouseEvent("click", {
  bubbles: true,
  cancelable: true,
  screenX: aEvent.screenX,
  screenY: aEvent.screenY,
  relatedTarget: aEvent.target,
  inputSource: aEvent.inputSource
});
```
... because this looks wonky. Perhaps something worth to 'just do'?

::: browser/components/customizableui/content/panelUI.js:602
(Diff revision 1)
>  
>        if (aAnchor != anchor && aAnchor.id) {
>          anchor.setAttribute("consumeanchor", aAnchor.id);
>        }
>  
> -      tempPanel.openPopup(anchor, "bottomcenter topright");
> +      tempPanel.openPopup(anchor, "bottomcenter topright", undefined,

I don't think this is something we want to live with. `openPopup` needs to start accepting a dictionary with options.

::: browser/themes/shared/customizableui/panelUI.inc.css:361
(Diff revision 1)
>    max-width: 30em;
>  }
>  
> +#appMenu-popup[touchmode] panelview,
> +#customizationui-widget-multiview panelview:not([extension]) {
> +  min-width: calc(@menuPanelWidth@ + 24px);

Can you please add a comment where you get the 24px from?
Attachment #8887087 - Flags: review?(mdeboer) → review-

Comment 8

2 years ago
mozreview-review
Comment on attachment 8887088 [details]
Bug 1371219 - Add a test for touchmode on toolbar menus.

https://reviewboard.mozilla.org/r/157824/#review163844

LGTM!

::: browser/base/content/test/touch/browser_menu_touch.js:100
(Diff revision 1)
> +
> +  CustomizableUI.addWidgetToArea("library-button", CustomizableUI.AREA_NAVBAR);
> +  CustomizableUI.addWidgetToArea("history-panelmenu", CustomizableUI.AREA_NAVBAR);
> +
> +  await BrowserTestUtils.waitForCondition(() =>
> +      CustomizableUI.getPlacementOfWidget("library-button").area == "nav-bar");

nit: this indentation looks a bit off. Can you fix it?
Attachment #8887088 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8887087 [details]
Bug 1371219 - Increase margin of main menu items when accessed through touch.

https://reviewboard.mozilla.org/r/157822/#review163536

> I think you agree with me that this looks very awkward and non-Javascript-y. Is there no other way to do this?

There is, we could change openPopup to accept an options object, but it's really not a change I want to perform in this bug. There are two related functions that we should patch up at that opportunity as well and there are lots of consumers that use openPopup passing default parameters instead of undefined, if you think that looks more JavaScript-y I'm happy to change it ;)

https://searchfox.org/mozilla-central/search?q=symbol:%23openPopup&redirect=false
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8887087 [details]
Bug 1371219 - Increase margin of main menu items when accessed through touch.

https://reviewboard.mozilla.org/r/157822/#review163536

> Just checking... you're specifically checking for inputSource != 0, right?

In theory it doesn't really matter since 0 means SOURCE_UNKNOWN, but it may be more correct to check inputSource != null.

> I wish you could simply use:
> 
> ```js
> domEvent = new MouseEvent("click", {
>   bubbles: true,
>   cancelable: true,
>   screenX: aEvent.screenX,
>   screenY: aEvent.screenY,
>   relatedTarget: aEvent.target,
>   inputSource: aEvent.inputSource
> });
> ```
> ... because this looks wonky. Perhaps something worth to 'just do'?

I agree, but yeah, we should make a DOM bug about this.

> Can you please add a comment where you get the 24px from?

Good point, thanks! I must have forgotten that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8887087 [details]
Bug 1371219 - Increase margin of main menu items when accessed through touch.

https://reviewboard.mozilla.org/r/157822/#review164160

::: toolkit/content/widgets/popup.xml:60
(Diff revision 2)
> +            aX = params.x;
> +            aY = params.y;
> +            aIsContextMenu = params.isContextMenu;
> +            aAttributesOverride = params.attributesOverride;
> +            aTriggerEvent = params.triggerEvent;
> +          }

\o/ Thanks, this looks very nice ;-) Was it worth it?
Attachment #8887087 - Flags: review?(mdeboer) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8887086 [details]
Bug 1371219 - Add an inputSource attribute to XULCommandEvent.

https://reviewboard.mozilla.org/r/157820/#review164162

http://searchfox.org/mozilla-central/search?q=DispatchXULCommand&path= certainly shows cases where we know the source, but which aren't handled in this patch.

::: dom/interfaces/xul/nsIDOMXULCommandEvent.idl:27
(Diff revision 2)
>    readonly attribute boolean shiftKey;
>    readonly attribute boolean altKey;
>    readonly attribute boolean metaKey;
>  
>    /**
> +   * The input source, if this event was triggered by a mouse event.

can be triggered by something else too, at least in theory, so rephrase a bit
Attachment #8887086 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8887086 [details]
Bug 1371219 - Add an inputSource attribute to XULCommandEvent.

https://reviewboard.mozilla.org/r/157820/#review164162

Thanks, I forgot about those. I skipped this part: http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/layout/xul/nsXULPopupManager.cpp#2884, because I'm not sure if it's worth adding support to popup manager (which is a little more complex than just getting the flag from an event) since it's not going to be used for now. Let me know if you want me to change that as well.

> can be triggered by something else too, at least in theory, so rephrase a bit

I don't really understand, if there's an inputSource there needs to be a mouse event behind it, right? Is you concern about the word "triggered"?

Comment 20

2 years ago
mozreview-review
Comment on attachment 8887086 [details]
Bug 1371219 - Add an inputSource attribute to XULCommandEvent.

https://reviewboard.mozilla.org/r/157820/#review164320

::: dom/xul/nsXULElement.cpp:1307
(Diff revision 3)
>          }
>          WidgetInputEvent* orig = aVisitor.mEvent->AsInputEvent();
> +
> +        uint16_t inputSource = nsIDOMMouseEvent::MOZ_SOURCE_UNKNOWN;
> +        WidgetMouseEventBase* mouseEvent = orig->AsMouseEventBase();
> +        if (mouseEvent) {

As far as I see, we have never mouseEvent here. original event is always the original command event.
You could take inputSource from that.
Attachment #8887086 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
Just saw that I missed a function call in nsMenuUtilsX.mm, I'll patch that up.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

2 years ago
mozreview-review
Comment on attachment 8887086 [details]
Bug 1371219 - Add an inputSource attribute to XULCommandEvent.

https://reviewboard.mozilla.org/r/157820/#review164726
Attachment #8887086 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1383003

Comment 34

2 years ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c98845cb6d5
Add an inputSource attribute to XULCommandEvent. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ee312146ee4e
Increase margin of main menu items when accessed through touch. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/cd19f7fa5160
Add a test for touchmode on toolbar menus. r=mikedeboer

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c98845cb6d5
https://hg.mozilla.org/mozilla-central/rev/ee312146ee4e
https://hg.mozilla.org/mozilla-central/rev/cd19f7fa5160
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Summary: Increase margin of main menu items when accessed through touch → Increase size of main menu items when accessed through touch
QA Contact: brindusa.tot → ovidiu.boca
I tested this on Surface Pro2 with Windows 10 with the latest Nightly 57.0a1(2017-08-17) and I can confirm the fix. 

Johann the increase of the menu size should be visible if I enabled tablet mode on Window 10, or only on devices with a touch screen? I'm asking you this because I enabled the tablet mode on Windows 10 and the menu size is the same. Thanks
Flags: needinfo?(jhofmann)
(Assignee)

Comment 37

2 years ago
(In reply to ovidiu boca[:Ovidiu] from comment #36)
> I tested this on Surface Pro2 with Windows 10 with the latest Nightly
> 57.0a1(2017-08-17) and I can confirm the fix. 
> 
> Johann the increase of the menu size should be visible if I enabled tablet
> mode on Window 10, or only on devices with a touch screen? I'm asking you
> this because I enabled the tablet mode on Windows 10 and the menu size is
> the same. Thanks

Only if you actually touch the element that opens a menu on a touchscreen.
Flags: needinfo?(jhofmann)
I tested this on Windows 8.1 with touch screen and the menu doesn't have the size from the specification. On Surface Pro2 with Windows 10, everything works as expected.
Flags: needinfo?(jhofmann)
(Assignee)

Comment 39

2 years ago
(In reply to ovidiu boca[:Ovidiu] from comment #38)
> I tested this on Windows 8.1 with touch screen and the menu doesn't have the
> size from the specification. On Surface Pro2 with Windows 10, everything
> works as expected.

That's expected. Bug 1383003. :)
Flags: needinfo?(jhofmann)
Thanks :)

I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+

Updated

2 years ago
Depends on: 1402727

Comment 41

a year ago
using v57 stable and when in Library (aka bookmarks manager/history manager) when I long press on items, they are not spaced out. Is this filed somewhere else or did this just fall through the cracks?

Comment 42

a year ago
sorry, typo. I meant, the items in the menu are not spaced out when accessed via touchscreen (long press)
You need to log in before you can comment on or make changes to this bug.