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

RESOLVED FIXED in mozilla36

Status

()

Core
Widget: Win32
--
trivial
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: geki007, Assigned: u518207, Mentored)

Tracking

unspecified
mozilla36
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

9 years ago
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 → ---

Updated

9 years ago
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.

Comment 6

7 years ago
Created attachment 534467 [details]
test case

Comment 7

7 years ago
(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
Last Resolved: 9 years ago7 years ago
Resolution: --- → INVALID

Updated

3 years ago
Duplicate of this bug: 872071

Comment 9

3 years ago
(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)

Comment 10

3 years ago
(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)

Comment 11

3 years ago
Let's at least reopen this, in any case. :-)
Mentor: jmathies@mozilla.com, gijskruitbosch+bugs@gmail.com
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Whiteboard: [lang=c++]
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 13

3 years ago
Created attachment 8506223 [details] [diff] [review]
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.
Attachment #8506223 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Attachment #8506223 - Flags: review?(gijskruitbosch+bugs) → review?(jmathies)

Comment 14

3 years ago
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?
(Assignee)

Comment 15

3 years ago
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

3 years ago
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?
(Assignee)

Comment 17

3 years ago
Whatever you say.
(Assignee)

Comment 18

3 years ago
The constant seems to be as in Chromium.

Comment 19

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

Ok, thanks for checking.
(Assignee)

Comment 20

3 years ago
Is something missing to get it closed?
(Assignee)

Updated

3 years ago
Attachment #8506223 - Attachment is obsolete: true
Attachment #8506223 - Flags: review?(jmathies)
(Assignee)

Comment 21

3 years ago
Created attachment 8512557 [details] [diff] [review]
451771.patch
Attachment #8512557 - Flags: review?(jmathies)

Comment 22

3 years ago
(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.
(Assignee)

Comment 23

3 years ago
Of course.

Comment 24

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

Looks great!
Attachment #8512557 - Flags: review?(jmathies) → review+

Comment 25

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

Updated

3 years ago
Flags: needinfo?(jmathies)

Updated

3 years ago
Assignee: nobody → mycoolclub
Flags: needinfo?(jmathies)

Updated

3 years ago
Flags: needinfo?(jmathies)
(Assignee)

Comment 26

3 years ago
Good.

Comment 27

3 years ago
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4f289f014c46
Flags: needinfo?(jmathies)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03320f5bab6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b03320f5bab6
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.