Implement element type selectors for the WebVTT ::cue pseudo class

• Component: Media
• Status: RESOLVED FIXED

[Dima Gorbik]

Element type selector matching should be implemented for such WebVTT node classes like ruby, rt, b, v and others. Full list: http://dev.w3.org/html5/webvtt/#the-'::cue'-pseudo-element

[Radar WebKit Bug Importer]

<rdar://problem/12914597>

[Dima Gorbik]

Created attachment 180441 [details]
Proposed fix 0.1

I suggest moving tag comparison to the overloaded "==" operator of a QualifiedName. That would simplify SelectorChecker::tagMatches quite a bit.
All general tag names like i, b, ruby and rt are being already handled. This patch adds the support for rare cases with "c" and "v" tags.
I will add layout tests after the previous patch is being reviewed (https://bugs.webkit.org/show_bug.cgi?id=105473) because they will be based on the existing layout tests from that patch.

[Dima Gorbik]

Created attachment 181422 [details]
Proposed fix 0.2

[WebKit Review Bot]

Comment on attachment 181422 [details]
Proposed fix 0.2

Attachment 181422 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15701587

New failing tests:
media/track/track-css-matching.html

[Antti Koivisto]

Comment on attachment 181422 [details]
Proposed fix 0.2

This is hacky. Also tagMatches() is hot and we don't want to add branches there. I think you should make the WebVTT shadow nodes really have these tag names (c, v), and add the UA CSS rules to style them.

[Dima Gorbik]

Created attachment 181578 [details]
Proposed fix 0.3

[WebKit Review Bot]

Comment on attachment 181578 [details]
Proposed fix 0.3

Attachment 181578 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15744753

New failing tests:
media/track/track-css-matching.html
media/track/track-webvtt-tc025-class-markup.html
media/track/track-webvtt-tc026-voice.html

[Dima Gorbik]

(In reply to comment #5)
> (From update of attachment 181422 [details])
> This is hacky. Also tagMatches() is hot and we don't want to add branches there. I think you should make the WebVTT shadow nodes really have these tag names (c, v), and add the UA CSS rules to style them.

WebVTT standard requires those nodes to be qTags and spans when they are being converted into DOM nodes. (that's why tests are failing). Do you suggest replacing those with "c" and "v" tagNames  in markPastFutureNodes (which already does some sorts of conversion for building rendering element?

[Dima Gorbik]

Also do you want me to make a v tag copy qTag behavior?

[Antti Koivisto]

If I'm reading the spec right, the cue rendering section does not reference "WebVTT cue text DOM construction rules" section at all. Those are only ever used when getCueAsHTML() API is invoked and that is the only time where <span> etc. are required being generated.

In other words it sounds to me like updateDisplayTree() and getCueAsHTML() paths should be separated with only the latter doing the "DOM construction rules" mappings.

[Antti Koivisto]

It seems to me that what we want is:

- Make WebVTTParser generate a tree of Elements corresponding to "WebVTT Node Objects" with tag names and attributes set for selector matching (http://dev.w3.org/html5/webvtt/#the-'::cue'-pseudo-element).
- getCueAsHTML() should not be used internally. It should be invoked on API access only and traverse the internal tree, constructing a new tree with mappings described in http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules
- I see no obvious need to make a copy of the node tree for rendering. It looks like we could just use it as-is, simplifying things (eliminate m_allDocumentNodes).
- Instead of marking the tree with bits, :future and :past should be implemented in SelectorChecker by grabbing the media elements time stamp and walking to the nearest timestamp node.

What do you think?

[Dima Gorbik]

> It seems to me that what we want is:
> 
> - Make WebVTTParser generate a tree of Elements corresponding to "WebVTT Node Objects" with tag names and attributes set for selector matching (http://dev.w3.org/html5/webvtt/#the-'::cue'-pseudo-element).
> - getCueAsHTML() should not be used internally. It should be invoked on API access only and traverse the internal tree, constructing a new tree with mappings described in http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules
> - I see no obvious need to make a copy of the node tree for rendering. It looks like we could just use it as-is, simplifying things (eliminate m_allDocumentNodes).
> - Instead of marking the tree with bits, :future and :past should be implemented in SelectorChecker by grabbing the media elements time stamp and walking to the nearest timestamp node.
> 
> What do you think?

These are good ideas. But it may be hard to build a DOM tree from the rendering tree in future, specifications require some nodes (for example ruby) be wrapped in anonymous boxes, and I suspect future implementations of the webvtt style word wrapping will introduce even more changes. It may be much easier making a rendering tree from the DOM tree but going backwards may be an overcomplication.
For now I suggest separating getCueAsHTML from the one that is used internally. API access function will map those new "v" and "c" tags to spans and qtags. We will still have 3 trees in this case since we will be still copying the rendering tree. Probably that's not that bad because it will be easy to make rendering and DOM trees from the tree that's produced by the parser. 
As for marking the tree with bits, I think we will still have to mark nodes as webvtt nodes. And presetting those bits may speed up things a little bit. (the only case when it's slower — when we don't have any cue styling rules at all).
Does this sound reasonable?

[Antti Koivisto]

(In reply to comment #12)
> As for marking the tree with bits, I think we will still have to mark nodes as webvtt nodes. And presetting those bits may speed up things a little bit. (the only case when it's slower — when we don't have any cue styling rules at all).

I doubt there is significant performance difference. Walking the internal tree to mark time is not that fast either and is not very nice architecturally. I'm not sure if there is really need to mark WebVTT nodes at all, everything SelectorChecker sees after matching cue() is going to be WebVTT node (it could just be part of SelectorCheckingContext).

> Does this sound reasonable?

Yeah.

[Dima Gorbik]

Created attachment 181838 [details]
Proposed fix 0.4

[Dima Gorbik]

Created attachment 181839 [details]
Proposed fix 0.5

[WebKit Review Bot]

Comment on attachment 181839 [details]
Proposed fix 0.5

Attachment 181839 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15757498

New failing tests:
media/track/track-css-matching.html

[WebKit Review Bot]

Comment on attachment 181839 [details]
Proposed fix 0.5

Attachment 181839 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15762516

New failing tests:
media/track/track-css-matching.html

[Dima Gorbik]

Created attachment 181974 [details]
Proposd fix 0.6

Now seeking to specific values in the layout test.

[Antti Koivisto]

Comment on attachment 181974 [details]
Proposd fix 0.6

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

> LayoutTests/media/track/captions-webvtt/styling.vtt:19
>  \ No newline at end of file

You could fix that.

> Source/WebCore/html/HTMLQuoteElement.cpp:37
>  inline HTMLQuoteElement::HTMLQuoteElement(const QualifiedName& tagName, Document* document)
>      : HTMLElement(tagName, document)
>  {
> -    ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag));
> +    ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag) || hasTagName(vTag));

This seems unnecessary. You never construct HTMLQuoteElement with vTag. Is this just because you want to use cloneNode() for making the rendering tree?

> Source/WebCore/html/HTMLTagNames.in:26
> +c interfaceName=HTMLElement

This is bit misleading as these are not really HTML names (we don't expose HTML elements with these names) and so should not be in HTMLTagNames. I think you should just construct and expose QualifiedNames for them manually in TextTrackCue.h/cpp (see anyName in QualifiedName.h for example).

> Source/WebCore/html/HTMLTagNames.in:139
> +v interfaceName=HTMLQuoteElement

Same thing.

> Source/WebCore/html/track/TextTrackCue.cpp:486
>          m_documentFragment = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);

m_documentFragment should be called something more sensible like m_webVTTNodeTree.

> Source/WebCore/html/track/TextTrackCue.cpp:490
> -
>          if (!m_documentFragment)
> -          return 0;
> +            return;
> +    }
> +}

This return branch looks unnecessary.

> Source/WebCore/html/track/TextTrackCue.cpp:492
> +void TextTrackCue::copyWebVTTNodeToDOMTree(PassRefPtr<Node> WebVTTNode, PassRefPtr<Node> root)

'root' is not root here. The variable should be called 'parent'. It could also be tightened to be ContainerNode.

PassRefPtr's are only needed when transferring ownership. Please use plain pointers.

In WebKit coding style, variable names start with lowercase letter (webVTTNode).

This could be a standalone function if you passed in the Document*.

> Source/WebCore/html/track/TextTrackCue.cpp:496
> +    for (Node* n = WebVTTNode->firstChild(); n && !ec; n = n->nextSibling()) {

Please use complete words are variable names, n -> node.

> Source/WebCore/html/track/TextTrackCue.cpp:499
> +        if (n->isElementNode() && toElement(n)->localName() == HTMLNames::cTag.localName())

Test with node->hasTagName(vTag), no need of element test or toElement.

> Source/WebCore/html/track/TextTrackCue.cpp:500
> +            clonedNode = HTMLElement::create(spanTag, static_cast<Document*>(m_scriptExecutionContext));

You should construct correct subclasses, HTMLSpanElement here...

Please save Document* to a variable for resuse.

> Source/WebCore/html/track/TextTrackCue.cpp:501
> +        else if (n->isElementNode() && toElement(n)->localName() == HTMLNames::vTag.localName())

hasTagName

> Source/WebCore/html/track/TextTrackCue.cpp:502
> +            clonedNode = HTMLElement::create(qTag, static_cast<Document*>(m_scriptExecutionContext));

... HTMLQuoteElement here.

> Source/WebCore/html/track/TextTrackCue.cpp:509
> +        if (!clonedNode)
> +            clonedNode = n->cloneNode(false);
> +        else {
> +            toElement(clonedNode.get())->setAttribute(classAttr, toElement(n)->getAttribute(classAttr));
> +            toElement(clonedNode.get())->setAttribute(titleAttr, toElement(n)->getAttribute(titleAttr));
>          }

With this class and title attributes are only getting set for elements created from v and q nodes. This seems wrong as there are other node types mapping to Elements.

> Source/WebCore/html/track/TextTrackCue.cpp:510
> +        root->appendChild(clonedNode, ec);

This should never fail. Get rid of the ec variable, do appendChild(clonedNode, ASSERT_NO_EXCEPTION) instead.

> Source/WebCore/html/track/TextTrackCue.cpp:511
> +        copyWebVTTNodeToDOMTree(n, clonedNode);

You shouldn't recurse if the node is not a ContainerNode.

> Source/WebCore/html/track/TextTrackCue.cpp:530
> +PassRefPtr<DocumentFragment> TextTrackCue::getCueRenderingTree()
> +{

We don't use get* naming like this, createCueRenderingTree.

> Source/WebCore/html/track/TextTrackCue.cpp:537
> +    Document* document;
> +    RefPtr<DocumentFragment> clonedFragment;
> +    
> +    createWebVTTNodeTree();
> +    document = static_cast<Document*>(m_scriptExecutionContext);
> +    
> +    clonedFragment = DocumentFragment::create(document);

Please define the variable and set its value on a single line.

> Source/WebCore/html/track/TextTrackCue.cpp:-745
> -    if (m_hasInnerTimestamps)
> -        updateDisplayTree(track()->mediaElement()->currentTime());

Why don't we need to call updateDisplayTree anymore?

[Dima Gorbik]

(In reply to comment #19)
> > Source/WebCore/html/HTMLQuoteElement.cpp:37
> >  inline HTMLQuoteElement::HTMLQuoteElement(const QualifiedName& tagName, Document* document)
> >      : HTMLElement(tagName, document)
> >  {
> > -    ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag));
> > +    ASSERT(hasTagName(qTag) || hasTagName(blockquoteTag) || hasTagName(vTag));
> 
> This seems unnecessary. You never construct HTMLQuoteElement with vTag. Is this just because you want to use cloneNode() for making the rendering tree?
> 
This is for cloneNode() to construct the right element. Also I think this should be for consistency.

> > Source/WebCore/html/track/TextTrackCue.cpp:486
> >          m_documentFragment = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);
> 
> m_documentFragment should be called something more sensible like m_webVTTNodeTree.

I agree. This is an old code. Do you want me to refactor this in this patch?

> Please use complete words are variable names, n -> node.
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:499
> > +        if (n->isElementNode() && toElement(n)->localName() == HTMLNames::cTag.localName())
> 

This is how it is in the cloning code.

> > Source/WebCore/html/track/TextTrackCue.cpp:500
> > +            clonedNode = HTMLElement::create(spanTag, static_cast<Document*>(m_scriptExecutionContext));
> 
> You should construct correct subclasses, HTMLSpanElement here...

This is how this node is constructed in the WebVTT parser. Should it be changed there as well?

> > Source/WebCore/html/track/TextTrackCue.cpp:509
> > +        if (!clonedNode)
> > +            clonedNode = n->cloneNode(false);
> > +        else {
> > +            toElement(clonedNode.get())->setAttribute(classAttr, toElement(n)->getAttribute(classAttr));
> > +            toElement(clonedNode.get())->setAttribute(titleAttr, toElement(n)->getAttribute(titleAttr));
> >          }
> 
> With this class and title attributes are only getting set for elements created from v and q nodes. This seems wrong as there are other node types mapping to Elements.

Why should I do this for other node types if I use cloneNode function that copies all the attributes automatically?

> > Source/WebCore/html/track/TextTrackCue.cpp:511
> > +        copyWebVTTNodeToDOMTree(n, clonedNode);
> 
> You shouldn't recurse if the node is not a ContainerNode.

Is this because it's unsafe to call firstChild for non ContainerNodes?

> > Source/WebCore/html/track/TextTrackCue.cpp:-745
> > -    if (m_hasInnerTimestamps)
> > -        updateDisplayTree(track()->mediaElement()->currentTime());
> 
> Why don't we need to call updateDisplayTree anymore?

The tree doesn't change anymore, since I removed past/future containers in the previous patch. All nodes stay on their places throughout the cue lifetime.

Thanks for a review!

[Dima Gorbik]

Antti, it looks like HTMLElementFactory is not used when HTMLElement::create is called. This means that the old code in the parser didn't create elements of the right type, or at least relied on the cloning which uses HTMLElementFactory::createHTMLElement and that actually calls the right constructor. What's better — to add vTag manually to the HTMLElementFactory function table so it would create the right element when cloning, or remove this call to the core cloning code and do it with my custom cloning function and just call the right constructor for this element?

[Dima Gorbik]

I also just released that my clone function will not create elements of the right types for other than v and c elements. And doing a switch is ugly. 
Probably we could add some option for HTMLTagNames.in so that we wouldn't create externally accessible  API's for those tags?

[Dima Gorbik]

Created attachment 182482 [details]
Proposed fix 0.7

[Dima Gorbik]

Created attachment 182483 [details]
Proposed fix 0.8

[Dima Gorbik]

Created attachment 182484 [details]
Proposed fix 0.9

[Eric Carlson]

Comment on attachment 182484 [details]
Proposed fix 0.9

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

> Source/WebCore/ChangeLog:9
> +        handled without any changes to the code. Creating a rendering tree and DOM tree nowuse different code paths. 

Typo: "nowuse" -> "now use"

[Dima Gorbik]

Created attachment 182494 [details]
Proposed fix 1.0

[Antti Koivisto]

(In reply to comment #21)
> Antti, it looks like HTMLElementFactory is not used when HTMLElement::create is called. This means that the old code in the parser didn't create elements of the right type, or at least relied on the cloning which uses HTMLElementFactory::createHTMLElement and that actually calls the right constructor. What's better — to add vTag manually to the HTMLElementFactory function table so it would create the right element when cloning, or remove this call to the core cloning code and do it with my custom cloning function and just call the right constructor for this element?

The internal node tree does not really need to be HTMLElements as it doesn't need HTML semantics. That some WebVTT tags have the same names as HTML tags is good for remembering them but does not make them HTML tags. Rendering and selector matching code works fine with generic Elements and it would probably be clearer to create internal nodes with Element::create(). It doesn't make much practical difference though.

On the other hand it is important that the tree exposed by getCueAsHTML() is correctly constructed from HTML elements.

As I mentioned above, as long as the rendering tree and the node tree are identical you can avoid the whole cloning question by just having one tree.

[Antti Koivisto]

(In reply to comment #22)
> I also just released that my clone function will not create elements of the right types for other than v and c elements. And doing a switch is ugly. 
> Probably we could add some option for HTMLTagNames.in so that we wouldn't create externally accessible  API's for those tags?

There is nothing particularly ugly about doing it in the switch. Matches well with the spec language.

[Dima Gorbik]

(In reply to comment #28)
> As I mentioned above, as long as the rendering tree and the node tree are identical you can avoid the whole cloning question by just having one tree.

Sounds reasonable, but specs already have some requirements to add several wrappers for webvtt nodes that shouldn't be in DOM. I think we shouldn't use the same tree now because if we do we will still have to revert it back to cloning pretty soon.

[Antti Koivisto]

(In reply to comment #30)
> Sounds reasonable, but specs already have some requirements to add several wrappers for webvtt nodes that shouldn't be in DOM. I think we shouldn't use the same tree now because if we do we will still have to revert it back to cloning pretty soon.

If you mean this 

"Runs of children of WebVTT Ruby Objects that are not WebVTT Ruby Text Objects must be wrapped in anonymous boxes whose 'display' property has the value 'ruby-base'."

it doesn't sound to me like this implies any wrappers in the webvtt node tree. The anonymous boxes are in rendering level (and possibly handled already, see comments in RenderRuby.h)

[Dima Gorbik]

(In reply to comment #31)
> (In reply to comment #30)
> > Sounds reasonable, but specs already have some requirements to add several wrappers for webvtt nodes that shouldn't be in DOM. I think we shouldn't use the same tree now because if we do we will still have to revert it back to cloning pretty soon.
> 
> If you mean this 
> 
> "Runs of children of WebVTT Ruby Objects that are not WebVTT Ruby Text Objects must be wrapped in anonymous boxes whose 'display' property has the value 'ruby-base'."
> 
> it doesn't sound to me like this implies any wrappers in the webvtt node tree. The anonymous boxes are in rendering level (and possibly handled already, see comments in RenderRuby.h)

There are also some requirements for the text wrapping. I guess they will be hard to comply with without modifying the tree. 
Could you please review the last patch please?

[Dima Gorbik]

(In reply to comment #31)
> it doesn't sound to me like this implies any wrappers in the webvtt node tree. The anonymous boxes are in rendering level (and possibly handled already, see comments in RenderRuby.h)

That just may be complicated to remove those additional wrappers to make a DOM tree from the WebVTT Node tree if we make it equal to a rendering tree.

[WebKit Review Bot]

Comment on attachment 182494 [details]
Proposed fix 1.0

Clearing flags on attachment: 182494

Committed r139692: <http://trac.webkit.org/changeset/139692>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.