Closed
Bug 260399
Opened 20 years ago
Closed 20 years ago
disabled textarea/input should use DEFAULT pointer
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: ginnchen+exoracle)
Details
Attachments
(4 files, 5 obsolete files)
426 bytes,
text/html
|
Details | |
843 bytes,
text/html
|
Details | |
7.38 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
7.31 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Comment 2•20 years ago
|
||
I thought that removing this code, would make a difference in this testcase.
But it doesn't seem to matter at all.
Comment 3•20 years ago
|
||
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.
Reporter | ||
Comment 5•20 years ago
|
||
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.
Reporter | ||
Comment 6•20 years ago
|
||
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
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)
Reporter | ||
Comment 9•20 years ago
|
||
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)
Comment 10•20 years ago
|
||
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•20 years ago
|
||
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 12•20 years ago
|
||
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.
Assignee | ||
Comment 13•20 years ago
|
||
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)
Comment 15•20 years ago
|
||
I don't understand the question nor the patch. bz?
Comment 16•20 years ago
|
||
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-
Comment 17•20 years ago
|
||
> 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...
Assignee | ||
Comment 18•20 years ago
|
||
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
Assignee | ||
Comment 19•20 years ago
|
||
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)
Assignee | ||
Comment 20•20 years ago
|
||
try again...
Attachment #176859 -
Flags: superreview?(bzbarsky)
Attachment #176859 -
Flags: review?(aaronleventhal)
Comment 21•20 years ago
|
||
Much better, although I don't feel qualified to r= this. Bz, what do you think
of the new patch?
Comment 22•20 years ago
|
||
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•20 years ago
|
||
ccing front-end folks, since xul.css is likely to get involved.
Assignee | ||
Comment 24•20 years ago
|
||
(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•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
> 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•20 years ago
|
||
(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•20 years ago
|
||
Hmm... we don't explicitly style the pointer on XUL buttons right now? Then we
probably want to keep not doing it, yes.
Assignee | ||
Comment 30•20 years ago
|
||
(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•20 years ago
|
||
> 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).
Assignee | ||
Comment 32•20 years ago
|
||
(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•20 years ago
|
||
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 34•20 years ago
|
||
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
Comment 36•20 years ago
|
||
They shouldn't. They should be:
cursor: inherit;
without a !important on them. I guess I should have made that clearer in
comment 34.
Assignee | ||
Comment 37•20 years ago
|
||
patch for check in
Assignee | ||
Comment 38•20 years ago
|
||
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: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•