260399 - disabled textarea/input should use DEFAULT pointer

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

Comment 2

[Martijn Wargers (dead)]

Attachment: Testcase

I thought that removing this code, would make a difference in this testcase.
But it doesn't seem to matter at all.

Comment 3

[Aaron Leventhal]

Maybe Ginn Chenn knows what it does.

Comment 4

[Ginn Chen]

Martijn,

I found the code works when the cursor is at the edge of disabled control.
Maybe we should fix it.

Comment 5

[Martijn Wargers (dead)]

Attachment: Exaggerated testcase

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.

Comment 6

[Martijn Wargers (dead)]

Attachment: Exaggerated testcase

Argh, previous testcase was wrong. In this testcase, I've set the cursor to
crosshair for all form controls.

Comment 7

[Ginn Chen]

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.

Comment 8

[Ginn Chen]

Attachment: patch

Comment 9

[Martijn Wargers (dead)]

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.

Comment 10

[Aaron Leventhal]

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 11

[Aaron Leventhal]

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.

Comment 12

[Aaron Leventhal]

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.

Comment 13

[Ginn Chen]

Attachment: patch with comments

Revised to add comments in.
The patch works for me.

Aaron, have you rebuilt mozilla/layout after rebuilt
mozilla/content/events/src?

Comment 14

[Ginn Chen]

It's not in Sun Mozilla 1.7.

Comment 15

[Hixie (not reading bugmail)]

I don't understand the question nor the patch. bz?

Comment 16

[Aaron Leventhal]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 18

[Ginn Chen]

Attachment: patch v2

Remove all this gunk from the C++ and add some !important rules to forms.css

Comment 19

[Ginn Chen]

Comment on attachment 176858 [details] [diff] [review]
patch v2

More lines should be deleted.

Comment 20

[Ginn Chen]

Attachment: patch v2

try again...

Comment 21

[Aaron Leventhal]

Much better, although I don't feel qualified to r= this. Bz, what do you think
of the new patch?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

ccing front-end folks, since xul.css is likely to get involved.

Comment 24

[Ginn Chen]

(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 25

[[e-mail]]

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.

Comment 26

[[e-mail]]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 28

[[e-mail]]

(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;"/>

Comment 29

[Boris Zbarsky [:bzbarsky]]

Hmm... we don't explicitly style the pointer on XUL buttons right now?  Then we
probably want to keep not doing it, yes.

Comment 30

[Ginn Chen]

(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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 32

[Ginn Chen]

(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 33

[Aaron Leventhal]

Comment on attachment 176859 [details] [diff] [review]
patch v2

I don't feel qualified to review this. Maybe Neil?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176859 [details] [diff] [review]
patch v2

r+sr=bzbarsky if you change those "default !important" in the CSS to "inherit".

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why should these be "!important" ?

Comment 36

[Boris Zbarsky [:bzbarsky]]

They shouldn't.  They should be:

  cursor: inherit;

without a !important on them.  I guess I should have made that clearer in
comment 34.

Comment 37

[Ginn Chen]

Attachment: patch v2 for check in

patch for check in

Comment 38

[Ginn Chen]

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