Closed Bug 892483 Opened 11 years ago Closed 11 years ago

Zoom controls should not have text labels

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mikedeboer, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [Australis:M8][Australis:P1])

Attachments

(2 files)

Attached image Text labels where they shouldn't be (deleted) —
See title and screenshot. The labels also cause the layout to shift, which doesn't look good.
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.
(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.
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.
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.
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Depends on: 877684
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
Attached patch Patch v1 (deleted) — Splinter Review
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.
Attachment #775634 - Flags: review?(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.
Attachment #775634 - Flags: review?(mconley) → review+
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M8][Australis:P1][fixed-in-ux]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: