1009714 - <button> looks different from other buttons when disabled on B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Arnaud Bienner]

As discussed in bug 1007278 comment 27, <button> doesn't look like other buttons (e.g. <input type="button">, <input type="reset">, etc.) when disabled, while these other buttons look similar though.

I don't think it's an intended behavior, so opening this follow up bug to investigate this.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reftest snapshot, demonstrating issue

Here's a reftest snapshot from one of bug 1007278's Try runs, demonstrating this issue.

The second line of buttons are all disabled. The first is <button>, and the others are button-flavored <inputs>. See https://bugzilla.mozilla.org/attachment.cgi?id=8420596&action=diff#a/layout/reftests/forms/button/disabled-2.html_sec2 for the exact source.

As you can see, the first one (the <button>) looks like it has a textfield-flavored background, while the others look more button-like.

Comment 2

[Daniel Holbert [:dholbert]]

The <button> appears to be sized differently when it's disabled, too -- everything after it on the second line is offset a bit to the left, as compared to the first line.

I'll bet authors assume that enabling/disabling a button won't affect their layout, but it apparently  can on B2G right now.

I suspect this might be all tied up in the same underlying issue; not sure though.

Comment 3

[Daniel Holbert [:dholbert]]

Simple data URL testcase: data:text/html,<button%20disabled>button</button><input%20type="button"%20disabled%20value="input">

I can reproduce this in the Firefox OS 1.4 Simulator, from
 https://ftp.mozilla.org/pub/mozilla.org/labs/fxos-simulator/

Comment 4

[Arnaud Bienner]

Haven't tested, but this might give a clue: is this valid?
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/content.css#262
i.e. button[disabled="true"] instead of simply button[disabled] like for lines above.

At least, in the style editor it doesn't work.

Comment 5

[Daniel Holbert [:dholbert]]

Yup, that's exactly it.

If I modify my data URI to use disabled="true" on the <button>...
data:text/html,<button%20disabled="true">button</button><input%20type="button"%20disabled%20value="input">

...it renders like the <input> and does actually look like a disabled button instead of a textfield.

So, we should drop the ="true" from the line of source that you linked to.

Comment 6

[Daniel Holbert [:dholbert]]

This dates back to this cset:
 http://hg.mozilla.org/mozilla-central/diff/66f381b9547e/b2g/chrome/content/content.css#l1.191
for bug 782973.

Comment 7

[Arnaud Bienner]

Attachment: bug1009714.patch

Patch that probably fixes the issue (will test it fully later)

Comment 8

[Arnaud Bienner]

Comment on attachment 8422023 [details] [diff] [review]
bug1009714.patch

I pushed to try and everything looks OK (I should probably have restrict to B2G only, sorry :(
Only R1 is failing but it is busted, and R8 is green now (R8 was failing without the fails-if in bug 1007278, with this run: https://tbpl.mozilla.org/?tree=Try&rev=cd3c9f953506)

Vivien, Fabrice, requesting your feedback just to be sure this was just an honest mistake, and that we didn't missed anything here (i.e. I will break something, again).

Comment 9

[Arnaud Bienner]

Forgot to add a link to the try run: https://tbpl.mozilla.org/?tree=Try&rev=3500e483a9f2

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8422023 [details] [diff] [review]
bug1009714.patch

Looks good, thanks!

I'm also just noticing that forms.css uses the ":disabled" pseudoclass instead of the [disabled] attribute selector. I imagine we probably should be using that throughout this file, too, though I'm not sure if there's a functional difference.

If that adds value. we can do a pass for that in a second bug; for now, this patch gets us to a better state than the one we're in now & fixes the bug.

Granting f+ and punting review to vingtetun, since I don't really have any ownership over this file and want to be sure I'm not missing something.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8422023 [details] [diff] [review]
bug1009714.patch

Actually, given fabrice's f+, I'm comfortable enough r+'ing this, since it's a trivial and pretty obviously just an oversight/typo. (and now we have input from someone involved with the existing code)

Could you post an updated version with a commit message & add 'checkin-needed' keyword?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> I'm also just noticing that forms.css uses the ":disabled" pseudoclass
> instead of the [disabled] attribute selector. I imagine we probably should
> be using that throughout this file, too, though I'm not sure if there's a
> functional difference.

Followed up on this, and discovered that we really should be using :disabled, to handle cases where form controls are disabled en masse inside of a fieldset element. I've filed bug 1010470 on that.

I still think this bug's patch is worth taking before that, though, to get us to a consistent state. That will allow bug 1010470 to be more of a clean mechanical conversion.

Comment 13

[Daniel Holbert [:dholbert]]

arnaud: while you're adding a commit message, could you rebase your patch on top of current mozilla-inbound (or at least on top of my modified version[1] of your patch for bug 1007278)?

The reftest.list tweak doesn't apply right now, after I swapped out "ref" for "notref" in disabled-3 through disabled-6. (You could fix this by applying the patch on top of my modified cset using fuzz, or by hand-editing the patch file to do the appropriate ref-->notref conversions at the very end).

[1] https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/9594562d60e9

Comment 14

[Daniel Holbert [:dholbert]]

(hand-editing *this* bug's patch file, I mean -- to fix the contextual lines at the end to match what actually landed in bug 1007278)

Comment 15

[Arnaud Bienner]

Attachment: bug1009714-2.patch

rebased on top of HEAD + commit message.
r+ per comment 11

Comment 16

[Daniel Holbert [:dholbert]]

(archeological note: it looks like this code was copypasted from a similar chunk of Android CSS (which also had the incorrect "=true" qualifier).  Then, this bug was subsequently fixed *in the Android CSS file* (but not the B2G CSS file) as part of this changeset:
 http://hg.mozilla.org/mozilla-central/rev/e60cab57965c#l1.34
which replaced button[disabled="true"] with button[disabled], along with making some other changes.)

Comment 17

[Daniel Holbert [:dholbert]]

(and before that, we had [disabled="true"] on all/most form controls in that android css file, but most of that was replaced with "[disabled]" in bug 561097 -- button was just the straggling thing with "=true")

Comment 18

[Daniel Holbert [:dholbert]]

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

Comment 19

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

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