Increase size of main menu items when accessed through touch

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
VERIFIED FIXED
2 months ago
15 hours ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox56 fixed, firefox57 verified)

Details

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

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

2 months ago
See https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/233342335

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]

Updated

2 months ago
Whiteboard: [photon-visual] → [photon-visual][p2]

Updated

2 months ago
Duplicate of this bug: 1373441
(Assignee)

Updated

2 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

29 days 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

29 days 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

29 days ago
Blocks: 1383003

Comment 34

29 days 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

28 days 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: 28 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

27 days ago
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

Comment 36

17 hours ago
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

17 hours 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)

Comment 38

17 hours ago
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

17 hours 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)

Comment 40

15 hours ago
Thanks :)

I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.