Closed Bug 1371219 Opened 7 years ago Closed 7 years ago

Increase size of main menu items when accessed through touch

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.3 - Jul 24
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

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

Details

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

Attachments

(3 files)

Bug 1371219 - Add an inputSource attribute to XULCommandEvent.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1371219 - Increase margin of main menu items when accessed through touch.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1371219 - Add a test for touchmode on toolbar menus.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Flags: qe-verify+
Priority: -- → P2
QA Contact: brindusa.tot
Whiteboard: [photon-visual][triage] → [photon-visual]
Whiteboard: [photon-visual] → [photon-visual][p2]
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
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 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 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 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+
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
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 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 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 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 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-
Just saw that I missed a function call in nsMenuUtilsX.mm, I'll patch that up.
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+
Blocks: 1383003
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
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)
(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)
(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+
Depends on: 1402727
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?
sorry, typo. I meant, the items in the menu are not spaced out when accessed via touchscreen (long press)
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.

Attachment

General

Creator:
Created:
Updated:
Size: