Closed Bug 451771 Opened 13 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: v.sinnlos, Assigned: u518207, Mentored)

References

()

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 1 obsolete file)

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
oh sorry, i think it's only because the background color on vista of a text input field is white and not grey
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
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)?
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
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.
Component: Layout: Form Controls → Widget: Win32
QA Contact: layout.form-controls → win32
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
This seems to be correctly in the "Widget: Win32" component, where a fix won't affect OS X.
Attached file test case
(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.
Status: NEW → RESOLVED
Closed: 13 years ago10 years ago
Resolution: --- → INVALID
Duplicate of this bug: 872071
(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?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Let's at least reopen this, in any case. :-)
Mentor: jmathies, gijskruitbosch+bugs
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Whiteboard: [lang=c++]
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.
Attached patch 451771.patch (obsolete) — Splinter Review
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.
Attachment #8506223 - Flags: review?(gijskruitbosch+bugs)
Attachment #8506223 - Flags: review?(gijskruitbosch+bugs) → review?(jmathies)
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?
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.
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?
Whatever you say.
The constant seems to be as in Chromium.
(In reply to mycoolclub from comment #18)
> The constant seems to be as in Chromium.

Ok, thanks for checking.
Is something missing to get it closed?
Attachment #8506223 - Attachment is obsolete: true
Attachment #8506223 - Flags: review?(jmathies)
Attached patch 451771.patchSplinter Review
Attachment #8512557 - Flags: review?(jmathies)
(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.
Of course.
Comment on attachment 8512557 [details] [diff] [review]
451771.patch

Looks great!
Attachment #8512557 - Flags: review?(jmathies) → review+
as soon as try opens I'll push this for a test run.
Flags: needinfo?(jmathies)
Assignee: nobody → mycoolclub
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Good.
https://hg.mozilla.org/mozilla-central/rev/b03320f5bab6
Status: REOPENED → RESOLVED
Closed: 10 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.