Bug 79746 – Main rendering steps for displaying the cue boxes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

Bug for implementing the "outer" steps 1 - 11 of the WebVTT cue text rendering rules.

"The rules are as follows:
1. If the media element is an audio element, or is another playback mechanism with no rendering area, abort these steps. There is nothing to render.
[...]
11. Return output."

[Victor Carbune]

Created attachment 129769 [details]
Preliminary patch

[Eric Carlson]

Comment on attachment 129769 [details]
Preliminary patch

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.

> 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.

> 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.

> 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?

> 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?

> 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?

> 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?

[Victor Carbune]

Created attachment 130702 [details]
Updated patch

[Victor Carbune]

(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.

[WebKit Review Bot]

Comment on attachment 130702 [details]
Updated patch

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

New failing tests:
media/media-document-audio-repaint.html
media/video-no-audio.html
media/controls-strict.html
media/video-volume-slider.html
media/controls-styling.html
media/video-display-toggle.html
media/audio-controls-rendering.html
media/video-zoom-controls.html
media/video-controls-rendering.html
media/controls-without-preload.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/video-empty-source.html
media/video-playing-and-pause.html
media/controls-after-reload.html

[Victor Carbune]

(In reply to comment #5)
> (From update of attachment 130702 [details])
> Attachment 130702 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11862025
> 
> New failing tests:
> media/media-document-audio-repaint.html
> media/video-no-audio.html
> media/controls-strict.html
> media/video-volume-slider.html
> media/controls-styling.html
> media/video-display-toggle.html
> media/audio-controls-rendering.html
> media/video-zoom-controls.html
> media/video-controls-rendering.html
> media/controls-without-preload.html
> media/media-controls-clone.html
> fast/layers/video-layer.html
> media/video-empty-source.html
> media/video-playing-and-pause.html
> media/controls-after-reload.html

These come from replacing the display:block with -webkit-box; From the image diffs it seems the functionality itself is preserved, but there are some unnoticeable pixels different.

[Eric Carlson]

Comment on attachment 130702 [details]
Updated patch

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

I made a few minor suggestions, but this looks great overall! It is probably worth regenerating the ChangeLogs to make sure all changes are documented.

> Source/WebCore/ChangeLog:13
> +        * css/mediaControls.css:
> +        (video::-webkit-media-text-track-display): Adjusted text color to match
> +        the color required in the WebVTT spec (section 3.5.1 'color' property)

It is worth mentioning the changes you made to position the controls.

> Source/WebCore/css/mediaControls.css:44
> -    display: block;
> +    display: -webkit-box;
>      direction: ltr;
> +    -webkit-box-align: start;
> +    -webkit-box-pack: end;
> +    -webkit-box-orient: vertical;

I expect this is the source of the chromium test failures?

> Source/WebCore/css/mediaControlsChromium.css:45
> -    position: absolute;
> +    position: relative;
>      overflow: visible;

This didn't make it into the ChangeLog.

> Source/WebCore/html/shadow/MediaControlElements.cpp:43
>  #include "HTMLMediaElement.h"
> +#include "HTMLVideoElement.h"
>  #include "HTMLNames.h"

Includes should be sorted.

> Source/WebCore/html/shadow/MediaControlElements.cpp:184
> +    stopTimer();
> +
> +    double duration = document()->page() ? document()->page()->theme()->mediaControlsFadeOutDuration() : 0;
> +    m_transitionTimer.startOneShot(duration);

It would be good to have a brief comment here about why we need to use a timer for this. As you know, this confused at least me ;-)

The timer functions also didn't make it into the ChangeLog.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1244
> +    // 4. If the user agent is exposing a user interface for video, add to
> +    // output one or more completely transparent positioned CSS block boxes that
> +    // cover the same region as the user interface.
> +
> +    // 5. If the last time these rules were run, the user agent was not exposing
> +    // a user interface for video, but now it is, let reset be true. Otherwise,
> +    // let reset be false.

A brief comment about why you don't have to do anything explicit for these steps would be helpful.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1256
> +    // Redraw
> +    removeChildren();

This comment a bit terse.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1260
> +    // 9. If reset is false, then, for each text track cue cue in cues: if cue's
> +    // text track cue display state has a set of CSS boxes, then add those boxes
> +    // to output, and remove cue from cues.

A brief comment about why you don't have to do anything explicit for these steps would be helpful.

> Source/WebCore/html/track/TextTrackCue.cpp:426
> +    // 10.11. Apply the terms of the CSS specifications to nodes within the
> +    // following constraints, thus obtaining a set of CSS boxes positioned
> +    // relative to an initial containing block:
> +    RefPtr<HTMLDivElement> m_displayTree = HTMLDivElement::create(document);

Could you empty the div if it already exists instead of deleting and allocating it every time a cue changes?

> Source/WebCore/html/track/TextTrackCue.cpp:429
> +    // The document tree is the tree of WebVTT Node Objects rooted at nodes.
> +    RefPtr<DocumentFragment> documentTree = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);

