Bug 112085

Created attachment 192677 [details] Patch

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?

Created attachment 192689 [details] actually remove isInert

Comment on attachment 192689 [details] actually remove isInert Clearing flags on attachment: 192689 Committed r145514: <http://trac.webkit.org/changeset/145514>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 112134

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

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

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

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.

(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.

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().

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.

Created attachment 194629 [details] Patch

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.

(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.

(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.

Comment on attachment 194629 [details] Patch Clearing flags on attachment: 194629 Committed r146744: <http://trac.webkit.org/changeset/146744>

All reviewed patches have been landed. Closing bug.