1468249 - Toggle buttons are too tall when using touch

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Martin Thomson [:mt:]]

Attachment: touched.jpg

STR:

Open the hamburger menu by touching it.
The toggle for TP is very tall.

Expected:

It should probably not get taller when using touch, but be centered instead.

Comment 1

[Johann Hofmann [:johannh]]

Hah, good point. Thanks for filing this.

Comment 2

[Mike Taylor [:miketaylr]]

Is this the right component? (maybe it doesn't matter)?

Comment 3

[Johann Hofmann [:johannh]]

Probably rather menus, I guess. I can take this.

Comment 4

[Mike Taylor [:miketaylr]]

Let's mark as fix-optional for 62, but it would be cool to land a patch in 62.

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

Review commit: https://reviewboard.mozilla.org/r/253564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253564/

Comment 6

[Johann Hofmann [:johannh]]

This patch fixes the visual issue, though there's still bug 1468311 which is slightly worse with touch input, IMO.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8988299 [details]
Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

https://reviewboard.mozilla.org/r/253564/#review260324

::: browser/themes/shared/customizableui/panelUI.inc.css:617
(Diff revision 1)
>  }
>  
> +/* Don't resize the toggle in touch mode. */
> +#appMenu-popup[touchmode] #appMenu-tp-toggle {
> +  margin-top: 12px;
> +  margin-bottom: 12px;

Can we add some kind of invisible padding within the button so that it's usable in touch mode without looking ridiculous?

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8988299 [details]
Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253564/diff/1-2/

Comment 9

[Johann Hofmann [:johannh]]

This is the best I can do, I think. This fixes bug 1468311 in the same commit, the button is still the same size, but at least the user doesn't accidentally close the main menu by tapping outside anymore.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8988299 [details]
Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

https://reviewboard.mozilla.org/r/253564/#review266310

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cacad1697a69
Avoid resizing the TP toggle in the main menu in touch mode. r=dao

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/cacad1697a69

Comment 13

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed, I tested using Windows Surface Pro, on Fx 63.0a1 (2018-07-26), both x86, x64.

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8988299 [details]
Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1462468
[User impact if declined]: Ugly tracking protection toggle when opening the main menu via touch
[Is this code covered by automated tests?]: No, it's CSS-only
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a very short and straightforward front-end patch that has baked in Nightly for a while now.
[String changes made/needed]: None

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8988299 [details]
Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode.

CSS only, regression fix, Beta62+

Comment 16

[Cosmin Sabou [:CosminS]]

This failed to graft for beta. I got:

grafting 490876:cacad1697a69 "Bug 1468249 - Avoid resizing the TP toggle in the main menu in touch mode. r=dao"
merging browser/components/customizableui/content/panelUI.inc.xul
warning: conflicts while merging browser/components/customizableui/content/panelUI.inc.xul! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Johann, could you please take a look at this? Thank you.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The conflicts is from bug 1472535.

Comment 18

[Johann Hofmann [:johannh]]

Attachment: Patch for Beta

Updated the patch to work in beta, thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/de436be5e56e

Comment 20

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed on Fx 62.0b15. I verified using Windows Surface Pro.