[Microdata] itemtype attribute must update correctly on adding or removing tokens

[Arko Saha]

itemType.add() and itemType.remove() must update the itemtype attribute properly.

Test:

var element = document.createElement('div');
element.itemType.add('foo');
element.itemType.add('FOO');
alert(element.getAttribute('itemtype'));

Expected result: element.getAttribute('itemtype') should return 'foo FOO'
Actual result: returns null

Same behavior can be observed with itemref and itemprop attribute.

[Arko Saha]

Created attachment 182226 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 182226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182226&action=review

> Source/WebCore/html/MicroDataAttributeTokenList.cpp:56
> +void MicroDataAttributeTokenList::setValue(const AtomicString& value)
> +{
> +    m_element->setAttribute(m_attributeName, value);
> +}
> +
> +void MicroDataAttributeTokenList::resetValue(const AtomicString& value)
> +{
> +    DOMSettableTokenList::setValue(value);
> +}

I don't like bi-directionally updated data structures... Why don't we do this inside Node::setItemRef and friends instead?

[Arko Saha]

Created attachment 184341 [details]
Patch2

[Arko Saha]

Let me try to explain the issue,

The actual problem is with adding tokens to itemref attribute.
element.itemRef.add('foo');

In this case setAttribute() method is not getting called for given element. As a result element.getAttribute('itemtype') returns null
Code flow is as below:

0	WebCore::DOMSettableTokenList::setValue	 DOMSettableTokenList.cpp	88	
1	WebCore::DOMTokenList::add	DOMTokenList.cpp	89
2	WebCore::DOMSettableTokenList::add	DOMSettableTokenList.cpp	54		
3	WebCore::jsDOMTokenListPrototypeFunctionAdd	JSDOMTokenList.cpp	271		

It does not call Node::setItemRef(). DOMSettableTokenList::add() is getting called in this case, which internally calls DOMSettableTokenList::setValue to update the tokens and value of DOMSettableTokenList.

In the patch I have override setValue() method in MicroDataAttributeTokenList. So that we can call setAttribute() to update the attribute.
With my change the code flow will be like :

0	WebCore::MicroDataAttributeTokenList::setValue	MicroDataAttributeTokenList.cpp	50	
1	WebCore::DOMTokenList::add	DOMTokenList.cpp	89
2	WebCore::DOMSettableTokenList::add	DOMSettableTokenList.cpp	54		
3	WebCore::jsDOMTokenListPrototypeFunctionAdd	JSDOMTokenList.cpp	271

[Ryosuke Niwa]

Comment on attachment 184341 [details]
Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=184341&action=review

> Source/WebCore/dom/NodeRareData.h:52
> +using namespace HTMLNames;
> +

We don't need this, right?

> Source/WebCore/html/MicroDataAttributeTokenList.cpp:55
> +    if (oldValue != newValue)
> +        m_element->setAttribute(m_attributeName, newValue);

What happens to this token list when someone manually sets the attribute? e.g. element.setAttribute('itemprop', ~);

> Source/WebCore/html/MicroDataAttributeTokenList.h:43
> +typedef int ExceptionCode;

Why do we need this?

[Arko Saha]

(In reply to comment #5)
> > Source/WebCore/dom/NodeRareData.h:52
> > +using namespace HTMLNames;
> > +
> 
> We don't need this, right?

This is required else we need to pass HTMLNames::itempropAttr while creating MicroDataAttributeTokenList as below:

+    DOMSettableTokenList* itemProp(Node* node) const
     {
         if (!m_itemProp)
-            m_itemProp = DOMSettableTokenList::create();
+            m_itemProp = MicroDataAttributeTokenList::create(toElement(node), HTMLNames::itempropAttr);
         return m_itemProp.get();
     }

> > Source/WebCore/html/MicroDataAttributeTokenList.cpp:55
> > +    if (oldValue != newValue)
> > +        m_element->setAttribute(m_attributeName, newValue);
> 
> What happens to this token list when someone manually sets the attribute? e.g. element.setAttribute('itemprop', ~);

Code flow in that case will be like:
0	WebCore::MicroDataAttributeTokenList::setValue	MicroDataAttributeTokenList.cpp	50	0x7fb94d1ebc89	
1	WebCore::Node::setItemProp	Node.cpp	2536	0x7fb94cfb7bbf	
2	WebCore::HTMLElement::parseAttribute	HTMLElement.cpp	233	0x7fb94d16510c	
3	WebCore::Element::attributeChanged	Element.cpp	816	0x7fb94cf6830c	
4	WebCore::StyledElement::attributeChanged	StyledElement.cpp	168	0x7fb94d003235	
5	WebCore::Element::didAddAttribute	Element.cpp	2561	0x7fb94cf6e66a	
6	WebCore::Element::addAttributeInternal	Element.cpp	1785	0x7fb94cf6bc14	
7	WebCore::Element::setAttributeInternal	Element.cpp	771	0x7fb94cf72ab7	
8	WebCore::Element::setAttribute	Element.cpp	749	0x7fb94cf680a5	
9	WebCore::jsElementPrototypeFunctionSetAttribute	JSElement.cpp	1773	0x7fb94da0e83d	

setValue() is called from Element::setAttribute() -> Element::attributeChanged(). So no need to call setAttribute() from MicroDataAttributeTokenList::setValue as attribute value is already updated.
	
> > Source/WebCore/html/MicroDataAttributeTokenList.h:43
> > +typedef int ExceptionCode;
> 
> Why do we need this?

My bad, this is not required.

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebCore/dom/NodeRareData.h:52
> > > +using namespace HTMLNames;
> > > +
> > 
> > We don't need this, right?
> 
> This is required else we need to pass HTMLNames::itempropAttr while creating MicroDataAttributeTokenList as below:
> 
> +    DOMSettableTokenList* itemProp(Node* node) const
>      {
>          if (!m_itemProp)
> -            m_itemProp = DOMSettableTokenList::create();
> +            m_itemProp = MicroDataAttributeTokenList::create(toElement(node), HTMLNames::itempropAttr);
>          return m_itemProp.get();
>      }
> 
> > > Source/WebCore/html/MicroDataAttributeTokenList.cpp:55
> > > +    if (oldValue != newValue)
> > > +        m_element->setAttribute(m_attributeName, newValue);
> > 
> > What happens to this token list when someone manually sets the attribute? e.g. element.setAttribute('itemprop', ~);
> 
> Code flow in that case will be like:
> 0    WebCore::MicroDataAttributeTokenList::setValue    MicroDataAttributeTokenList.cpp    50    0x7fb94d1ebc89    
> 1    WebCore::Node::setItemProp    Node.cpp    2536    0x7fb94cfb7bbf

We might want to add something like setValueFromAttribute or setValueInternal to clarify the semantics here. Otherwise, it look as if we're gonna fall into an infinite loop. Also, it's inefficient to get the string, compare the string, and set it.

[Arko Saha]

Created attachment 184373 [details]
Patch3

Incorporated review comments.

[Arko Saha]

Created attachment 184544 [details]
Patch for landing

Added new files MicroDataAttributeTokenList.{cpp,h} to xcode project.

[WebKit Review Bot]

Comment on attachment 184544 [details]
Patch for landing

Clearing flags on attachment: 184544

Committed r140745: <http://trac.webkit.org/changeset/140745>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.