1007278 - Firefox 28 and above do not display HTML disabled buttons with lighter text

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Ben Reilly]

Attachment: DisabledButtonLooksEnabled.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20140421162246

Steps to reproduce:

1. On Windows 7, right click on desktop, then click on Personalize. Go to the "Basic and High Contrast Themes" section, and select the Windows Classic theme.
2. Open Firefox 28 or 29, and browse any webpage which includes a disabled button. For example, go to 

http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_button_disabled

3. Compare how the HTML disabled button looks and how then enabled button looks.


Actual results:

Observe:
There is no difference between how a disabled or enabled button looks: font color and borders look exactly the same (black), giving the erroneous feeling that disabled buttons are actually enabled.


Expected results:

This issue does not occur when having another theme selected. In FF 28 and 29, at least the button borders take a gray color when disabled, and on FF 24 borders AND font color become gray as well. Buttons also look good in IE and Chrome no matter what theme is chosen.

This issue seemingly occurs exclusively with Firefox 28, 29 and Windows Classic and High Contrast themes.

Comment 1

[Alice0775 White]

I can reproduce on Ubuntu12.04 too.
https://hg.mozilla.org/mozilla-central/rev/2a03b34c8953
Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140508030203

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/518f5bff0ae4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131028165857
Bad:
http://hg.mozilla.org/mozilla-central/rev/cd94525c17a4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131029052157
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=518f5bff0ae4&tochange=cd94525c17a4


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4d1dde8f2d2e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131028200357
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/07c532718310
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131028200657
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4d1dde8f2d2e&tochange=07c532718310

Regressed by: Bug 928891

Comment 2

[Alice0775 White]

Also, I can reproduce the problem on Windows7 Aero.

Comment 3

[Arnaud Bienner]

