Bug 165390 – [Modern Media Controls] Rendering issues with controls bar when captions are on

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

[Antoine Quint]

Try this following HTML on ToT with the Modern Media Controls runtime flag turned on:

<video src="/path/to/webkit/LayoutTests/media/content/CC+Subtitles.mov" style="width: 848px" controls autoplay></video>

This plays a vide with captions, let it play for a bit, notice how the timestamps disappear around the scrubber as the captions become visible. Then pause the video and notice how the entire controls bar loses all of its content and never recovers it.

[Antoine Quint]

Created attachment 296165 [details]
Screen recording

[Antoine Quint]

This is coming from RenderImage::layoutShadowControls() assuming there is a single RenderBox child under the media element's UA shadow root, but in our case with modern media controls, we have two: the captions and the media controls. The simple fix is to use a single container in the case of modern media controls.

[Antoine Quint]

Cc'ing Sam who implemented the original system to set runtime flags with HTML comments in tests.

[Antoine Quint]

Oops, wrong bug :)

[Antoine Quint]

Created attachment 296302 [details]
Patch

[Dean Jackson]

Comment on attachment 296302 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        We would face some layout issues with captions due to RenderImage::layoutShadowControls()
> +        expecting a single RenderBox in the media controls shadow root, which was the case with
> +        legacy media controls, but no longer the case with modern media controls. We now host
> +        both the captions and the media controls elements under a single container, and add
> +        an asertion in RenderImage to check that a single RenderBox child exists.

Wouldn't the better fix be changing RenderImage to allow multiple children? Otherwise someone will probably run into the same issue eventually.

Maybe you could apply this patch and file a bug (on yourself) to fix RenderImage.

[Antoine Quint]

(In reply to comment #6)
> Comment on attachment 296302 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296302&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        We would face some layout issues with captions due to RenderImage::layoutShadowControls()
> > +        expecting a single RenderBox in the media controls shadow root, which was the case with
> > +        legacy media controls, but no longer the case with modern media controls. We now host
> > +        both the captions and the media controls elements under a single container, and add
> > +        an asertion in RenderImage to check that a single RenderBox child exists.
> 
> Wouldn't the better fix be changing RenderImage to allow multiple children?
> Otherwise someone will probably run into the same issue eventually.
> 
> Maybe you could apply this patch and file a bug (on yourself) to fix
> RenderImage.

Ryosuke's advice was to add the assertion, which would at least clearly indicate the cause of the issue.

[WebKit Commit Bot]

Comment on attachment 296302 [details]
Patch

Rejecting attachment 296302 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 296302, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ntroller-single-container-expected.txt
patching file LayoutTests/media/modern-media-controls/media-controller/media-controller-single-container.html
patching file LayoutTests/platform/ios-simulator/TestExpectations
Hunk #1 FAILED at 2761.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/ios-simulator/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2635627

[Antoine Quint]

https://trac.webkit.org/changeset/209431