Bug 93717 - Microdata item with itemprop attribute should not include the item itself in the HTMLPropertiesCollection
Summary: Microdata item with itemprop attribute should not include the item itself in ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arko Saha
Depends on:
Blocks: 92986
  Show dependency treegraph
Reported: 2012-08-10 06:18 PDT by Arko Saha
Modified: 2012-08-10 19:33 PDT (History)
2 users (show)

See Also:

Patch (12.70 KB, patch)
2012-08-10 06:29 PDT, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (12.37 KB, patch)
2012-08-10 16:59 PDT, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.

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]
Comment 2 Ryosuke Niwa 2012-08-10 12:08:28 PDT
Comment on attachment 157728 [details]

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.