RESOLVED FIXED 87371
RadioNodeList does not include a object element 
https://bugs.webkit.org/show_bug.cgi?id=87371
Summary RadioNodeList does not include a object element
Rakesh
Reported 2012-05-24 04:44:02 PDT
RadioNodeList does not include a object element even if its id/name attribute is equal to filter criteria.
Attachments
patch (4.42 KB, patch)
2012-05-24 06:33 PDT, Rakesh 		no flags
DetailsFormatted DiffDiff
patch (5.84 KB, patch)
2012-05-25 00:22 PDT, Rakesh 		no flags
DetailsFormatted DiffDiff
patch (5.84 KB, patch)
2012-05-25 00:45 PDT, Rakesh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Rakesh
Comment 1 2012-05-24 06:33:38 PDT
Created attachment 143816 [details] patch
Kent Tamura
Comment 2 2012-05-24 07:36:28 PDT
Comment on attachment 143816 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=143816&action=review > Source/WebCore/html/RadioNodeList.cpp:93 > + if (testElement->hasTagName(objectTag)) > + return checkIfNameOrIdMatches(testElement); Would you add an assertion that ASSERT(m_formElement == static_cast<HTMLObjectElement*>(testElement)->form()); please?
Rakesh
Comment 3 2012-05-24 12:01:57 PDT
(In reply to comment #2) > (From update of attachment 143816 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143816&action=review > Thanks for r+. > > Source/WebCore/html/RadioNodeList.cpp:93 > > + if (testElement->hasTagName(objectTag)) > > + return checkIfNameOrIdMatches(testElement); > > Would you add an assertion that > ASSERT(m_formElement == static_cast<HTMLObjectElement*>(testElement)->form()); > > please? I think we should check this condition in 'if' here as the root node for DynamicSubTreeNodeList is document() and sub tree can contain other object elements whose form owner is not this form. And this is applicable only for form.elements and not fieldset.elements. Please let me know your thoughts on this.
Kent Tamura
Comment 4 2012-05-24 18:02:55 PDT
Comment on attachment 143816 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=143816&action=review >>> Source/WebCore/html/RadioNodeList.cpp:93 >>> + return checkIfNameOrIdMatches(testElement); >> >> Would you add an assertion that >> ASSERT(m_formElement == static_cast<HTMLObjectElement*>(testElement)->form()); >> >> please? > > I think we should check this condition in 'if' here as the root node for DynamicSubTreeNodeList is document() and sub tree can contain other object elements whose form owner is not this form. And this is applicable only for form.elements and not fieldset.elements. > Please let me know your thoughts on this. Oh, I see. I thought testElement came from HTMLFormElement::m_associatedElements. So we should check form(). How about moving the form() check into checkIfNameOrIdMatches(), and rename it to an appropriate name?
Rakesh
Comment 5 2012-05-25 00:22:22 PDT
Created attachment 143990 [details] patch Added form check for object element also.
Kent Tamura
Comment 6 2012-05-25 00:27:28 PDT
Comment on attachment 143990 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=143990&action=review > Source/WebCore/html/RadioNodeList.cpp:103 > + if (!testElement->hasTagName(objectTag)) { > + if (!testElement->isFormControlElement()) nit: this can be if (!testElement->hasTagName(objectTag) && !testElement->isFormControlElement()) return false;
Rakesh
Comment 7 2012-05-25 00:45:33 PDT
Created attachment 143994 [details] patch Patch for landing.
WebKit Review Bot
Comment 8 2012-05-25 01:51:22 PDT
Comment on attachment 143994 [details] patch Clearing flags on attachment: 143994 Committed r118495: <http://trac.webkit.org/changeset/118495>
WebKit Review Bot
Comment 9 2012-05-25 01:51:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.