In bug 928891, I split buttons rules in two parts: text vs non-text rules.
Looks like I missed "button:disabled:active, button:disabled," in the last part :(

Comment 4

[Arnaud Bienner]

Attachment: bug1007278.patch

I believe something like this should solve the problem.

Comment 5

[Alice0775 White]

I was surprised that there was no automated test ;(

Comment 6

[Arnaud Bienner]

I will review nothing else was possible broken by bug 928891 and provide a clean patch tomorrow.
I believe my patch is OK but IMO it would be worth to add reftests for this for button, to avoid similar problems in the future.

Comment 7

[Arnaud Bienner]

Actually, I realized by reading comment 1 again that this has been broken since FF 28, so I think it's not so urgent to fix it, if no one complain about this before (i.e. no need to revert for FF 29.0.1, though I guess we should have a fix for FF 30).

Comment 8

[Daniel Holbert [:dholbert]]

Thanks!

Comment 9

[Arnaud Bienner]

Attachment: bug1007278.patch

Cleaner patch, which is working as expected.
Before asking for review, I would like to add some reftests.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Not tracking. See comment #7 for reasons.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this should still be tracking for 30-32 despite comment 7; just because we ship a regression in one release doesn't mean it's not important to fix promptly.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

OK! (I also untracked it because it seems to affect only a few users)

Comment 13

[Kohei Yoshino]

Tweeted https://twitter.com/FxSiteCompat/status/464768636704808960

Comment 14

[Daniel Holbert [:dholbert]]

(Just for clarity, note that we do display these buttons *somewhat* differently from normal buttons (on Linux at least) -- we just don't customize the text-color.  In particular, we're not applying the "color: GrayText;" rule here:
  http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css?rev=bf99640d19e5#667
to disabled buttons, but we should be.)

Comment 15

[Arnaud Bienner]

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC-7) from comment #11)
> I think this should still be tracking for 30-32 despite comment 7; just
> because we ship a regression in one release doesn't mean it's not important
> to fix promptly.

In comment 7, I just suggested we might want to wait for Firefox 30 to release a fix (i.e. not urgent enough for a 29.0.X minor release IMHO).
Agreed we should fix this soon (actually, the fix is ready but just missing some reftests).

Comment 16

[Daniel Holbert [:dholbert]]

One approach to reftesting this could be to simply disable the button's border and background (so you're only comparing the text), and add "color: GrayText;" in the reference case (copypasted from forms.css:

e.g. testcase:
 data:text/html,<button disabled style="background:transparent; border-width: 0">test
reference case:
 data:text/html,<button style="background:transparent; border-width: 0; color: GrayText">test

Comment 17

[Arnaud Bienner]

Attachment: bug1007278.patch

Same patch with the one line of CSS in forms.css fix + reftests added.
- test default layout is applied (a bit different from what was suggested in comment 16, as I don't want to test only text color)
- test different types of buttons all look the same: this was the behavior before the regression, and I believe this will not change. Worth to check it as well IMO.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=55a34b5564ac

Daniel, could you please review the patch? (or delegate to someone else).

Comment 18

[Arnaud Bienner]

Attachment: bug1007278-2.patch

Oops, wrong patch, sorry.

https://tbpl.mozilla.org/?tree=Try&rev=ea9f34a50c7b

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8420592 [details] [diff] [review]
bug1007278-2.patch

># HG changeset patch
># User Arnaud Bienner <[e-mail]>
># Date 1399763979 -7200
>#      Sun May 11 01:19:39 2014 +0200
># Node ID 3a959894a98d4be08b8e942b24f3c5230163ba12
># Parent  9ac3e2dd08982dfa5ac24ef8b293420afa8ba3dd
>Bug 1007278 - fix regression from 928891: 'button:disabled' didn't changed color text anymore

Tweak the commit message to describe the change, rather than the problem.

e.g. "Bug 1007278: Restore the distinct text-color on disabled buttons"


>diff --git a/layout/reftests/forms/button/disabled-1-ref.html b/layout/reftests/forms/button/disabled-1-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/forms/button/disabled-1-ref.html
>@@ -0,0 +1,16 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>Bug 1007278: test buttons "disabled" style</title>

grammar nit: s/buttons/buttons'/  (or perhaps "buttons with")

>diff --git a/layout/reftests/forms/button/disabled-1.html b/layout/reftests/forms/button/disabled-1.html
>+<title>Bug 1007278: test buttons "disabled" style</title>

(same here)

>+++ b/layout/reftests/forms/button/disabled-2-ref.html
>@@ -0,0 +1,20 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<title>Bug 1007278: test all types of buttons look similar when disabled</title>
>+<style>
>+</style>

Drop the empty <style></style> here.

>+  <button>Some text</button>
>+  <input type="button"value="Some text">

Add a space before "value"

>+  <input type="reset">
>+  <input type="submit">


>+  <br>
>+  <button disabled>Some text</button>
>+  <input disabled type="button"value="Some text">

Same here.

>+  <input disabled type="reset">
>+  <input disabled type="submit">
>+</body>
>+</html>
>+

Drop the extra blank line @ the end.

>diff --git a/layout/reftests/forms/button/disabled-2.html b/layout/reftests/forms/button/disabled-2.html
>+<style>
>+</style>

As in the other -2 file, drop the empty <style> block.

>+<body>
>+  <button>Some text</button>
>+  <button>Some text</button>
>+  <button>Reset</button>
>+  <button>Submit Query</button>

IMHO it'd make more sense for this to be the reference case, instead of the testcase, since
 (a) generally, reference cases are supposed to be simpler than testcases
 (b) the idea here seems to be "all of these various things should look like the equivalent <button>"

(I know what I'm suggesting is a bit backwards from the perspective of this exact bug, because <button> was the thing that was broken. But ignoring this exact bug & thinking about what we're asserting here, I think this makes more sense with the testcase/reference swapped.)

If you'd like to keep the test/reference it as-is, though, I won't object.

>+</html>
>+

As above, drop the blank line at the end.

>diff --git a/layout/reftests/forms/button/reftest.list b/layout/reftests/forms/button/reftest.list
>--- a/layout/reftests/forms/button/reftest.list
>+++ b/layout/reftests/forms/button/reftest.list
>@@ -12,8 +12,10 @@ fuzzy-if(B2G||Android,125,20) == percent
> fuzzy-if(B2G||Android,125,80) == percent-width-child-2.html  percent-width-child-2-ref.html
> 
> == vertical-centering.html vertical-centering-ref.html
> 
> != line-height-button-0.5.html line-height-button-1.0.html
> != line-height-button-1.5.html line-height-button-1.0.html
> != line-height-input-0.5.html line-height-input-1.0.html
> != line-height-input-1.5.html line-height-input-1.0.html
>+
>+== disabled-1.html disabled-1-ref.html

Looks like the manifest is missing a line for disabled-2.html.  (Might be worth canceling your try run & re-pushing when that's fixed?)

Thanks!

Comment 20

[Daniel Holbert [:dholbert]]

(Ignore the empty comment below <input type="reset">/<input type="submit"> above; accidentally left that chunk of the patch in my bug comment, with nothing to say about it. :))

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8420592 [details] [diff] [review]
bug1007278-2.patch

r=me with comment 19 addressed.

Comment 22

[Arnaud Bienner]

Attachment: bug1007278-3.patch

Patch updated, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=626a88ebe165 (old one canceled)
Let's see if everything is always green...

Comment 23

[Daniel Holbert [:dholbert]]

Looks like the first reftest has a test failure on Android, because setting "background" isn't sufficient to trigger the non-native button look on that platform. (And in particular, it doesn't give us borders that match the reference case's hardcoded borders.)

From some local testing with my android tablet, I can't figure out a way to force non-native buttons on android. "-moz-appearance:none" doesn't do it; setting background + border together don't do it (there are still curvy edges to the borders, as compared to square edges on my linux box).

So for now, I'd suggest simply setting background *and* border in the testcase, the same way you do in the reference case. So then the only thing you're testing is the text-color, which is a bit more restricted than what you were trying to do, but still better than nothing & an effective regression test for this bug.

Comment 24

[Arnaud Bienner]

What do you think is best?
- testing only text color as you suggested?
- failif(Android) for this test?
- both? (i.e. testing text color everywhere + fail-il(Android) on a more complete test)

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Bienner from comment #24)
> What do you think is best?
> - testing only text color as you suggested?
> - failif(Android) for this test?

I feel like the former is more correct/robust, and shouldn't be too hard.  (I'd prefer to avoid intended-to-last-forever "fails" annotations where possible; they kind of imply the test is asserting something that's not actually guaranteed to be true.)

So: I'd suggest giving the test a Try run with both border and background set in the testcase (per comment 23) -- this doesn't diminish the scope of the test too much, from what it already was testing.  If that *still* fails, then maybe just use fails-if().

> - both? (i.e. testing text color everywhere + fail-il(Android) on a more complete test)

(I think your disabled-2.html already sort of serves the purpose of a more complete test, so I wouldn't worry about adding another more complete test here.)

Thanks!

Comment 26

[Arnaud Bienner]

I set the boder-width to 0 and background: transparent, and pushed to try: still failing for Android, because it seems we don't use "color: GrayText" for inactive button on Android.
Except if there is an easy to make the value used in the reference file different only on Android, I would suggest to add fail-if(Android) for this test.

Would you be OK with this?

https://tbpl.mozilla.org/?tree=Try&rev=cd3c9f953506

Comment 27

[Daniel Holbert [:dholbert]]

Yeah, fails-if (not "fail-if") for Android & B2G on the first test seems fine, perhaps with a comment saying "Widgets on Android and B2G are harder to reliably style" or something.

Per IRC, please file a followup on the B2G failure of disabled-2.html, though, and note that bug number in the manifest alongside the line for disabled-2.html (which will be fails-if(B2G)). That part seems like a real bug which we should fix. (tl;dr: <button disabled> renders with a textfield-flavored background, but <input disabled type="submit"> & other buttony things do not, apparently.)

Comment 28

[Arnaud Bienner]

Attachment: bug1007278-4.patch

Patch updated, with new tests to check disabled buttons non-disabled are rendered differently (those should work on all platforms).

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b335c1e468ac
not completely over yet, but sounds promising :)

Comment 29

[Arnaud Bienner]

Comment on attachment 8422030 [details] [diff] [review]
bug1007278-4.patch

try run looks good (only one reftest failing on Android, but this doesn't look related to what I did).
As I added new reftests, I believe it's safer to ask you for review, just in case.
Sorry I missed to generate an interdiff, but compared to the previous you r+, there are only new reftests_i (with i > 2) + the changes you requested (including the missing space in reftest2.html I missed the other day)

Comment 30

[Daniel Holbert [:dholbert]]

I also noticed that the Try run looks good, so I took the liberty of landing this, after making two tweaks (and I just midaired your review request on posting this :)).

>+++ b/layout/reftests/forms/button/disabled-1-ref.html
>+  button { 
            ^------------.
Fixed end-of-line whitespace

>+!= disabled-3.html disabled-3-ref.html
>+!= disabled-4.html disabled-4-ref.html
>+!= disabled-5.html disabled-5-ref.html
>+!= disabled-6.html disabled-6-ref.html

I renamed all of the -ref files for these "!=" tests to "-notref", to make it clearer that they are *not* actually reference cases -- they're anti-reference cases.

(I ran reftests locally in this directory, to make sure I didn't accidentally forget one of the files or break something.)

With that, landed as:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9594562d60e9

Comment 31

[Daniel Holbert [:dholbert]]

[needinfo=arnaud to request aurora/beta approval, probably after this makes it into a nightly build, though perhaps sooner if you're feeling lucky / anxious. :)]

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/9594562d60e9

Comment 33

[Arnaud Bienner]

Attachment: 9594562d60e9.patch

Patch taken from https://hg.mozilla.org/mozilla-central/raw-rev/9594562d60e9 (version actually landed, slightly different from attachment 8422030 [details] [diff] [review], as explained in comment 30).

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 928891

User impact if declined:
<button disabled>'s text default color remains black (should be gray). On some configurations where other CSS rules for button:disabled cannot be applied, there is no difference between disabled and non-disabled buttons.

Testing completed (on m-c, etc.): 
On m-c. Patch contain many reftests, to be sure the problem is fixed everywhere now.

Risk to taking this patch (and alternatives if risky): 
Very low: only one line of CSS added.

String or IDL/UUID changes made by this patch:
None

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Arnaud, any reason why you are not requesting an uplift to beta?

Comment 35

[Arnaud Bienner]

(In reply to Sylvestre Ledru [:sylvestre] from comment #34)
> Arnaud, any reason why you are not requesting an uplift to beta?

No. Actually I would like to be in beta as well, indeed.
I just thought I had to ask in order (aurora, then beta).
I didn't know it was possible to ask for both at the same time.

Comment 36

[Arnaud Bienner]

Comment on attachment 8423097 [details] [diff] [review]
9594562d60e9.patch

[Approval Request Comment]
See comment 33

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8423097 [details] [diff] [review]
9594562d60e9.patch

No, it is perfectly fine to ask for both at the same time.

In some cases, we do 1) uplift to aurora 2) uplift to beta 
when we want to check at the results in aurora for more coverage (example: in case of risk)
I don't think it is the case here.

Comment 38

[Arnaud Bienner]

(In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> I don't think it is the case here.

I don't think either.

Thanks for the clarification and for the approval :)

I would need someone to push the change to aurora and beta. Not sure who can do this.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

(In reply to Arnaud Bienner from comment #38)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> I would need someone to push the change to aurora and beta. Not sure who can
> do this.
The sheriffs are watching for approval-mozilla-* a few times per day. They are going to merge it very soon I think. (ie, you don't have to do anything)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc4f0d37798a
https://hg.mozilla.org/releases/mozilla-beta/rev/75ff52caee21

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/75ff52caee21

Comment 42

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.7.5 using:
#latest Aurora 31.0a2, build ID: 20140516004003  
#latest Nightly 32.0a1, build ID: 20140516030204
#Fx 30 beta 5, build ID: 20140515140857

Comment 43

[Arnaud Bienner]

So it seems we are done with this bug, that's great :)

Thanks Ben Reilly for testing and reporting the issue: even we do our best to test everything, it's important to have people like you to test and report problems we might miss ;)