742431 - Remove IsDisabled() in favor of EventStates

Status: RESOLVED DUPLICATE of bug 1405087

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Why not keep IsDisabled() to return State().HasState(NS_EVENT_STATE_DISABLED), but more readable?

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

>+   * We need a method to do this because of the recursive check.

How about:

  This should only be called from intrinsic state computation.

instead?

r=me

Comment 4

[Mounir Lamouri (:mounir)]

Actually, this is failing some tests because checking State() in IntrinsicState() is wrong because mState isn't updated so it returns a wrong value. I should try to find an idea to fix that cleanly. Boris, feel free to step in if you have one...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er... what's checking State() in IntrinsicState()?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Er... what's checking State() in IntrinsicState()?

Arg, I've been looking at ::AfterSetAttr() thinking it was ::IntrinsicState() yesterday... It's not that but something more subtle actually: when fieldset's disabled state is changed, all descendants have FieldSetDisabledChanged() called and form controls that can be invalid will update their validity state based on that change, *then*, UpdateState() is called. Which means NS_EVENT_STATE_DISABLED will have the same state when checking the validity state.

The cleanest way I see to fix that would be to have UpdateState() called twice: once before checking the validity and once after. With a callback when states change (I have a patch doing that), we could actually update the validity state on state change. Would that be an okay solution?

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

I'm adding a call to UpdateState(aNotify) in *::FieldSetDisabledChanged(). This is fixing the bug but might make us notify twice for a state change Shouldn't be horrible though.

If we implement a callback when a state changes, we could remove *::FieldSetDisabledChanged() and only update the validity when NS_EVENT_STATE_DISABLED is changed. However, that would make us call UpdateState() for the StateChanged() callback called from UpdateState().

Comment 8

[Boris Zbarsky [:bzbarsky]]

> +   * We need a method to do this because of the recursive check.

See comment 3.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 612879 [details] [diff] [review]
Patch v2.1

r=me

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > +   * We need a method to do this because of the recursive check.
> 
> See comment 3.

Oups, I missed that. It's fixed locally.

So, you confirm you are okay with the two UpdateState() call after each other?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, that sounds reasonable to me.

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

This is done in: