Bug 105480 - Implement element type selectors for the WebVTT ::cue pseudo class
Summary: Implement element type selectors for the WebVTT ::cue pseudo class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 43668 105481
  Show dependency treegraph
 
Reported: 2012-12-19 16:51 PST by Dima Gorbik
Modified: 2013-01-14 17:18 PST (History)
14 users (show)

See Also:


Attachments
Proposed fix 0.1 (1.19 KB, patch)
2012-12-20 16:41 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Proposed fix 0.2 (8.46 KB, patch)
2013-01-04 19:26 PST, Dima Gorbik
koivisto: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.3 (9.38 KB, patch)
2013-01-07 15:39 PST, Dima Gorbik
dgorbik: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.4 (16.92 KB, patch)
2013-01-08 21:11 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Proposed fix 0.5 (16.74 KB, patch)
2013-01-08 21:25 PST, Dima Gorbik
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposd fix 0.6 (16.76 KB, patch)
2013-01-09 13:25 PST, Dima Gorbik
koivisto: review-
koivisto: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.7 (20.98 KB, patch)
2013-01-13 04:17 PST, Dima Gorbik
dgorbik: review-
dgorbik: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.8 (21.00 KB, patch)
2013-01-13 04:39 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Proposed fix 0.9 (20.91 KB, patch)
2013-01-13 04:48 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Proposed fix 1.0 (20.91 KB, patch)
2013-01-13 16:23 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Gorbik 2012-12-19 16:51:14 PST
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
Comment 1 Radar WebKit Bug Importer 2012-12-19 16:52:25 PST
<rdar://problem/12914597>
Comment 2 Dima Gorbik 2012-12-20 16:41:01 PST
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.
Comment 3 Dima Gorbik 2013-01-04 19:26:33 PST
Created attachment 181422 [details]
Proposed fix 0.2
Comment 4 WebKit Review Bot 2013-01-04 22:16:36 PST
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
Comment 5 Antti Koivisto 2013-01-05 01:48:26 PST
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.
Comment 6 Dima Gorbik 2013-01-07 15:39:21 PST
Created attachment 181578 [details]
Proposed fix 0.3
Comment 7 WebKit Review Bot 2013-01-07 17:20:23 PST
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
Comment 8 Dima Gorbik 2013-01-07 18:29:38 PST
(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?
Comment 9 Dima Gorbik 2013-01-07 18:32:11 PST
Also do you want me to make a v tag copy qTag behavior?
Comment 10 Antti Koivisto 2013-01-08 02:34:23 PST
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.
Comment 11 Antti Koivisto 2013-01-08 05:15:02 PST
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?
Comment 12 Dima Gorbik 2013-01-08 13:20:05 PST
> 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?
Comment 13 Antti Koivisto 2013-01-08 14:02:54 PST
(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.
Comment 14 Dima Gorbik 2013-01-08 21:11:34 PST
Created attachment 181838 [details]
Proposed fix 0.4
Comment 15 Dima Gorbik 2013-01-08 21:25:14 PST
Created attachment 181839 [details]
Proposed fix 0.5
Comment 16 WebKit Review Bot 2013-01-09 06:05:14 PST
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
Comment 17 WebKit Review Bot 2013-01-09 07:22:25 PST
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
Comment 18 Dima Gorbik 2013-01-09 13:25:53 PST
Created attachment 181974 [details]
Proposd fix 0.6

Now seeking to specific values in the layout test.
Comment 19 Antti Koivisto 2013-01-09 14:20:02 PST
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?
Comment 20 Dima Gorbik 2013-01-09 14:42:29 PST
(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!
Comment 21 Dima Gorbik 2013-01-09 20:47:39 PST
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?
Comment 22 Dima Gorbik 2013-01-09 21:16:59 PST
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?
Comment 23 Dima Gorbik 2013-01-13 04:17:25 PST
Created attachment 182482 [details]
Proposed fix 0.7
Comment 24 Dima Gorbik 2013-01-13 04:39:11 PST
Created attachment 182483 [details]
Proposed fix 0.8
Comment 25 Dima Gorbik 2013-01-13 04:48:35 PST
Created attachment 182484 [details]
Proposed fix 0.9
Comment 26 Eric Carlson 2013-01-13 09:56:37 PST
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"
Comment 27 Dima Gorbik 2013-01-13 16:23:47 PST
Created attachment 182494 [details]
Proposed fix 1.0
Comment 28 Antti Koivisto 2013-01-14 12:38:25 PST
(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.
Comment 29 Antti Koivisto 2013-01-14 12:39:27 PST
(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.
Comment 30 Dima Gorbik 2013-01-14 14:34:20 PST
(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.
Comment 31 Antti Koivisto 2013-01-14 15:51:44 PST
(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)
Comment 32 Dima Gorbik 2013-01-14 15:54:49 PST
(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?
Comment 33 Dima Gorbik 2013-01-14 16:45:24 PST
(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.
Comment 34 WebKit Review Bot 2013-01-14 17:18:43 PST
Comment on attachment 182494 [details]
Proposed fix 1.0

Clearing flags on attachment: 182494

Committed r139692: <http://trac.webkit.org/changeset/139692>
Comment 35 WebKit Review Bot 2013-01-14 17:18:49 PST
All reviewed patches have been landed.  Closing bug.