1010470 - B2G (and Firefox on Android) renders disabled buttons differently from buttons in a disabled fieldset

Status: RESOLVED WORKSFORME

(Core :: Widget, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Load testcase
 2. Compare the two lines of buttons

ACTUAL RESULTS: The two lines look different.
EXPECTED RESULTS: They should look the same.

I get ACTUAL RESULTS in Firefox Simulator for Firefox OS 1.4, and EXPECTED RESULTS in desktop Firefox nightly.

NOTE:
The testcase's first line is <button disabled> & friends; the second line has <button> & friends *inside* of a <fieldset disabled> (which should disable the controls inside of it, per https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset )


The problem is that B2G's CSS for these buttons is all using a [disabled] selector -- i.e. checking for the attribute being set on the actual form control -- when it should instead be conditioned on ":disabled", a pseudoclass that is controlled by nsGenericHTMLFormElement::IsDisabled(), which checks both the attribute and the fieldset (if any).

(I noticed this when following up on a question in bug 1007278 comment 10.)

Comment 1

[Daniel Holbert [:dholbert]]

(Testcase contents shamelessly stolen from arnaud's "disabled-2" reftest added in bug 1007278.  I intend to add a variant of that reftest using a fieldset, to test this bug.)

Comment 2

[Arnaud Bienner]

Nice catch Daniel :)
So that explain why those were different between forms.css and b2g's content.css

Viven, do you have any thoughts about this? (as you did the initial change)

Comment 3

[Arnaud Bienner]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> (I noticed this when following up on a question in bug 1007278 comment 10.)

I believe you meant bug 1009714 comment 10 ;)

Comment 4

[Daniel Holbert [:dholbert]]

Indeed, thanks.

MXR reference to where we actually check the fieldset, when deciding whether to add this pseudoclass:
IsDisabled() impl:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp?rev=263812990c50#2262

...which is checked in nsGenericHTMLFormElement::IntrinsicState(), here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp?rev=263812990c50#2313

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase 1, in Firefox OS 1.4 simulator

Here's a screenshot. Note that the second row does still look disabled, but it's missing some of the native styling that the first row's buttons get.

(The dark "Some text" is bug 1007278; ignore that part.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 7

[Daniel Holbert [:dholbert]]

For reference, http://mxr.mozilla.org/mozilla-central/source/layout/style/forms.css targets these same various form controls using :disabled (textarea, select, various input[type="XYZ"], and button)

Try run with just the reftest part of the patch (expected to be orange, on B2G ICS Emulator):
 https://tbpl.mozilla.org/?tree=Try&rev=883b3b903ab1

Try run with the full patch (reftest + fix):
 https://tbpl.mozilla.org/?tree=Try&rev=a1e97252d0ed

Comment 8

[Daniel Holbert [:dholbert]]

Heh -- the second try run shows that we actually have the same bug on Android, too.

Updated patch coming to fix that.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (now android + B2G)

This makes the same change on Android.

We also have a bunch of ":not([disabled])" in the Android CSS. Instead of converting that to ":not(:disabled)", I went with :enabled, since I believe they're mutually exclusive (as you'd expect), per the last link in comment 4.

Comment 10

[Daniel Holbert [:dholbert]]

Try run with "fix v2" (including Android fix): https://tbpl.mozilla.org/?tree=Try&rev=e05ee0e8146e

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8422756 [details] [diff] [review]
fix v2 (now android + B2G)

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

r=me

::: layout/reftests/forms/button/reftest.list
@@ +20,5 @@
>  
>  # Looks like Android and B2G change the text color, but to something slightly
>  # different from ColorGray
>  fails-if(Android||B2G) == disabled-1.html disabled-1-ref.html
> +== disabled-2a.html disabled-2-ref.html

it may just be splinter, but I don't see a disabled-2a.html file anywhere.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8422756 [details] [diff] [review]
fix v2 (now android + B2G)

(In reply to Fabrice Desré [:fabrice] from comment #11)
> it may just be splinter, but I don't see a disabled-2a.html file anywhere.

Yeah, that's just bugzilla's diff viewer being unhelpful. (I could have made it clearer in my review-request comment, too -- sorry about that).

This patch renames the existing "disabled-2.html" test to "disabled-2a.html", and creates a modified copy "disabled-2b.html", as shown here:

>diff --git a/layout/reftests/forms/button/disabled-2.html b/layout/reftests/forms/button/disabled-2a.html
>rename from layout/reftests/forms/button/disabled-2.html
>rename to layout/reftests/forms/button/disabled-2a.html
...and....
>diff --git a/layout/reftests/forms/button/disabled-2.html b/layout/reftests/forms/button/disabled-2b.html
>copy from layout/reftests/forms/button/disabled-2.html
>copy to layout/reftests/forms/button/disabled-2b.html
>--- a/layout/reftests/forms/button/disabled-2.html
>+++ b/layout/reftests/forms/button/disabled-2b.html
>@@ -1,17 +1,18 @@
> <!DOCTYPE html>
> <html>
[etc etc]


So disabled-2a.html is there.

Thanks for the review!

Comment 13

[Daniel Holbert [:dholbert]]

(Canceling comment 2's needinfo?vingtetun; I think it's pretty clear this was just an oversight in the original forms styling for android, which ended up getting propagated to b2g, as noted in bug 1009714 comment 16 - 17. (of course, vingtetun is free to chime in if I'm missing something; but I don't think this needs to be taking up space in his queue of things-to-dig-into-and-respond-to.)

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/179cea3a84da

Comment 14

[Daniel Holbert [:dholbert]]

[Moving component to "Core|Widget", since I think that's the most coherent place for such an android+b2g-misthemed-form-controls bug to live in.]

Comment 15

[Daniel Holbert [:dholbert]]

Darn -- this caused some R6 and R7 disabled-form-control-related reftest failures (shown in the Try run in comment 7, actually; I think those runs must have completed after the last time I looked at that Try run. :-/ )

Backing out; will investigate further tomorrow.

Comment 16

[Daniel Holbert [:dholbert]]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/db356f641d04

Comment 17

[Daniel Holbert [:dholbert]]

For reference, here's the TBPL run where this landed, showing the test failures that it hit:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=179cea3a84da

Humorously, the Android R4 orange is for us unexpectedly passing a reftest that already exists in the tree which tests this behavior -- 557087-1.html -- which is labeled "skip-if(B2G) fails-if(Android)", and has been ever since we did a "get the trees green" reftest mass-labeling. :-(  I wonder how many other bugs we have on these platforms that our tests would be telling us about if only the tests were enabled...

Comment 18

[Daniel Holbert [:dholbert]]

In most of the test failures, the rendering difference is that there's a slightly different shade of green in the background of the form control. In one case, select-fieldset-4.html, the 1px-wide white vertical bar on the <select> control is a slightly different height. I don't know exactly what's going on, but I'm guessing it might just be flavors of the "native widgets are hard to style reliably on Android and B2G" situation hinted at in bug 1007278 comment 27 & previous comments there.)

I don't have the cycles to follow up on this thoroughly at the moment, given that this isn't a high-priority bug, but I'll hopefully circle back to it before too long.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: screenshot from 2017-07-26 Android Nightly

This seems to be WFM in current Android Nightly -- see attached screenshot of "testcase 1", taken with yesterday's Nightly on my OnePlus One phone (with a LineageOS version based on Android 7.1.2)

Not sure precisely when/why this behavior changed, but a lot of time has passed so I'm not too surprised.