Closed Bug 260399 Opened 17 years ago Closed 17 years ago
disabled textarea/input should use DEFAULT pointer
426 bytes, text/html
843 bytes, text/html
7.38 KB, patch
|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.
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.
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.
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.
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
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)
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.
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.
Revised to add comments in. The patch works for me. Aaron, have you rebuilt mozilla/layout after rebuilt mozilla/content/events/src?
It's not in Sun Mozilla 1.7.
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...
Remove all this gunk from the C++ and add some !important rules to forms.css
Comment on attachment 176858 [details] [diff] [review] patch v2 More lines should be deleted.
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?
Comment on attachment 176859 [details] [diff] [review] patch v2 r+sr=bzbarsky if you change those "default !important" in the CSS to "inherit".
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: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.