Summary: | [Forms] The "meter" element should not be a form-associated element. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ojan, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 80392 | ||||||||||||
Bug Blocks: | 80240 | ||||||||||||
Attachments: |
|
Description
yosin
2012-03-05 22:21:43 PST
Created attachment 131778 [details]
Patch 1
Patch 1 contains changes for HTMLMeterElement itself and HTMLLabelElement for supporting LabelabelElement instead of HTMLFormControlElement. This HTMLLabelElement changes is also required to change base class of the "progress" element to LabelableElement. Comment on attachment 131778 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=131778&action=review > Source/WebCore/html/HTMLElement.h:105 > + virtual bool isLabelable() const { return false; } The function means: * This HTMLElement can be casted to LabelableElement, AND * This LabelableElement actually supports "labels" property. isClassName() functions of HTMLElement and Element usually have only the first role. isLabelable() in this patch will make other developers confused. I recommend to rename the existing LabelableElement::isLabelable() to LabelableElement::supportLabels(), and restrict the role of HTMLElement::isLabelable(). Created attachment 131805 [details]
Patch 2
Comment on attachment 131805 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=131805&action=review > Source/WebCore/html/LabelableElement.h:50 > + virtual bool isLabelable() const OVERRIDE { return true; } This should be "isLabelable() const OVERRIDE FINAL { return true; }" because subclasses must not override this. > LayoutTests/ChangeLog:14 > + * fast/dom/HTMLMeterElement/meter-element-form-expected.txt: Removed. > + * fast/dom/HTMLMeterElement/meter-element-form.html: Removed. Please do not remove the test. We should change it so that it confirms HTMLMeterElement doesn't support "form" IDL attribute. Created attachment 131819 [details]
Patch 3
Comment on attachment 131819 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=131819&action=review > LayoutTests/fast/dom/HTMLMeterElement/meter-element-form.html:16 > +if (document.getElementById('meter1').form == null) Should it be undefined, not null, because HTMLMeterElement doesn't have form IDL attribute? Created attachment 131972 [details]
Patch 4
(In reply to comment #7) > (From update of attachment 131819 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131819&action=review > > > LayoutTests/fast/dom/HTMLMeterElement/meter-element-form.html:16 > > +if (document.getElementById('meter1').form == null) > > Should it be undefined, not null, because HTMLMeterElement doesn't have form IDL attribute? You're right. I forgot to update IDL file which contains the "form" attribute of the "meter" lement. Comment on attachment 131972 [details]
Patch 4
ok
Comment on attachment 131972 [details] Patch 4 Clearing flags on attachment: 131972 Committed r110927: <http://trac.webkit.org/changeset/110927> All reviewed patches have been landed. Closing bug. |