204237 - Disabled <input> form elements don't have Aqua appearance

Status: VERIFIED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Avi Drissman]

If an <input> form element is disabled, it has the harsh cross-platform default
look rather than the cool Aqua look. It should look like a disabled text field.

Build ID: 2003050105

Comment 1

[Avi Drissman]

Attachment: Disabled and enabled text fields

Comment 2

[Chris Casciano]

This seems to have changed post 0.7.

My suspicion (gleaned from other bugs like bug 203438) is
that this may be a result of some extra styling applied to the disabled control.
So checking in on forms.css tho I only see this:

input[disabled],
textarea[disabled],
option[disabled],
optgroup[disabled],
select[disabled] {
  color: GrayText;
  background-color: ThreeDFace;
  cursor: default;
}

[2minutes later] Commenting those line out of forms.css and gives you the same
appearance as was in 0.7. I haven't done any testing to see if that causes any
regressions.

Comment 3

[Chris Casciano]

Attachment: complex testcase

Complex testcase including disabled controls other then input. slapped this
together to make sure commenting those lines out completely (as opposed to just
the input related selector) didn't break other disabled controls

Comment 4

[Chris Casciano]

I was going to just go ahead and make a patch to fix this specific problem but
it doesn't appear as if there's a *forms.css file specific to the Camino
project. My fix is noted above, but it appears it needs a place to go. An
alternate solution (as in so many of these form style bugs) is to ignore certain
style declarations outright.

Comment 5

[Avi Drissman]

> This seems to have changed post 0.7.

No surprise there; lots of widget styling broke on the trunk landing. See bug
197094 for a thorough sampling.

> it doesn't appear as if there's a *forms.css file specific to the Camino
project

I'm not familiar at all with the innards of Mozilla or Camino, but from the
attachments on bug 197094 there's a reference to:

layout/html/document/src/mac/platform-forms.css

Is that what you're looking for?

Comment 6

[Mike Pinkerton (not reading bugmail)]

taking for 0.8

Comment 7

[Jasper]

This is a verly easy thing to fix.

This should be added to platform.css:

input[disabled],
textarea[disabled] {
  border-width: 2px;
  padding: 0 2px 0 1px;
  font-size: 11px;
  background-color: transparent;
  margin: 2px 1px;
}

Comment 8

[Jasper]

Let me be more specific, this what it should look like:

input[disabled],
textarea[disabled] {
  border-width: 2px;
  padding: 0 2px 0 1px;
  font-size: 11px;
  background-color: transparent;
  margin: 2px 1px;
}

option[disabled],
optgroup[disabled],
select[disabled] {
  color: GrayText !important;
}

Comment 9

[Mike Pinkerton (not reading bugmail)]

landed as part of 228499.

Comment 10

[Stuart Morgan]