Why not use TextTrackCue::getCueAsHTML() instead? That way we will use the fragment if it has already been created (always the case for a cue that has not been modified by script).

> LayoutTests/media/media-controls.js:35
> -function textTrackDisplayElement(parentElement, id)
> +function textTrackDisplayElement(parentElement)

Nit: I think this would be easier to understand if it kept the "id" parameter.

[Victor Carbune]

Created attachment 131107 [details]
Patch

More refactoring and marked tests for rebaselining

[Victor Carbune]

(In reply to comment #7)
> (From update of attachment 130702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130702&action=review
> 
> I made a few minor suggestions, but this looks great overall! It is probably worth regenerating the ChangeLogs to make sure all changes are documented.
> 
> > Source/WebCore/ChangeLog:13
> > +        * css/mediaControls.css:
> > +        (video::-webkit-media-text-track-display): Adjusted text color to match
> > +        the color required in the WebVTT spec (section 3.5.1 'color' property)
> 
> It is worth mentioning the changes you made to position the controls.
Done.

> > Source/WebCore/css/mediaControls.css:44
> > -    display: block;
> > +    display: -webkit-box;
> >      direction: ltr;
> > +    -webkit-box-align: start;
> > +    -webkit-box-pack: end;
> > +    -webkit-box-orient: vertical;
> 
> I expect this is the source of the chromium test failures?
Yes, just these lines and functional visual changes.

> > Source/WebCore/css/mediaControlsChromium.css:45
> > -    position: absolute;
> > +    position: relative;
> >      overflow: visible;
> 
> This didn't make it into the ChangeLog.
Regenerated changelogs.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:43
> >  #include "HTMLMediaElement.h"
> > +#include "HTMLVideoElement.h"
> >  #include "HTMLNames.h"
> 
> Includes should be sorted.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:184
> > +    stopTimer();
> > +
> > +    double duration = document()->page() ? document()->page()->theme()->mediaControlsFadeOutDuration() : 0;
> > +    m_transitionTimer.startOneShot(duration);
> 
> It would be good to have a brief comment here about why we need to use a timer for this. As you know, this confused at least me ;-)
Added.

> The timer functions also didn't make it into the ChangeLog.
Regenerated ChangeLogs.
 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1244
> > +    // 4. If the user agent is exposing a user interface for video, add to
> > +    // output one or more completely transparent positioned CSS block boxes that
> > +    // cover the same region as the user interface.
> > +
> > +    // 5. If the last time these rules were run, the user agent was not exposing
> > +    // a user interface for video, but now it is, let reset be true. Otherwise,
> > +    // let reset be false.
> 
> A brief comment about why you don't have to do anything explicit for these steps would be helpful.
Done.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1256
> > +    // Redraw
> > +    removeChildren();
> 
> This comment a bit terse.
Changed.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1260
> > +    // 9. If reset is false, then, for each text track cue cue in cues: if cue's
> > +    // text track cue display state has a set of CSS boxes, then add those boxes
> > +    // to output, and remove cue from cues.
> 
> A brief comment about why you don't have to do anything explicit for these steps would be helpful.
> 
> > Source/WebCore/html/track/TextTrackCue.cpp:426
> > +    // 10.11. Apply the terms of the CSS specifications to nodes within the
> > +    // following constraints, thus obtaining a set of CSS boxes positioned
> > +    // relative to an initial containing block:
> > +    RefPtr<HTMLDivElement> m_displayTree = HTMLDivElement::create(document);
> 
> Could you empty the div if it already exists instead of deleting and allocating it every time a cue changes?
Yes, it indeed makes a lot of sense not adding and deleting nodes all the time.

However, we certainly don't want to end up with a lot of empty divs (for each cue in the track), so they should be removed even if they're empty.

I have changed the methods to keep the div appended only while the cue has the active flag set, and called remove() on the displayTree when the flag is unset (within TextTrackCue::setIsActive).

> > Source/WebCore/html/track/TextTrackCue.cpp:429
> > +    // The document tree is the tree of WebVTT Node Objects rooted at nodes.
> > +    RefPtr<DocumentFragment> documentTree = WebVTTParser::create(0, m_scriptExecutionContext)->createDocumentFragmentFromCueText(m_content);
> 
> Why not use TextTrackCue::getCueAsHTML() instead? That way we will use the fragment if it has already been created (always the case for a cue that has not been modified by script).
Done.

> > LayoutTests/media/media-controls.js:35
> > -function textTrackDisplayElement(parentElement, id)
> > +function textTrackDisplayElement(parentElement)
> 
> Nit: I think this would be easier to understand if it kept the "id" parameter.
Done.

[Eric Carlson]

Comment on attachment 131107 [details]
Patch

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

This is a *terrific* patch, thank you! I am marking this cq- only because of the duplicate entry in the layout test ChangeLog.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1224
> +    LOG(Media, "MediaControlTextTrackContainerElement::updateDisplay()");

Nit: This function is called frequently enough that this logging generates a *lot* of output. We should remove it in a future check in.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1285
> +        if (!contains(displayTree.get()) && displayTree->hasChildNodes())
> +            appendChild(displayTree, ASSERT_NO_EXCEPTION, true);

Nit: I think the "hasChildNodes()" test will be faster than "contains()", so it should be the first test in the expression. This would be good to fix the next time this function is modified.

> Source/WebCore/html/track/TextTrackCue.cpp:424
> +    if (!m_displayTreeShouldChange || !m_scriptExecutionContext->isDocument())

Nit: unless a scriptExecutionContext can change type, we should "ASSERT(m_scriptExecutionContext->isDocument())" in the constructor instead of checking it every time through here. Again, this can be changed the next time this file is modified.

> Source/WebCore/html/track/TextTrackCue.cpp:441
> +    m_displayTree->appendChild(getCueAsHTML(), ASSERT_NO_EXCEPTION, true);

Why not use m_documentFragment directly, getCueAsHTML creates a new fragment every time? If it isn't possible to use m_documentFragment, why do we cache it in getCueAsHTML at all?

Either way, this is an optimization and can be done in a follow up patch.

> LayoutTests/ChangeLog:3175
> [phone]  Victor Carbune  <[e-mail]>
> +
> +        Updated LayoutTests for basic rendering of cues.
> +        https://bugs.webkit.org/show_bug.cgi?id=79746
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * media/media-controls.js:
> +        (mediaControlsElement):
> +        (textTrackDisplayElement):
> +        * media/track/track-cue-mutable-text-expected.txt:
> +        * media/track/track-cue-nothing-to-render-expected.txt:
> +        * media/track/track-cue-rendering-expected.txt:
> +        * media/track/track-cue-rendering.html:
> +

Oops, the ChangeLog should not have two entries.

[Victor Carbune]

Created attachment 131260 [details]
Small fixed and corrected ChangeLog

[Victor Carbune]

(In reply to comment #10)
> (From update of attachment 131107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131107&action=review
> 
> This is a *terrific* patch, thank you! I am marking this cq- only because of the duplicate entry in the layout test ChangeLog.
> 
> > Source/WebCore/html/shadow/MediaControlElements.cpp:1224
> > +    LOG(Media, "MediaControlTextTrackContainerElement::updateDisplay()");
> 
> Nit: This function is called frequently enough that this logging generates a *lot* of output. We should remove it in a future check in.
Fixed.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:1285
> > +        if (!contains(displayTree.get()) && displayTree->hasChildNodes())
> > +            appendChild(displayTree, ASSERT_NO_EXCEPTION, true);
> 
> Nit: I think the "hasChildNodes()" test will be faster than "contains()", so it should be the first test in the expression. This would be good to fix the next time this function is modified.
Fixed.
 
> > Source/WebCore/html/track/TextTrackCue.cpp:424
> > +    if (!m_displayTreeShouldChange || !m_scriptExecutionContext->isDocument())
> 
> Nit: unless a scriptExecutionContext can change type, we should "ASSERT(m_scriptExecutionContext->isDocument())" in the constructor instead of checking it every time through here. Again, this can be changed the next time this file is modified.
Fixed.

> > Source/WebCore/html/track/TextTrackCue.cpp:441
> > +    m_displayTree->appendChild(getCueAsHTML(), ASSERT_NO_EXCEPTION, true);
> 
> Why not use m_documentFragment directly, getCueAsHTML creates a new fragment every time? If it isn't possible to use m_documentFragment, why do we cache it in getCueAsHTML at all?
> 
> Either way, this is an optimization and can be done in a follow up patch.
Here's my approach:
* m_documentFragment is cached in getCueAsHTML() because it's faster to clone it, rather than re-parse & re-create it (re-parsing is required only when text contents have changed).
* m_displayTree is the cached 'internal version' of m_documentFragment for the shadow dom (it gets reprocessed only when m_displayTreeShouldChange is true), which later will have additional styling or other nodes we need for the cue rendering rules.

This implies that m_documentFragment can't be used directly in m_displayTree because calling getCueAsHTML() needs to have a clean version of it.

> > LayoutTests/ChangeLog:3175
> > [phone]  Victor Carbune  <[e-mail]>
> > +
> > +        Updated LayoutTests for basic rendering of cues.
> > +        https://bugs.webkit.org/show_bug.cgi?id=79746
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * media/media-controls.js:
> > +        (mediaControlsElement):
> > +        (textTrackDisplayElement):
> > +        * media/track/track-cue-mutable-text-expected.txt:
> > +        * media/track/track-cue-nothing-to-render-expected.txt:
> > +        * media/track/track-cue-rendering-expected.txt:
> > +        * media/track/track-cue-rendering.html:
> > +
> 
> Oops, the ChangeLog should not have two entries.
Sorry about this, it seems I'm constantly having issues merging ChangeLogs ;)

[Eric Carlson]

(In reply to comment #12)
> (In reply to comment #10)
> > 
> > Why not use m_documentFragment directly, getCueAsHTML creates a new fragment every time? If it isn't possible to use m_documentFragment, why do we cache it in getCueAsHTML at all?
> > 
> > Either way, this is an optimization and can be done in a follow up patch.
> Here's my approach:
> * m_documentFragment is cached in getCueAsHTML() because it's faster to clone it, rather than re-parse & re-create it (re-parsing is required only when text contents have changed).
> * m_displayTree is the cached 'internal version' of m_documentFragment for the shadow dom (it gets reprocessed only when m_displayTreeShouldChange is true), which later will have additional styling or other nodes we need for the cue rendering rules.
> 
> This implies that m_documentFragment can't be used directly in m_displayTree because calling getCueAsHTML() needs to have a clean version of it.
> 

  That makes sense. Lets add a comment explaining this the next time this file is modified.

[WebKit Review Bot]

Comment on attachment 131260 [details]
Small fixed and corrected ChangeLog

Clearing flags on attachment: 131260

Committed r110409: <http://trac.webkit.org/changeset/110409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This patch broke the ability of the media controls to be dragged in full screen mode.  This is due to going from absolute to relative positioning in mediaControls.css.

[Eric Carlson]

(In reply to comment #16)
> This patch broke the ability of the media controls to be dragged in full screen mode.  This is due to going from absolute to relative positioning in mediaControls.css.

Jer fixed the issue with https://bugs.webkit.org/show_bug.cgi?id=81176.

[Victor Carbune]

(In reply to comment #17)
> (In reply to comment #16)
> > This patch broke the ability of the media controls to be dragged in full screen mode.  This is due to going from absolute to relative positioning in mediaControls.css.
> 
> Jer fixed the issue with https://bugs.webkit.org/show_bug.cgi?id=81176.
Thank you, Jer.