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

Description Arko Saha 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.
Comment 1 Arko Saha 2012-08-10 06:29:35 PDT
Created attachment 157728 [details]
Patch
Comment 2 Ryosuke Niwa 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.
Comment 3 Arko Saha 2012-08-10 16:59:28 PDT
Created attachment 157838 [details]
Updated patch

Incorporated review comments.
Comment 4 WebKit Review Bot 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>
Comment 5 WebKit Review Bot 2012-08-10 19:33:35 PDT
All reviewed patches have been landed.  Closing bug.