Bug 112085 - Refactoring: Pull Node::disabled() and Node::isInert() down to Element.
Summary: Refactoring: Pull Node::disabled() and Node::isInert() down to Element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on: 112134 112767 112993
Blocks: 84635
  Show dependency treegraph
 
Reported: 2013-03-11 18:00 PDT by Hajime Morrita
Modified: 2013-03-24 22:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.47 KB, patch)
2013-03-12 02:22 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
actually remove isInert (8.76 KB, patch)
2013-03-12 02:57 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (9.48 KB, patch)
2013-03-22 14:15 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2013-03-11 18:00:21 PDT
Suggested at https://bugs.webkit.org/show_bug.cgi?id=110952#c16.
Comment 1 Matt Falkenhagen 2013-03-12 02:22:41 PDT
Created attachment 192677 [details]
Patch
Comment 2 Hajime Morrita 2013-03-12 02:46:08 PDT
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?
Comment 3 Matt Falkenhagen 2013-03-12 02:57:43 PDT
Created attachment 192689 [details]
actually remove isInert
Comment 4 WebKit Review Bot 2013-03-12 04:15:15 PDT
Comment on attachment 192689 [details]
actually remove isInert

Clearing flags on attachment: 192689

Committed r145514: <http://trac.webkit.org/changeset/145514>
Comment 5 WebKit Review Bot 2013-03-12 04:15:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2013-03-12 04:37:42 PDT
Re-opened since this is blocked by bug 112134
Comment 7 Build Bot 2013-03-12 11:40:48 PDT
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 8 Build Bot 2013-03-12 12:41:34 PDT
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 9 Build Bot 2013-03-12 12:41:56 PDT
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
Comment 10 Darin Adler 2013-03-12 19:20:59 PDT
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.
Comment 11 Matt Falkenhagen 2013-03-12 22:20:06 PDT
(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.
Comment 12 Matt Falkenhagen 2013-03-21 16:17:21 PDT
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().
Comment 13 Matt Falkenhagen 2013-03-22 12:12:56 PDT
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.
Comment 14 Matt Falkenhagen 2013-03-22 14:15:13 PDT
Created attachment 194629 [details]
Patch
Comment 15 Hajime Morrita 2013-03-24 18:43:28 PDT
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.
Comment 16 Matt Falkenhagen 2013-03-24 19:58:14 PDT
(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.
Comment 17 Hajime Morrita 2013-03-24 20:16:33 PDT
(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 18 WebKit Review Bot 2013-03-24 22:01:07 PDT
Comment on attachment 194629 [details]
Patch

Clearing flags on attachment: 194629

Committed r146744: <http://trac.webkit.org/changeset/146744>
Comment 19 WebKit Review Bot 2013-03-24 22:01:12 PDT
All reviewed patches have been landed.  Closing bug.