598329 - Disabled XUL widgets are displayed as if they were enabled

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Dão Gottwald [:dao]]

Attachment: screenshot

Comment 1

[Dão Gottwald [:dao]]

Actually this affects all sorts of widgets.

Comment 2

[Dão Gottwald [:dao]]

Attachment: more states, broken post bug 557087

Comment 3

[Dão Gottwald [:dao]]

Attachment: more states, correct pre bug 557087

Comment 4

[Mounir Lamouri (:mounir)]

It looks like you inverted both screenshots, don't you?

Comment 5

[Mounir Lamouri (:mounir)]

Looks like part 4 of bug 557087 has to be the cause. I changed how widget was checking the disabled state of the elements using the content states instead of the disabled attribute. However, it looks like XUL elements behave differently...

I can re-create a disabled method which will use NS_EVENT_STATE_DISABLED for HTML elements and something else for XUL ones but I guess if we do that, XUL elements will not look disabled when inside a disabled fieldset. But I guess we do not really care?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't care.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 477356 [details] [diff] [review]
Patch v1

I'm not the right person to review this.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // For XML/XUL elements, an attribute must be equal to the literal
+  // string "true" to be counted as true.  An empty string should _not_
+  // be counted as true.
+  return content->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled,
+                              NS_LITERAL_STRING("true"), eCaseMatters);

I don't think we should do this for all XML elements. Creating behaviour for the "disabled" attribute for all elements in all namespaces seems wrong.

Let's just do this for XUL and check NS_EVENT_STATE_DISABLED for the other elements. Or maybe it would be even better for NS_EVENT_STATE_DISABLED state to be supported on XUL elements?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> +  // For XML/XUL elements, an attribute must be equal to the literal
> +  // string "true" to be counted as true.  An empty string should _not_
> +  // be counted as true.
> +  return content->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled,
> +                              NS_LITERAL_STRING("true"), eCaseMatters);
> 
> I don't think we should do this for all XML elements. Creating behaviour for
> the "disabled" attribute for all elements in all namespaces seems wrong.

This is what CheckBoolAttr was doing. This new method very similar to CheckBookAttr except that it's looking at the content states instead of the attribute when the element an HTML element.
Before, IsDisabled() was just calling CheckBoolAttr() with nsWidgetAtoms::disabled.

> Let's just do this for XUL and check NS_EVENT_STATE_DISABLED for the other
> elements. Or maybe it would be even better for NS_EVENT_STATE_DISABLED state to
> be supported on XUL elements?

Actually, I thought it would be hard to do that but given that you mentioned it I had a look at nsXULElement.cpp and it "seems" simple. I will try tomorrow.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

So, this patch is adding NS_EVENT_STATE_DISABLED and NS_EVENT_STATE_ENABLED to XUL elements when they can be disabled.

Thank you for the suggestion roc :)

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 478119 [details] [diff] [review]
Patch v2

hyatt hasn't reviewed any Mozilla code for a long time :-)

Comment 14

[Mounir Lamouri (:mounir)]

Boris, this patch is adding a QI in the IntrinsicState() method of nsXULElement. Do you think we should prevent that because it's too expensive?

The other solutions would be to have an hardcoded list of XUL elements which can be disabled or having nsIDOMXULControlElement informs the element it can now be disabled. It might be others... that's the first time I touch content/xul/.

Comment 15

[Boris Zbarsky [:bzbarsky]]

It'd be really nice to not have to QI there every time, yes... especially since that's not compatible with the direction I'd like to move states.  ;)

It looks like disabled == "is xul control and @disabled=='true'" right?  And enabled == "is xul control and not disabled"?  And of course whether we QI to XUL control depends on our XBL bindings, which depend on our style, which depends on whether we match :disabled, right?  Given that, I think that trying to base :disabled/:enabled matching on the "is xul control" QI is just broken, pending XBL2 existing.  Note that your code in this patch doesn't handle dynamic changes to the disabled state correctly, for example...

So I'd prefer to not change the intrinsic state for XUL for now, use the "Let's just do this for XUL and check NS_EVENT_STATE_DISABLED for the other elements." approach for the moment, and then once we have xbl2 we can make the xul control stuff actually reflect its disabled state into the intrinsic state (via new API, if needed).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478119 [details] [diff] [review]
Patch v2

I don't think this is a good approach as things stand.  Too easy to shoot yourself with the resulting footgun.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 477356 [details] [diff] [review]
Patch v1

Asking back a review for this patch per bz comment.

Comment 18

[Johnny Stenback (:jst)]

Given that this is a very visual regression I think we should take this for beta7.

Comment 19

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/1ae0e3bd04d0