Bug 93717

Summary: Microdata item with itemprop attribute should not include the item itself in the HTMLPropertiesCollection
Product: WebKit Reporter: Arko Saha <arko>
Component: DOMAssignee: Arko Saha <arko>
Status: RESOLVED FIXED    
Severity: Normal CC: rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 92986    
Attachments:
Description Flags
Patch
none
Updated patch none

Arko Saha
Reported 2012-08-10 06:18:10 PDT
Currently we add microdata item in its property collection if item has an itemprop attribute specified. Test: var testEl = makeEl('div',{itemscope:'itemscope'},'<div itemscope itemprop="foo"><div itemprop="bar"></div></div>'); var item = testEl.firstChild; assert_equals( item.properties.length, 1, 'properties length') Expected: PASS (properties length must be 1) Actual: FAIL- properties length expected 1 but got 2 (foo and bar) item (inner div) has itemprop attribute specified with value 'foo'. It has only one property 'bar'. So, item.propeties.length must return 1.
Attachments
Patch (12.70 KB, patch)
2012-08-10 06:29 PDT, Arko Saha 		no flags
DetailsFormatted DiffDiff
Updated patch (12.37 KB, patch)
2012-08-10 16:59 PDT, Arko Saha 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Arko Saha
Comment 1 2012-08-10 06:29:35 PDT
Created attachment 157728 [details] Patch
Ryosuke Niwa
Comment 2 2012-08-10 12:08:28 PDT
Comment on attachment 157728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157728&action=review > Source/WebCore/dom/PropertyNodeList.cpp:86 > - if (testElement == m_itemRefElementsCache[i] || elementIsPropertyOfRefElement(testElement, m_itemRefElementsCache[i])) { > + Node* refElement = m_itemRefElementsCache[i]; > + if (testElement == refElement || elementIsPropertyOfRefElement(testElement, refElement)) { Why are we making this change? > Source/WebCore/html/HTMLPropertiesCollection.cpp:96 > + Node* ownerNode = this->base(); Can we call ownerNode() instead here so that there is no confusion about base vs. ownerNode? > Source/WebCore/html/HTMLPropertiesCollection.cpp:99 > + for (; current; current = nextNodeWithProperty(base, current, ownerNode)) { We should probably rename base to rootNode here. It's too confusing. (make sure to update terms in nextNodeWithProperty.
Arko Saha
Comment 3 2012-08-10 16:59:28 PDT
Created attachment 157838 [details] Updated patch Incorporated review comments.
WebKit Review Bot
Comment 4 2012-08-10 19:33:31 PDT
Comment on attachment 157838 [details] Updated patch Clearing flags on attachment: 157838 Committed r125348: <http://trac.webkit.org/changeset/125348>
WebKit Review Bot
Comment 5 2012-08-10 19:33:35 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.