973704 - Australis: disabled="false" buttons don't get appropriate hover/active styles

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0 (Beta/Release)
Build ID: 20131022185104

Steps to reproduce:

Create a toolbaritem containing a toolbarbutton and customise it to the nav-bar.
Hover over button.
Press button.


Actual results:

Nothing.


Expected results:

The button should show hover styling when the mouse is over it (including in the overflow panel) and active styling when it is pressed.

Comment 1

[:Gijs (he/him)]

(In reply to Ian Nartowicz from comment #0)
> User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20100101
> Firefox/17.0 (Beta/Release)
> Build ID: 20131022185104
> 
> Steps to reproduce:
> 
> Create a toolbaritem containing a toolbarbutton and customise it to the
> nav-bar.
> Hover over button.
> Press button.

Did you add the toolbarbutton-1 class to the toolbarbutton?

Comment 2

[:Gijs (he/him)]

Also, you marked this bug as "Australis" but also reported the version you're using as 17. Can you clarify?

Comment 3

[Ian Nartowicz]

Yes, tested on 29, reported on 17ESR.  The toolbarbutton has class toolbarbutton-1 as well as others.  It is a button from a longstanding addon called Torrent Status.

Comment 4

[Ian Nartowicz]

Found it!  I had assumed that being inside a toolbaritem was the problem, but not so.  It is caused by an attribute disabled="false".  Works OK in 28, not in 29.

Comment 5

[:Gijs (he/him)]

(In reply to Ian Nartowicz from comment #4)
> Found it!  I had assumed that being inside a toolbaritem was the problem,
> but not so.  It is caused by an attribute disabled="false".  Works OK in 28,
> not in 29.

Updating some fields, please doublecheck that I've understood correctly. :-)

Comment 6

[Alexander Dietrich]

I wonder if I'm observing the same bug:

Similar button setup, but the button is "disabled = true" after adding it to the nav-bar with addWidgetToArea(). Button is NOT disabled properly until Firefox is restarted.

Comment 7

[Ian Nartowicz]

I think what I reported is Linux-only.  It is down to the assumption that a disabled widget does not have a disabled attribute, so any disabled attribute with any value (even "false") may get treated as a disabled widget.  Linux didn't used to do this, but in Australis the Firefox CSS has been used to override the OS theming.

Sounds like you're seeing something else.  Does the button actually have the expected attribute when you've added it?  Or does it not get the attribute until after the restart?

Comment 8

[Alexander Dietrich]

I'm on Linux as well, the attribute is not set until I restart Firefox.

The "disabled" property is also "undefined" immediately after the button is added.

Comment 9

[:Gijs (he/him)]

(In reply to Alexander Dietrich from comment #8)
> I'm on Linux as well, the attribute is not set until I restart Firefox.
> 
> The "disabled" property is also "undefined" immediately after the button is
> added.

This is a separate bug. Please file it with more detailed steps to reproduce.

Comment 10

[:Gijs (he/him)]

Attachment: only style disabled='true' as disabled, not disabled='false', after Australis restyle,

Search/replace to the rescue... Note that I've only updated the generic toolbarbutton styles, not the ones for specific buttons/menuitems, because there we control the state and this shouldn't be an issue (and I don't think those styles were changed for Australis)

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8383128 [details] [diff] [review]
only style disabled='true' as disabled, not disabled='false', after Australis restyle,

We also do things like :not([disabled]) on Windows, so I'd rather not let these get out of sync.

Comment 12

[:Gijs (he/him)]

Attachment: only style disabled='true' as disabled, not disabled='false', after Australis restyle,

Now with windows and OS X goodness. Still only through corrective use of search/replace.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8384702 [details] [diff] [review]
only style disabled='true' as disabled, not disabled='false', after Australis restyle,

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

r=me (don't forget to update the commit message to say r=jaws)

Comment 14

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/87606dd9c0f8

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/87606dd9c0f8

Comment 16

[:Gijs (he/him)]

Comment on attachment 8384702 [details] [diff] [review]
only style disabled='true' as disabled, not disabled='false', after Australis restyle,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: non-disabled buttons will look/behave disabled
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): low, CSS-only fix
String or IDL/UUID changes made by this patch: none

Comment 17

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/245a462c36b6