985786 - [10.6] The fullscreen button's icon is missing while in fullscreen

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

The button that appears on the tabstrip when in fullscreen mode on 10.6 (with Lion fullscreen) doesn't have an icon but the button is still present if you know to hover in the empty area.

Comment 1

[Matthew N. [:MattN]]

This was caused by bug 875488, specifically http://hg.mozilla.org/projects/ux/diff/1010d94e216b/browser/themes/osx/browser.css#l1.30

Since this button is somewhat special, we should have a test created.

Comment 2

[Matthew N. [:MattN]]

This issue was actually pointed out by Gijs while reviewing that bug (bug 875488 comment 32).

Patch coming soon.

Comment 3

[Matthew N. [:MattN]]

Attachment: v.1 Make the button match the styles of other toolbarbuttons

If you want to test the patch and don't have 10.6, you can redirect to mconley who does have a 10.6 machine.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

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

::: browser/themes/osx/browser.css
@@ -474,5 @@
>  toolbar .toolbarbutton-1:not(:-moz-any([type="menu-button"],[disabled],[open],#back-button,#forward-button)):hover,
>  toolbar .toolbarbutton-1[type="menu-button"]:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button,
>  toolbar .toolbarbutton-1[type="menu-button"]:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-dropmarker,
> -toolbar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined,
> -#restore-button:not([disabled]):hover {

I don't even think @disabled was even getting set on this element anyways.

@@ +909,1 @@
>    #fullscreen-button[cui-areatype="toolbar"] {

Since the button isn't customizable, I didn't bother including the attribute here as it adds noise and confusion IMO.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

redirecting, because I indeed don't have a 10.6 machine :(

Comment 6

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

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

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

This looks fine - just not sure what the value of having the cui-areatype attribute on the element is.

Tested on 10.6 and 10.8.

::: browser/themes/osx/browser.css
@@ +909,1 @@
>    #fullscreen-button[cui-areatype="toolbar"] {

If we're not including the attribute here - why are we including it on the element itself?

Comment 7

[Matthew N. [:MattN]]

(In reply to Mike Conley (:mconley) from comment #6)
> >    #fullscreen-button[cui-areatype="toolbar"] {
> 
> If we're not including the attribute here - why are we including it on the
> element itself?

because some selectors using @primaryToolbarButtons@ (of which this button is included in) are checking that attribute: e.g. https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css?rev=c8809e6c34ab#1238

Comment 8

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

(In reply to Matthew N. [:MattN] from comment #7)
> (In reply to Mike Conley (:mconley) from comment #6)
> > >    #fullscreen-button[cui-areatype="toolbar"] {
> > 
> > If we're not including the attribute here - why are we including it on the
> > element itself?
> 
> because some selectors using @primaryToolbarButtons@ (of which this button
> is included in) are checking that attribute: e.g.
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css?rev=c8809e6c34ab#1238

Gotcha. As you were, then. :)

Comment 9

[Matthew N. [:MattN]]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/e6cc1fb21112

Comment 10

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

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

Comment 11

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #0)
> The button that appears on the tabstrip when in fullscreen mode on 10.6
> (with Lion fullscreen) doesn't have an icon but the button is still present
> if you know to hover in the empty area.

Just for my curiosity... 10.6 is Snow Leopard. Did you mean s/Lion// ? If not, I am confused. :-)

But really, the reason I'm commenting is uplift. Do you think this needs bake time on m-a before we go to m-b or not?

Comment 12

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Matthew N. [:MattN] from comment #0)
> > The button that appears on the tabstrip when in fullscreen mode on 10.6
> > (with Lion fullscreen) doesn't have an icon but the button is still present
> > if you know to hover in the empty area.
> 
> Just for my curiosity... 10.6 is Snow Leopard. Did you mean s/Lion// ? If
> not, I am confused. :-)

I meant "without", good catch.

> But really, the reason I'm commenting is uplift. Do you think this needs
> bake time on m-a before we go to m-b or not?

I think it's fine for Aurora now as I haven't seen any fallout yet and this is only 10.6 where the bug went undetected since last June so it likely doesn't get much testing.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 875488
User impact if declined: Fullscreen icon to exit fullscreen will be missing on OS X 10.6.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Fairly low risk trying to make this button more consistent with the styling of other buttons so it doesn't get forgotten about again.
String or IDL/UUID changes made by this patch: None

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

Are you going to nominate this for beta as well?

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8394029 [details] [diff] [review]
v.1 Make the button match the styles of other toolbarbuttons

[Approval Request Comment]
See comment 13

Comment 16

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b92b19781ae3

Comment 17

[Matthew N. [:MattN]]

Thanks!
https://hg.mozilla.org/releases/mozilla-beta/rev/fe96465ab232

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

The icon is now displayed after entering fullscreen mode on:
- latest Nightly (build ID: 20140327030203)
- latest Aurora (build ID: 20140328004001)
- Firefox 29 beta 3 (build ID: 20140327113732)

User Agents:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:31.0) Gecko/20100101 Firefox/31.0