Closed Bug 260399 Opened 16 years ago Closed 15 years ago

disabled textarea/input should use DEFAULT pointer

Categories

(Core :: CSS Parsing and Computation, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: ginnchen+exoracle)

Details

Attachments

(4 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a4) Gecko/20040916 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a4) Gecko/20040916 Firefox/0.9.1+

I think this code -which I will attach as a patch- doesn't do anything useful
anymore. But maybe I'm completely wrong.
Boris told me to cc you guys, to have a look at it. 

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Attached patch patch (obsolete) — Splinter Review
Attached file Testcase
I thought that removing this code, would make a difference in this testcase.
But it doesn't seem to matter at all.
Maybe Ginn Chenn knows what it does.
Martijn,

I found the code works when the cursor is at the edge of disabled control.
Maybe we should fix it.
Attached file Exaggerated testcase (obsolete) —
Thanks Ginn Chenn! That seems to be the difference.
Here, I've made the padding and border of the form controls very large. 
Now, when the form controls are disabled, you can't set the mouse cursor by css
in some cases.
For checkboxes, radio controls, select drop down boxes and input type=submit
buttons you can't set the mouse cursor entirely when they are disabled.
For disabled textarea's and disabled input type=text boxes, the cursor isn't
set for the padding and the border area (while it is when it is not disabled).

The patch I attached, fixes this (tested in my debug build).
So I guess it would make sense to remove this code.
Attached file Exaggerated testcase
Argh, previous testcase was wrong. In this testcase, I've set the cursor to
crosshair for all form controls.
Attachment #161940 - Attachment is obsolete: true
Martijn,

I think the proper behavior is disabled form control will always use DEFAULT
cursor instead of css setting.
So we need to let disabled textarea/textbox not uses css cursor, just like
checkbox/radio/dropdown/button.
Attachment #159418 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Assignee: dbaron → ginn.chen
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Summary: Some unnecessary code in nsEventStateManager::UpdateCursor? → disabled textarea/input should use DEFAULT point
Attachment #161946 - Flags: superreview?(jst)
Attachment #161946 - Flags: review?(daniel)
Well, that's fine with me. But maybe it would be better if you could specify
that code with a !important setting in forms.css? 
But then Mozilla would need to have to support the :disabled selector:
http://www.w3.org/TR/css3-selectors/#enableddisabled

For the record, in Opera7.5 you can specify any cursor you like for disabled
form controls. In IE6 this is only the case for checkboxes and radio controls,
for the other form controls it is very partial or entirely not.
Attachment #161946 - Flags: review?(daniel) → review?(aaronleventhal)
Whiteboard: sunport17
Ian, what should we do for the mouse cursor over disabled form controls? Should
we always use the default cursor unless the rule was specified using :disabled
(which we don't support yet)?
Comment on attachment 161946 [details] [diff] [review]
patch

Ginn, there is no way to look at your new code and immediately understand what
it does. It's very unclear why we are checking the parent of anything, and why
that test is only on a div. We need to add code comments for all pieces of code
that are not obvious.
Attachment #161946 - Flags: superreview?(jst)
Attachment #161946 - Flags: review?(aaronleventhal)
Attachment #161946 - Flags: review-
Comment on attachment 161946 [details] [diff] [review]
patch

I applied the patch and it did not change the mouse cursor to the default
appearance for the disabled button or input field.
Attached patch patch with comments (obsolete) — Splinter Review
Revised to add comments in.
The patch works for me.

Aaron, have you rebuilt mozilla/layout after rebuilt
mozilla/content/events/src?
Attachment #161946 - Attachment is obsolete: true
Attachment #176676 - Flags: review?(aaronleventhal)
It's not in Sun Mozilla 1.7.
Whiteboard: sunport17
I don't understand the question nor the patch. bz?
Comment on attachment 176676 [details] [diff] [review]
patch with comments

That sounds wrong. Why would a div be inside a text area or input? Kyle and I
think the div should be outside of the text area.
Attachment #176676 - Flags: review?(aaronleventhal) → review-
> I don't understand the question nor the patch. bz?

The question is whether disabled form controls should use the default mouse
cursor or the mouse cursor specified in CSS.  Alternately, the question is
whether disabled form controls should have an !important rule setting the mouse
cursor to the default one.  The bug was initially filed on two things:

1)  We have some code to do this, and the code seems sucky
2)  The code works for all form controls except <textarea> and <input
    type="text>

I'm not quite sure how the last patch in this bug (which is _adding_ code)
thinks it addresses issue #1.

> Why would a div be inside a text area or input?

Because it's the anonymous div being edited by the editor on that control.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsTextControlFrame.cpp&rev=3.193&mark=1788-1792,1801#1788

Again, I think the right way to solve this is to remove all this gunk from the
C++ and add some !important rules to forms.css...
Attached patch patch v2 (obsolete) — Splinter Review
Remove all this gunk from the C++ and add some !important rules to forms.css
Attachment #176858 - Flags: superreview?(bzbarsky)
Attachment #176858 - Flags: review?(aaronleventhal)
Attachment #176676 - Attachment is obsolete: true
Comment on attachment 176858 [details] [diff] [review]
patch v2

