Closed Bug 204237 Opened 18 years ago Closed 17 years ago

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

Categories

(Camino Graveyard :: HTML Form Controls, defect)

PowerPC
macOS
defect
Not set
trivial

Tracking

(Not tracked)

VERIFIED FIXED
Camino0.9

People

(Reporter: avi, Assigned: mikepinkerton)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

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
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.
Attached file 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
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.
Blocks: 197865
Keywords: regression
> 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?
taking for 0.8
Assignee: bryner → pinkerton
Target Milestone: --- → Camino0.8
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;
}
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;
}
landed as part of 228499.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
> 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.
Also, none of the select bug stuff affected the branch.
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)?
Yes we had. Just check and older build.
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.
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.
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.
Attached patch branch hack fix (obsolete) — Splinter Review
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.
Assignee: pinkerton → stuart.morgan
Status: REOPENED → ASSIGNED
Comment on attachment 150894 [details] [diff] [review]
branch hack fix

Trivial, as described, so skipping right to sr?.
Attachment #150894 - Flags: superreview?(pinkerton)
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.
Attachment #150894 - Flags: superreview?(pinkerton)
Attached patch Much better branch workaround (obsolete) — Splinter Review
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.
Attachment #150894 - Attachment is obsolete: true
Somehow grabbed the wrong file
Attachment #151406 - Attachment is obsolete: true
Comment on attachment 151407 [details] [diff] [review]
correct branch fix

Hopefully this can make 0.8
Attachment #151407 - Flags: superreview?(bryner)
Attachment #151407 - Flags: review?(pinkerton)
Comment on attachment 151407 [details] [diff] [review]
correct branch fix

r=pink
Attachment #151407 - Flags: review?(pinkerton) → review+
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.
Attachment #151407 - Flags: superreview?(bryner) → superreview+
> 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 on attachment 151407 [details] [diff] [review]
correct branch fix

a=asa
Attachment #151407 - Flags: approval1.7.1+
branch fix landed on branch. keeping open for better fix on trunk.
Target Milestone: Camino0.8 → Camino0.9
Attached patch trunk patchSplinter Review
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.
Attachment #152081 - Flags: superreview?(bryner)
Attachment #152081 - Flags: review?(pinkerton)
(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?
Giving my bugs back to pink.
Assignee: stuart.morgan → pinkerton
Status: ASSIGNED → NEW
Comment on attachment 152081 [details] [diff] [review]
trunk patch

We should work on pushing this through before it gets too old.
Attachment #152081 - Flags: review?(pinkerton) → review?(sfraser)
This still needs testing on Windows as far as I can tell.
Attachment #152081 - Flags: review?(sfraser) → review?(joshmoz)
Attachment #152081 - Flags: superreview?(bryner)
Attachment #152081 - Flags: superreview+
Attachment #152081 - Flags: review?(joshmoz) → review+
looked at it with darin and bryner, tested build on windows with this patch,
looks good

landed on trunk
Status: NEW → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
This regressed button theming on firefox for windows xp.  Windows classic
buttons are shown rather than the themed buttons.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
This patch fixes a type that would probably bust Windows widgets.
Attachment #164893 - Flags: superreview?(bryner)
Comment on attachment 164893 [details] [diff] [review]
fix windows button bustage

sr=darin

no need to request sr on trivial bustage fixes.
Attachment #164893 - Flags: superreview?(bryner) → superreview+
landed
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
I can't test on Max OS X but I filed bug 322424 because there is a regression on windows relating to this bug.   
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.