1398531 - [Photon] Zoom indicator in hamburger menu looks bulky

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Franck (Wip)]

To my eyes, the zoom indicator in the hamburger menu feels a little bulky and eye-catching.

It is smaller and brighter on the mock-up:
https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/230516723

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

It's probably so high, due to button size standardization.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

https://reviewboard.mozilla.org/r/181766/#review187098

I mean, I kind of like the current state, but OK!

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

https://reviewboard.mozilla.org/r/181766/#review187116

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81457c208695
Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment. r=Gijs

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81457c208695

Comment 7

[Valentina Claudia Ona]

Attachment: zoom indicator.bmp

Hi Mike, 
I tested this issue and the actual results are in the attachment
Tell me it is the expected behavior?

[Note]: I marked the differences with red.

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Valentina Claudia Ona from comment #7)
> Created attachment 8911128 [details]
> zoom indicator.bmp
> 
> Hi Mike, 
> I tested this issue and the actual results are in the attachment
> Tell me it is the expected behavior?
> 
> [Note]: I marked the differences with red.

The change you noticed in there is the _width_ of the button, whereas this bug changed the _height_ - you can measure that change in the same picture too.
The width change was bug 1379357.

Comment 9

[Valentina Claudia Ona]

Attachment: zoom indicator (2).bmp

I retested this issue and I can't see differences between the old height (Nightly 57.0a1 from 2017-09-06) and the new height (Latest Nightly 58.0a1).
So please tell me which are the specifications?
If I compare with the specifications we have 24 px.
https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/230516723

Comment 10

[Mike de Boer [:mikedeboer]]

Ah, yes, so this is OSX only, then!

Comment 11

[Franck (Wip)]

Checked on Nightly 58.0a1 20170922100051 on Windows 7, there is no difference whatsoever before and after the patch.

Although the specifications/mock-up do not give any explicit height for the zoom button, I measured one of 16px.

So, the patch has landed but the issue is obviously not fixed.
Can we reopen this bug or should I file a new one ?

Comment 12

[:Gijs (he/him)]

Ni for comment #11. :-)

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

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

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

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

Comment 15

[:Gijs (he/him)]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

https://reviewboard.mozilla.org/r/181766/#review188402

::: browser/themes/shared/customizableui/panelUI.inc.css:1374
(Diff revision 3)
>    margin-inline-start: 10px;
>  }
>  
>  /* An em-based minimum height works better for a text-only button. */
>  #appMenu-zoomReset-button {
> -  min-height: 2em;
> +  min-height: calc(1em + 8px);

What happens if we just use unset/auto?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #15)
> What happens if we just use unset/auto?

Brilliant! 'unset' does the job just fine as well and is much cleaner. Nice.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181766/diff/3-4/

Comment 18

[:Gijs (he/him)]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

https://reviewboard.mozilla.org/r/181766/#review188414

Great, rs=me!

Comment 19

[Mike de Boer [:mikedeboer]]

[Tracking Requested - why for this release]: Small look & feel change, trivial to uplift.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7eb30eea1779
Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment. r=Gijs

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7eb30eea1779

Comment 22

[:Gijs (he/him)]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

Approval Request Comment
[Feature/Bug causing the regression]: photon stuff
[User impact if declined]: zoom "pill" button in the hamburger looks a bit oversized
[Is this code covered by automated tests?]: nope, styling only
[Has the fix been verified in Nightly?]: not yet, but should be trivial
[Needs manual test from QE? If yes, steps to reproduce]: not really... it's a 1-line CSS change
[List of other uplifts needed for the feature/fix]: none - we just need the min-height to be "unset" here, instead of whatever it is on 57 (we landed 2 patches in this bug, maybe there be conflicts trying to graft this as-is, I'm not sure - but it's a 1-line change!)
[Is the change risky?]: no
[Why is the change risky/not risky?]: css-only 1-liner change, loads of beta to go.
[String changes made/needed]: nope

Comment 23

[Franck (Wip)]

You're gonna hate me... but FYI the width change introduced in the latest patch lightly regressed what bug 1379357 fixed.

At least on Windows 7, witch uses Segoe UI font family, it is 2 pixels short, making for a very subtle 'jump' when switching back and forth 90% and 100%.

This rules fixes it for me:  min-width: calc(3ch + 10px);

Comment 24

[Valentina Claudia Ona]

Attachment: zoom indicator (3).bmp

I verified this issue with latest Nightly 58.0a1 and now the zoom indicator is smaller than the "-" and "=" button, but still, in the specifications, it seems to be a bit smaller.
Shorlander, what is your opinion in regards to this?

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910271 [details]
Bug 1398531 - Toolbarbuttons are standardized with a min-height of 24px, but the zoom percentage button is text-only, thus needs a different treatment.

Polish photon, taking it
Should be in 57b4 (gtb tomorrow Thursday)

Comment 26

[:Gijs (he/him)]

Attachment: Patch for beta

Beta patch considering the concerns about whether this would apply and comment 23, this only adjusts the height, not the width. Please uplift this instead of the original patch, per discussion with Mike on IRC.

Comment 27

[:Gijs (he/him)]

(In reply to Franck (Wip) from comment #23)
> You're gonna hate me... but FYI the width change introduced in the latest
> patch lightly regressed what bug 1379357 fixed.
> 
> At least on Windows 7, witch uses Segoe UI font family, it is 2 pixels
> short, making for a very subtle 'jump' when switching back and forth 90% and
> 100%.
> 
> This rules fixes it for me:  min-width: calc(3ch + 10px);

Can you file a separate bug for this, please?

Comment 28

[Franck (Wip)]

Done.
It is bug 1403675.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/555fadd8b835

Comment 30

[:Gijs (he/him)]

(In reply to Franck (Wip) from comment #28)
> Done.
> It is bug 1403675.

Thanks!

Comment 31

[Stephen Horlander [:shorlander] (If you're waiting on a response please ping me on Slack or Riot)]

(In reply to Valentina Claudia Ona from comment #24)
> Created attachment 8912599 [details]
> zoom indicator (3).bmp
> 
> I verified this issue with latest Nightly 58.0a1 and now the zoom indicator
> is smaller than the "-" and "=" button, but still, in the specifications, it
> seems to be a bit smaller.
> Shorlander, what is your opinion in regards to this?

Looks good to me. Bryan worked on that, getting his feedback.

Comment 32

[bbell]

Looks good to me in Nightly 58.0a1 - Windows and Mac.

Comment 33

[Valentina Claudia Ona]

What do you think about this issue? Should I mark this as verified fixed?

Comment 34

[Valentina Claudia Ona]

Based on the fact that in comment 33 and comment 32 this issue is fixed, I will mark this as verified fixed.

Comment 35

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

I also verified this bug on Beta 57.0b9 (20171016185129) under Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64. Besides the issue mentioned in comment 31, I haven't seen any new ones during testing, so I will mark this as verified fixed.