892483 - Zoom controls should not have text labels

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike de Boer [:mikedeboer]]

Attachment: Text labels where they shouldn't be

See title and screenshot.

The labels also cause the layout to shift, which doesn't look good.

Comment 1

[:Gijs (he/him)]

When did this regress? Also, is this really P1? I think OS X, with the window bordering on the edge of the screen, is the only situation where the labels get cut off. At least on Windows, they looked fine when I checked yesterday... I was under the impression the labels had always been there.

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #1)
> When did this regress? Also, is this really P1? I think OS X, with the
> window bordering on the edge of the screen, is the only situation where the
> labels get cut off. At least on Windows, they looked fine when I checked
> yesterday... I was under the impression the labels had always been there.

This is locale-dependent anyway. Even if they were visible on all OSes for en-US, that wouldn't really mean anything for other locales.

Comment 3

[Mike de Boer [:mikedeboer]]

Per the specs (see the live demo available at the URL), there is no label for the +/- button visible at all times, independent of the locale. Also in the older panel UI mockups, there were no labels to be seen there.

The labels were never supposed to be there and weren't there in the initial implementation (bug 870897). Somehow they got introduced and I therefore marked it a regression.

Comment 4

[:Gijs (he/him)]

Good: http://hg.mozilla.org/projects/ux/rev/34c0f40827de (July 9th Mac nightly)
Bad: http://hg.mozilla.org/projects/ux/rev/727132bea25c (July 11th Windows nightly)

(no nightly on the 10th that I can see)

https://hg.mozilla.org/projects/ux/pushloghtml?tochange=727132bea25c&fromchange=34c0f40827de

Will try to narrow this down more. Still unsure this really needs to be P1.

Comment 5

[:Gijs (he/him)]

Locally backed out https://hg.mozilla.org/projects/ux/rev/a39e19167f5e (bug 877684), which fixes this. Still looking where I went wrong, but taking anyway.

Comment 6

[Mike de Boer [:mikedeboer]]

Gijs, thanks for narrowing that down! I was able to indeed pinpoint https://hg.mozilla.org/projects/ux/rev/a39e19167f5e - bug 877684 as the offending changeset.

More specifically, the changes in browser/themes/osx/customizableui/panelUIOverlay.css - https://hg.mozilla.org/projects/ux/rev/a39e19167f5e#l5.19

Comment 7

[:Gijs (he/him)]

Attachment: Patch v1

I was looking at this, but -moz-box is the default, so I don't really understand why we need to add this here. Getting rid of it doesn't seem to break anything on Windows, at least... Mike?

PS: I also think that these buttons should have an aria-label instead of a label if we're not actually wanting to ever show the text, but that's a separate bug I guess... because of our setAttributes magic that's not as obvious to fix.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 775634 [details] [diff] [review]
Patch v1

Review of attachment 775634 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +50,5 @@
>  
>  #PanelUI-contents > toolbarpaletteitem > toolbaritem > toolbarbutton > .toolbarbutton-text,
>  #PanelUI-contents > toolbaritem > toolbarbutton > .toolbarbutton-text,
>  #PanelUI-contents > toolbarpaletteitem > toolbarbutton > .toolbarbutton-text,
>  #PanelUI-contents > toolbarbutton > .toolbarbutton-text {

Yeah, that -moz-box didn't seem necessary.

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/dbb311c24804

Comment 10

[:Gijs (he/him)]

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