Open Bug 559176 Opened 11 years ago Updated 8 years ago

[XUL] textbox which have disabled attribute is not disabled on the border

Categories

(Core :: XUL, defect)

defect
Not set
minor

Tracking

()

People

(Reporter: shadow912kage, Unassigned)

Details

Attachments

(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100413 Minefield/3.7a5pre BuildID: 20100413040241 

when textbox have disabled="true" attribute, tooltip isn't shown and click event isn't sent on the text field.
but on the border, tooltip is shown and click event is sent.

this issue occurs on Fx 2/3/3.5/3.6, TB 2/3/Lanikai/Shredder too.

Reproducible: Always

Steps to Reproduce:
1.hover mouse pointer to disabled textbox border.
2.when tooltip is shown, then click.
3.
Actual Results:  
tooltip is shown and click event is sent.

Expected Results:  
tooltip isn't shown and click event isn't sent.

this issue doesn't occur for similar HTML case.
Version: unspecified → Trunk
Attached file similar HTML case
Assignee: nobody → netzen
I can reproduce this issue, marking as NEW.  

However I believe the expected result should be:
Tooltip is shown whether you hover on the textbox or the border and whether disabled or enabled.  Click event is not handled on neither the textbox nor border when disabled. 

See related Bug 595161 (not exactly the same issue):
> It is important that tooltips show for disabled widgets so a reason for being disabled can be displayed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This patch applies to both of these overlapping tasks:
Bug 595161 - Disabled XUL textbox doesn't display tooltiptext.
Bug 559176 - [XUL] textbox which have disabled attribute is not disabled on the border 

Attached a patch which fixes several problems as described below in the "Testing notes" section.
The "Development notes" section describes what changes do what for the code review.
The patch is based on mozilla-central.

I ran the following tests after creating the patch and they all pass:
- reftests in /content\test\reftest\reftest.list
- all of the mochitest-plain and mochitest-browser-chrome within /content/html and /content/xul

Testing notes:
- Regarding disabled HTML items and tooltips:
  The way it should work is that disabled items should have tooltips (to display reasoning of why it is disabled for example)
  The way I just described is how it works in IE, Chrome/Safari, Opera
  The way it used to work before the fix in Firefox (for as far back as I could find) is that disabled HTML elements would not show tooltips.
  The way it works after the fix in Firefox is the same as how it works in IE, Chrome/Safari, Opera
- Regarding disabled HTML items and click events:
  There was no bug here in Firefox, disabled item click events are ignored as they should be.

- Regarding disabled XUL items and tooltips:
  The way it should work is that disabled items should have tooltips (to display reasoning of why it is disabled for example)
  I base how it should work to be similar to how HTML works, because I can't try on other browsers since they don't do XUL
  The way it used to work before the fix in Firefox (for as far back as I could find) is that disabled XUL elements would not show tooltips, but their borders would
  The way it works after the fix in Firefox is that both the border and the element itself shows tooltips. 
- Regarding disabled XUL items and click events:
  The way it should work is that disabled items should ignore click events
  The way it used to work before the fix in Firefox (for as far back as I could find) is that disabled XUL elements would not handle click events, but their borders would
  The way it works after the fix in Firefox is that both the border and the element itself ignore click events.
  
Development notes:
The changes to content\xul\content\src\nsXULElement.cpp
- Fixes problem with disabled XUL textboxes handling click events when clicking on disabled items borders
- Fixes problem with disabled XUL checkboxes handling click events when clicking on disabled items borders
  The problem was that like the HTML input element already had, we needed to suppress events on disabled items except for NS_MOUSE_MOVE

The changes to content\html\content\src\nsHTMLInputElement.cpp
- Fixes problem with disabled HTML input elements not showing tooltips 
  The problem was that PreHandleEvent was suppressing the NS_MOUSE_MOVE events which layout\xul\base\src\nsXULTooltipListener.cpp needed in order to display the tooltip

The changes to content\html\content\src\nsHTMLButtonElement.cpp
- Fixes problem with disabled HTML button elements not showing tooltips 
  The problem was that PreHandleEvent was suppressing the NS_MOUSE_MOVE events which layout\xul\base\src\nsXULTooltipListener.cpp needed in order to display the tooltip

The changes to content\html\content\src\nsHTMLTextAreaElement.cpp
- Fixes problem with disabled HTML textarea elements not showing tooltips
  The problem was that PreHandleEvent was suppressing the NS_MOUSE_MOVE events which layout\xul\base\src\nsXULTooltipListener.cpp needed in order to display the tooltip

The changes to content\html\content\src\nsHTMLSelectElement.cpp
- Fixes problem with disabled HTML select/option elements not showing tooltips

The changes to content\html\content\src\nsHTMLFieldSetElement.cpp
- Fixes problem with disabled HTML fieldset elements not showing tooltips
Attachment #539711 - Flags: review?(bzbarsky)
Attachment #539711 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 539711 [details] [diff] [review]
Fixes for disabled HTML and XUL tooltips not showing, and the widget borders handling events when they shouldn't

> nsXULElement::PreHandleEvent(nsEventChainPreVisitor& aVisitor)
> {
>+  nsCOMPtr<nsIDOMXULControlElement> xulControl = 
>+    do_QueryInterface(static_cast<nsIContent*>(this));
Adding new QI to event handling is really not acceptable.
It would slow down event handling significantly.
We should use some flag which is set when control becomes disabled
(if possible).
But I'm not sure this is what we want to do in XUL.
Better to ask enndeakin.


For Form controls we should actually get rid of most of the
don't-handle-events-on-disabled-elements, and in a way the patch is a right
step in that case.
Attachment #539711 - Flags: review?(Olli.Pettay) → review-
Thanks for the review Olli, I agree that QI should not be present in the PreHandleEvent, I think it is acceptable to instead use HasAttr. I will attach a patch I just finished shortly for review by enndeakin.
I attached an updated patch with the same notes as last time in Comment 5.

The only difference from the old patch is the new one will use HasAttr for disabled item detection.
Attachment #539711 - Attachment is obsolete: true
Attachment #540277 - Flags: review?(enndeakin)
(In reply to comment #5)
> - Regarding disabled XUL items and click events:
>   The way it should work is that disabled items should ignore click events
>   The way it used to work before the fix in Firefox (for as far back as I
> could find) is that disabled XUL elements would not handle click events, but
> their borders would
>   The way it works after the fix in Firefox is that both the border and the
> element itself ignore click events.
>   

That's not correct. The disabled state of a XUL element should have no effect on which mouse events fire. That is, both mousemove and click should fire regardless.

And yes, checking the disabled state through the nsIDOMXULControlElement interface is the right way to check for a disabled xul element, not to check the disabled attribute directly, as elements that do not implement that interface cannot be disabled (as they aren't expected to be controls).

Note that the most of the tests for xul elements are in toolkit/content/tests/
OK thanks for the clarifications Neil.  

Is the HTML related ways that I wrote in Comment 5 correct? Or do we work different from the other browsers?  If the HTML way I described is correct it seems it would be in contrast with the way the XUL elements work, but the XUL elements are sometimes based on HTML elements.  For example textbox.xml is based on html:input.  Perhaps I'd need to traverse the DOM in the HTML cpp files to see what the parent is and then act based on that in pre handle event?

Thanks in advance.
In my next patch by the way I'll use nsIDOMXULControlElement interface to determined if disabled, and also run the tests in toolkit/content/tests instead of only /content/html and /content/xul.
The click event in html is a bit odd, mostly for compatibility with early browsers, as it can fire even for key events. XUL doesn't have any backwards compatibility issue, so didn't inherit any of the odd behaviour.

Only <textbox> is defined in terms of an html element. That was probably done to make implementing it quicker, but it's caused a number of problems.

I don't think there's any bug here for xul elements for click events, just for the mousemove event on the html:input inside a <textbox>. Am I understanding correctly?
> Only <textbox> is defined in terms of an html element.

OK good to know that. Thanks.

> I don't think there's any bug here for xul elements for click events, just for the mousemove event on the html:input inside a <textbox>. Am I understanding correctly?

Not exactly correct.  Before my fixes, clicking on the border of a disabled XUL editbox will handle the click event (which you mentioned is correct), but clicking on the actual contained html:input will not handle the click event (but it should).  Likewise the border will show a tooltip when you mouse move, but the contained html:input would not show the tooltip. 

After my fix the tooltip issue is working like it should in both XUL editboxes and across all HTML elements, showing on disabled. But the patch in error is currently not accepting click events on neither a disabled editbox's border nor its html:input when contained inside the XUL editobox.  

---

Where to go from here: 
So I think the fix in nsXULElement.cpp/.h should be removed as the events should go through no matter what. 
Likewise I think that nsHTMLInputElement.cpp would need to do something like check if the parent is XUL and if so it would allow all events through.

Please let me know if that's how you would like me to proceed or if you'd prefer in some way to remove the html:input from the XBL textbox.xml.
Update: Talked to Neil and Olli on IRC, details contained below.  

How it should work:
XUL elements: Disabled items should show tooltips and handle click events.
HTML elements: Disabled items should show tooltips but not handle click events (like other browsers).

I will provide the patch later for tooltips in Bug 595161
This patch will allow disabled HTML items to show tooltips and hence also the XUL editbox which is based on an htlp:input.

I will leave this bug (Bug 559176) for the click event being handled in XUL when clicking on the disabled editbox XBL's contained html:input.
Attachment #540277 - Flags: review?(enndeakin)
This bug is about tooltips and click events.

The tooltip part of this task is a duplicate of Bug 595161 which is in turn fixed in Bug 274626.

Since XUL textboxes' are based on html:inputs I'm not sure what the best way to handle click events is since they should work different as described in Comment 14.
Assignee: netzen → nobody
You need to log in before you can comment on or make changes to this bug.