Bug 112085 – Refactoring: Pull Node::disabled() and Node::isInert() down to Element.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Suggested at https://bugs.webkit.org/show_bug.cgi?id=110952#c16.

[Matt Falkenhagen]

Created attachment 192677 [details]
Patch

[Hajime Morrita]

Comment on attachment 192677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192677&action=review

> Source/WebCore/dom/Node.h:668
>  

Don't you need to remove isInert() declaration?

[Matt Falkenhagen]

Created attachment 192689 [details]
actually remove isInert

[WebKit Review Bot]

Comment on attachment 192689 [details]
actually remove isInert

Clearing flags on attachment: 192689

Committed r145514: <http://trac.webkit.org/changeset/145514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112134

[Build Bot]

Comment on attachment 192689 [details]
actually remove isInert

Attachment 192689 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17115356

[Build Bot]

Comment on attachment 192689 [details]
actually remove isInert

Attachment 192689 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17183114

[Build Bot]

Comment on attachment 192689 [details]
actually remove isInert

Attachment 192689 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17074335

[Darin Adler]

I still don’t think that “disabled” makes sense for all elements; it’s really a form control element concept, but maybe you could convince me.

[Matt Falkenhagen]

(In reply to comment #10)
> I still don’t think that “disabled” makes sense for all elements; it’s really a form control element concept, but maybe you could convince me.

Ah I thought it might be used for more than form control elements, but I think you're right. If so, the work I intended for future patches to separate disabled from inertness may be simpler than I imagined. I'll give it a try in one patch.

[Matt Falkenhagen]

One issue with moving disabled to HTMLFormControlElement is that HTMLOptGroupElement, HTMLOptionElement, SliderThumbElement, and SpinButtonElement override Node::disabled() and are not HTMLFormControlElements.

I think Element::isEnabledFormControl and Node::disabled can be refactored to be the same function. isEnabledFormControl simply returns !disabled() except for SliderThumbElement and SpinButtonElement, where isEnabledFormControl() of the host is returned.

I thought the two functions might be used to distinguish between readonly and disabled. But it looks redundant.

For <input type="number" disabled>:
SpinButtonElement: disabled is false, isEnabledFormControl is false
SpinButtonElement's shadow host: disabled is true, isEnabledFormControl is false

For <input type="number" readonly>:
SpinButtonElement: disabled is false, isEnabledFormControl is true
SpinButtonElement's shadow host: disabled is false, isEnabledFormControl is true

The readonly case is just the same as the default, enabled case. Unless I've missed a case, there are just two states. So probably we can just use one function, provided that the shadow host of a SpinButtonElement is never a SliderThumbElement or SpinButtonElement, and that there no reason we can't have disabled be true (maybe for event handling?). SliderThumbElement is the same.

I'm not sure the name isEnabledFormControl() is best. It returns true for elements that are not form controls. I think I'll refactor them into a function Element::isDisabledFormControl().

[Matt Falkenhagen]

Here is my plan:

1. Move Node::disabled and Node::isInert to Element.
2. Remove Element::isEnabledFormControl, by calling Element::disabled instead (this is possible after bug 112993).
3. Rename Element::disabled to Element::isDisabledFormControl.

Does that sound OK? I'll do (1) in one patch and (2) and (3) in another patch.

[Matt Falkenhagen]

Created attachment 194629 [details]
Patch

[Hajime Morrita]

Comment on attachment 194629 [details]
Patch

Feels like we could have isDisabledElement(Node*) in Element.h. Having isElementNode() check makes the code a bit noisy.

[Matt Falkenhagen]

(In reply to comment #15)
> (From update of attachment 194629 [details])
> Feels like we could have isDisabledElement(Node*) in Element.h. Having isElementNode() check makes the code a bit noisy.

Thanks. Hm, the problem is that I plan to rename Element::disabled() to Element::isDisabledFormControl() in the next patch, so isDisabledElement(Node*) might be a confusing name. Or maybe it's ok. I'll try it in or after the next patch.

[Hajime Morrita]

(In reply to comment #16)
> Thanks. Hm, the problem is that I plan to rename Element::disabled() to Element::isDisabledFormControl() in the next patch, so isDisabledElement(Node*) might be a confusing name. Or maybe it's ok. I'll try it in or after the next patch.
Okay, isDisabledFormControl(Node*) will be sufficient. 
And making that change is another patch sounds fine to me.

[WebKit Review Bot]

Comment on attachment 194629 [details]
Patch

Clearing flags on attachment: 194629

Committed r146744: <http://trac.webkit.org/changeset/146744>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.