More lines should be deleted.
Attachment #176858 - Attachment is obsolete: true
Attachment #176858 - Flags: superreview?(bzbarsky)
Attachment #176858 - Flags: review?(aaronleventhal)
Attached patch patch v2Splinter Review
try again...
Attachment #176859 - Flags: superreview?(bzbarsky)
Attachment #176859 - Flags: review?(aaronleventhal)
Much better, although I don't feel qualified to r= this. Bz, what do you think
of the new patch?
First of all, does that patch work?  If we needed to mess with the div before,
why do we suddenly not need to now?

Don't we need a xul.css change for the xul button cursor thing?
ccing front-end folks, since xul.css is likely to get involved.
(In reply to comment #22)
> First of all, does that patch work?  If we needed to mess with the div before,
> why do we suddenly not need to now?
> 

Yes, it works.
We messed with the div, because we need to judge whether it is disabled in C++ code.
If the user setting and default setting are both in .css, the problem is solved.

> Don't we need a xul.css change for the xul button cursor thing?

I don't know.
Comment on attachment 176859 [details] [diff] [review]
patch v2

I like the ESM cleanup but I'm not so convinced about the forms.css changes - I
would have thought cursor: inherit; would have been more useful.
Oh, and if you do use inherit then I don't mind if it's !important or not.
But if you feel default is better then I'd prefer that it wasn't !important.
> If the user setting and default setting are both in .css, the problem is
> solved.

Ah, because it's inherited.  Makes sense.

> I don't know.

Let me rephrase.  The ESM code affects what happens for XUL buttons.  Your new
code does not.  It should.

All that said, I think we need to step back and figure out what the goal is...
Neil's right that cursor:inherit makes more sense (effectively "turn off" our
setting of cursor for this widgets, but still allow the page to style it).  I
suspect that this is what the code was trying to achieve, though someone should
do the CVS archeology to dig up the bug that it was added for.

If that's what we want, we just want to use inherit (or -moz-initial), with no
!important (and make sure the ua.css rules have the right ordering/specificity
to beat the other ua.css rules).
(In reply to comment #27)
>Let me rephrase.  The ESM code affects what happens for XUL buttons.  Your new
>code does not.  It should.
I'm not convinced, because unlike textfields, buttons typically inherit the
cursor, although I'm still undecided as to what to expect for e.g. <hbox
style="cursor: pointer;"><button label="pointer" disabled="true"/></hbox> or
<button label="pointer" disabled="true" style="cursor: pointer;"/>
Hmm... we don't explicitly style the pointer on XUL buttons right now?  Then we
probably want to keep not doing it, yes.
(In reply to comment #26)
> Oh, and if you do use inherit then I don't mind if it's !important or not.
> But if you feel default is better then I'd prefer that it wasn't !important.

I think using default or inherit without !important is the same result, the
cursor is set in spite of whether it is disabled.
I don't know if it is what we what.

I tried the testcase with IE and Opera.
With IE, cursor on disabled input elements(checkbox, radio,text and submit) are
crosshair, on other disabled elements are not.
With Opera, cursor on disabled button element is crosshair, on other disabled
elements are default.

What should we do?
> I think using default or inherit without !important is the same result, the
> cursor is set in spite of whether it is disabled.

Only if the node is styled directly!  If an ancestor is styled, these behave
differently.

Sounds like we just want to make sure that forms.css sets no special cursor on
disabled elements (so use inherit, with no !important).
(In reply to comment #31)
> > I think using default or inherit without !important is the same result, the
> > cursor is set in spite of whether it is disabled.
> 
> Only if the node is styled directly!  If an ancestor is styled, these behave
> differently.

I mean the same result for this testcase.

If it is the expected behavior, we should change this bug's title.
Comment on attachment 176859 [details] [diff] [review]
patch v2

I don't feel qualified to review this. Maybe Neil?
Attachment #176859 - Flags: review?(aaronleventhal)
Comment on attachment 176859 [details] [diff] [review]
patch v2

r+sr=bzbarsky if you change those "default !important" in the CSS to "inherit".
Attachment #176859 - Flags: superreview?(bzbarsky)
Attachment #176859 - Flags: superreview+
Attachment #176859 - Flags: review+
Why should these be "!important" ?
Summary: disabled textarea/input should use DEFAULT point → disabled textarea/input should use DEFAULT pointer
They shouldn't.  They should be:

  cursor: inherit;

without a !important on them.  I guess I should have made that clearer in
comment 34.
patch for check in
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.561; previous revision: 1.560
done
Checking in content/events/src/nsEventStateManager.h;
/cvsroot/mozilla/content/events/src/nsEventStateManager.h,v  <-- 
nsEventStateManager.h
new revision: 1.130; previous revision: 1.129
done
Checking in layout/style/forms.css;
/cvsroot/mozilla/layout/style/forms.css,v  <--  forms.css
new revision: 3.101; previous revision: 3.100
done
Checking in layout/forms/resources/content/xbl-forms.css;
/cvsroot/mozilla/layout/forms/resources/content/xbl-forms.css,v  <--  xbl-forms.css
new revision: 1.27; previous revision: 1.26
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.