This regressed again at some point, since it's broken in the recent 0.8b builds
(probably trunk as well, but I haven't checked). Could someone check through the
nightlies for the past few months and find out when it regressed?

Comment 11

[Jasper]

As far as I can see nothing changed in the platform css file, I'm afraid this
aslo has to do with the button/select bug you made a patch for that braked input
field styling. Let's landthat fix first and see if it fixes this. Especially
since the css file is still intact.

Comment 12

[Stuart Morgan]

> I'm afraid this aslo has to do with the button/select bug you made
> a patch for that braked input field styling.

I don't think so; the bug I introduced should *prevent* any widgets from
reverting to the non-themed look (which is why we can't style them). I think
this is probably something else.

Comment 13

[Stuart Morgan]

Also, none of the select bug stuff affected the branch.

Comment 14

[Stuart Morgan]

The culprit is the mac patch for bug 237138. Removing that patch makes <input>s
look aqua even when disabled. We'll have to figure out how to work around that.

Without it, though, disabled <input>s look like enabled ones except for the grey
text. Did we ever have a disabled version that looked correct (lighter grey border)?

Comment 15

[Jasper]

Yes we had. Just check and older build.

Comment 16

[Josh Aas]

Is anybody working on this? Will somebody that knows more about this than I do
post thougths on whether or not this should block 0.8? Please post a good
argument one way or the other if you have time. For the time being I'm leaving
this as non-blocking due to its "trivial" status.

Comment 17

[Stuart Morgan]

I'm working on it in that I definitely plan to take a look at it more this week,
but haven't had a chance yet.  The biggest problem that I see is that the
disabled form controls look as active (if not more) as the enabled ones, since
the disabled ones get the bulky gtk look.  I first found the bug because I was
confused as to why an input wasn't working--most users will stay confused rather
than discovering that it's a reversion to gtk-style widgets.

Comment 18

[Stuart Morgan]

I'll have a patch for this shortly. The only clean way to do it that I see
involves refactoring in nsNativeTheme slightly though, so we'll need external
reviewing and 1.7 permission to get this into 0.8.

Comment 19

[Stuart Morgan]

Attachment: branch hack fix

In the interests of getting this done in time for 0.8, I'm sacrificing
maintainability and dignity in favor of speed and safety. My hope is that we
can put this patch into the branch now, as it's cocoa-only, then I'll make a
real patch for the trunk in the next week or two.

This patch replicates the entirety of IsWidgetStyled into nsNativeThemeMac,
then makes one tiny change--always using moz_field as the background of
textfields, instead of using the border color as the background when disabled
and thus triggering the reversion to gfx.

Despite the size, it's a trivial patch. The *only* effect is to change the
behavior of unstyled text inputs to not incorrectly claim they are styled.

Comment 20

[Stuart Morgan]

Comment on attachment 150894 [details] [diff] [review]
branch hack fix

Trivial, as described, so skipping right to sr?.

Comment 21

[Stuart Morgan]

Comment on attachment 150894 [details] [diff] [review]
branch hack fix

The hack version appears to be unpopular. I'll post a good patch for both
branch and trunk ASAP.

Comment 22

[Stuart Morgan]

Attachment: Much better branch workaround

Duh. This is the much, much better quick branch fix.

A more comprehensive trunk patch will still be coming soon, so this is 1.7/0.8
only.

Comment 23

[Stuart Morgan]

Attachment: correct branch fix

Somehow grabbed the wrong file

Comment 24

[Stuart Morgan]

Comment on attachment 151407 [details] [diff] [review]
correct branch fix

Hopefully this can make 0.8

Comment 25

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 151407 [details] [diff] [review]
correct branch fix

r=pink

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 151407 [details] [diff] [review]
correct branch fix

fwiw, I think the correct trunk fix would be to make a new static on
nsNativeTheme.

Comment 27

[Stuart Morgan]

> fwiw, I think the correct trunk fix would be to make a new static on
> nsNativeTheme.

Yes, my plan is to make all of the defauts in IsWidgetStyled into statics. That
way the function will be consistent, it will make more sense, and no subclasses
will ever run into something like this again.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 151407 [details] [diff] [review]
correct branch fix

a=asa

Comment 29

[Mike Pinkerton (not reading bugmail)]

branch fix landed on branch. keeping open for better fix on trunk.

Comment 30

[Stuart Morgan]

Attachment: trunk patch

The good patch, for the trunk. Takes every hard-coded default from
IsWidgetStyled and replaces it with a static class member. This allows
subclasses to tailor defaults to their particular CSS without re-implementing
the whole function.

Fixes the Camino bug by then overriding the one problematic default.

I tried to be very careful to preserve the behavior, but since I can't build on
Windows I couldn't test all the logic to be absolutly sure I didn't make a
mistake. Someone will need to run a simple test of all the affected controls on
a windows build.

Comment 31

[Hiro]

(In reply to comment #26)
> (From update of attachment 151407 [details] [diff] [review])
> fwiw, I think the correct trunk fix would be to make a new static on
> nsNativeTheme.
> 

The patch is not checkined yet to AVIARY_1_0_20040515_BRANCH.
Isn't a problem in Firefox?

Comment 32

[Stuart Morgan]

Giving my bugs back to pink.

Comment 33

[Josh Aas]

Comment on attachment 152081 [details] [diff] [review]
trunk patch

We should work on pushing this through before it gets too old.

Comment 34

[Simon Fraser [no longer active]]

This still needs testing on Windows as far as I can tell.

Comment 35

[Josh Aas]

looked at it with darin and bryner, tested build on windows with this patch,
looks good

landed on trunk

Comment 36

[Stephen Walker]

This regressed button theming on firefox for windows xp.  Windows classic
buttons are shown rather than the themed buttons.

Comment 37

[Josh Aas]

When I tested on windows with Darin, buttons looked themed to me on Windows.
Anyway, architecturally this patch is a step in the right direction so if there
really is a regression I recommend fixing the problem instead of backing out.

Comment 38

[Josh Aas]

Attachment: fix windows button bustage

This patch fixes a type that would probably bust Windows widgets.

Comment 39

[Darin Fisher]

Comment on attachment 164893 [details] [diff] [review]
fix windows button bustage

sr=darin

no need to request sr on trivial bustage fixes.

Comment 40

[Josh Aas]

landed

Comment 41

[Matthew Schultz]

I can't test on Max OS X but I filed bug 322424 because there is a regression on windows relating to this bug.