[Webkit-unassigned] [Bug 79746] Main rendering steps for displaying the cue boxes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79746





--- Comment #4 from Victor Carbune <vcarbune at adobe.com>  2012-03-07 15:16:12 PST ---
(In reply to comment #2)
> (From update of attachment 129769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129769&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        * css/mediaControls.css:
> > +        (video::-webkit-media-text-track-display): Adjusted text color.
> 
> Please say why the color was changed.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1203
> > +    if (!toParentMediaElement(this)->isVideo())
> > +        return;
> > +
> > +    // 2. Let video be the media element or other playback mechanism.
> > +    HTMLVideoElement* video = static_cast<HTMLVideoElement*>(toParentMediaElement(this));
> 
> Nit: you can avoid one of the calls to toParentMediaElement() by changing this slightly.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1214
> > +    // FIXME (should be copying the media controls box into a transparent box):
> > +    if (video->controls())
> > +        output.append(video->mediaControls());
> 
> Could you achieve the desired effect, keeping the captions above the default controls, by putting the controls and captions in the same box with appropriate alignment (controls always at the bottom, etc)? This way, we could animate the containing box when the controls visibility is changed and the captions and controls will move in sync.

Indeed, I think it's a much better approach. The media-controls now has display:-webkit-box such that the two boxes (track container and controls panel) are automatically positioned.

The only drawback of using -webkit-box is that the captions don't transition to the bottom (but I think it's cleaner to use -webkit-box, rather than manually extending the size of the container and so on).

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1259
> > +        if (cue->getDisplayState().size()) {
> > +            ExceptionCode ec;
> > +            appendChild(cue->getDisplayState()[0], ec, true);
> > +
> 
> Calling getDisplayState() twice is unnecessary, you might as well put it into a local.
> 
> Why is only the first element in the vector used?
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:409
> > +Vector<RefPtr<HTMLDivElement> > TextTrackCue::getDisplayState()
> 
> I don't think this is a great name for this method, I would never guess that "getDisplayState" returns the display element(s). Maybe "getDisplayNodes()", or "getDisplayTree()", or ...
> 
> I don't understand why this returns a Vector of elements. Couldn't it return a single root node with however many child nodes it needs to display the cue?
I read the spec again more closely - I initially thought that re-positioning is done for each individual box (children of the root node), but it seems the whole box itself is moved.

I have changed the name to getDisplayTree and returns a single root node.

> > Source/WebCore/html/track/TextTrackCue.cpp:412
> > +    if (m_displayBoxes.size() || !m_scriptExecutionContext->isDocument())
> > +        return m_displayBoxes;
> 
> Don't you need to clear m_displayBoxes in TextTrackCue::cueDidChange() so we update when a cue is modified?
Marked a boolean variable false.

> > Source/WebCore/html/track/TextTrackCue.cpp:425
> > +    // The document tree is the tree of WebVTT Node Objects rooted at nodes.
> > +    simpleBox->appendChild(m_documentFragment, ASSERT_NO_EXCEPTION, true);
> 
> Won't this allow a arbitrary markup in a cue? What does tc028-unsupported-markup.vtt look like with this?
No, this doesn't happen - the WebVTT spec 3.4 is what gets in the fragment (so anything not appropriate is stripped out):
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-dom-construction-rules

This is implemented in WebVTTParser::createDocumentFragmentFromCueText.

I have also changed to not use m_documentFragment, as this is what is returned to the developer / user when it calls ::getCueAsHTML(), so I have used a different instance here.

> > LayoutTests/media/track/track-cue-rendering-expected.txt:33
> > -EXPECTED (getComputedStyle(textTrackDisplayElement(video, 'container')).fontSize == '12px') OK
> > +EXPECTED (getComputedStyle(mediaControlsElement(internals.shadowRoot(video).firstChild, '-webkit-media-text-track-container')).fontSize == '12px') OK
> 
> I would rather have specifics about the shadow hierarchy in a helper function so less will have to change if (when) we change it. Can't you use textTrackDisplayElement() by changing it to work when the "id" parameter is missing and call "textTrackDisplayElement(video)" here?

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.