451771 - disabled of input (type=text) doesn't grey the background

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[geki007]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

if you disable a input, than the background color doesn't change to grey on vista, but i does on XP.

i think the disable function itself works correctly.

Reproducible: Always

Steps to Reproduce:
1. disable a input field (i disabled a input type=text field)
Actual Results:  
than the background color doesn't change to grey on vista, but i does on XP

Expected Results:  
the background color should change like on XP

Comment 1

[geki007]

oh sorry, i think it's only because the background color on vista of a text input field is white and not grey

Comment 2

[geki007]

this problem seems to be not on all sites but on the smf (http://www.simplemachines.org/) login sites.

is it a problem of the site or a bug of firefox (because on XP it works correctly)?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  I wouldn't be surprised if the native theme on Vista provides different default "disabled control" colors from XP.  We're just doing whatever the OS tells us here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Same can be observed too on OS X. It is visible with the following steps with latest Minefield:

1. Open the bookmarks sidebar
2. Open context menu of bookmarks root folders, e.g. Bookmarks menu
3. Open properties dialog

We don't allow editing in the dialog and the textbox should be disabled. It's fine on XP but has a white background on Vista and even get the focus ring on OS x. Will OS X be a separate bug or can we change this bug to All/All?

Comment 5

[Dão Gottwald [:dao]]

This seems to be correctly in the "Widget: Win32" component, where a fix won't affect OS X.

Comment 6

[Jim Mathies [:jimm]]

Attachment: test case

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Hmm.  I wouldn't be surprised if the native theme on Vista provides
> different default "disabled control" colors from XP.  We're just doing
> whatever the OS tells us here.

yep. tested on win7 and xp in fx and ie, white is the correct background.

Comment 9

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from bug 872071 comment #15)
> (In reply to :Gijs Kruitbosch from bug 872071 comment #14)
> > Jim, you marked this invalid in bug 451771 - but it seems IE and Chrome act
> > differently here. Is that reason enough to reconsider?
> 
> dupe this over to bug 451771 I guess? Also, that would make a [good first
> bug][lang=c++], with you as the mentor! :)

Would it? I have some questions:

1) which nsLookAndFeel magic thing is this? I didn't find it when looking for it. And isn't this (also) going to involve changing layout/style/forms.css ?
2) if we're going to override the OS default, what value are we picking if we want to deal with things like high contrast themes correctly?

Comment 10

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Jim Mathies [:jimm] from bug 872071 comment #15)
> > (In reply to :Gijs Kruitbosch from bug 872071 comment #14)
> > > Jim, you marked this invalid in bug 451771 - but it seems IE and Chrome act
> > > differently here. Is that reason enough to reconsider?
> > 
> > dupe this over to bug 451771 I guess? Also, that would make a [good first
> > bug][lang=c++], with you as the mentor! :)
> 
> Would it? I have some questions:
> 
> 1) which nsLookAndFeel magic thing is this? I didn't find it when looking
> for it. And isn't this (also) going to involve changing
> layout/style/forms.css ?
> 2) if we're going to override the OS default, what value are we picking if
> we want to deal with things like high contrast themes correctly?

I'm not sure about look and feel constants, this could be a system color we draw or some sort of themed element that we rendered using the theme lib. Testing with the test case though I see IE and chrome now paint grayed out text edits on Win7.

Comment 11

[:Gijs (he/him)]

Let's at least reopen this, in any case. :-)

Comment 12

[u518207]

If we open file nsNativeThemeWin.cpp, GetThemePartAndState() method and look at NS_THEME_TEXTFIELD case; we can find that for Vista or later the border color of a filed shows whether it is disabled as I suppose.

Comment 13

[u518207]

Attachment: 451771.patch

Here is my proposal for the correction of this bug. 
The new case was added to the drawing method DrawWidgetBackground() that changes the drawing behavior only for the case, when the field is disabled; in which case the grey rectangular background is painted.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8506223 [details] [diff] [review]
451771.patch

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

::: widget/windows/nsNativeThemeWin.cpp
@@ +1832,5 @@
> +  else if (aWidgetType == NS_THEME_NUMBER_INPUT ||
> +           aWidgetType == NS_THEME_TEXTFIELD ||
> +           aWidgetType == NS_THEME_TEXTFIELD_MULTILINE) {
> +    DrawThemeBackground(theme, hdc, part, state, &widgetRect, &clipRect);
> +     if (state == TFS_EDITBORDER_DISABLED) {

I dont understand why we have to paint this gray background manually, shouldn't DrawThemeBackground (which receive both part and state) paint the correct background? What's going wrong here?

Comment 15

[u518207]

I found only two options that paint the background, but they both wasn't satisfiable:
part name - EP_BACKGROUND did not have a border and EP_EDITTEXT looked different (larger size, etc.). To preserve original drawing format, it was chosen just to fill the background.

Comment 16

[Jim Mathies [:jimm]]

Taking a look at chromium's code for this, looks like they are handling this similarly - 

https://code.google.com/p/chromium/codesearch#chromium/src/ui/native_theme/native_theme_win.cc&q=DrawThemeBackground&sq=package:chromium&type=cs&l=1545

I'm left wondering about the background color here, and whether we're using the right constant?

Comment 17

[u518207]

Whatever you say.

Comment 18

[u518207]

The constant seems to be as in Chromium.

Comment 19

[Jim Mathies [:jimm]]

(In reply to mycoolclub from comment #18)
> The constant seems to be as in Chromium.

Ok, thanks for checking.

Comment 20

[u518207]

Is something missing to get it closed?

Comment 21

[u518207]

Attachment: 451771.patch

Comment 22

[Jim Mathies [:jimm]]

(In reply to mycoolclub from comment #20)
> Is something missing to get it closed?

No, I just need to apply and test the final patch. I've got an internal deadline I'm working on so if you can give me a day or so I'll get back to this.

Comment 23

[u518207]

Of course.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 8512557 [details] [diff] [review]
451771.patch

Looks great!

Comment 25

[Jim Mathies [:jimm]]

as soon as try opens I'll push this for a test run.

Comment 26

[u518207]

Good.

Comment 27

[Jim Mathies [:jimm]]

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4f289f014c46

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b03320f5bab6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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