Bug 165390 - [Modern Media Controls] Rendering issues with controls bar when captions are on
Summary: [Modern Media Controls] Rendering issues with controls bar when captions are on
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-05 11:15 PST by Antoine Quint
Modified: 2016-12-06 15:46 PST (History)
5 users (show)

See Also:


Attachments
Screen recording (2.80 MB, video/quicktime)
2016-12-05 11:18 PST, Antoine Quint
no flags Details
Patch (15.51 KB, patch)
2016-12-06 11:04 PST, Antoine Quint
dino: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-12-05 11:15:15 PST
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.
Comment 1 Antoine Quint 2016-12-05 11:18:32 PST
Created attachment 296165 [details]
Screen recording
Comment 2 Antoine Quint 2016-12-05 20:31:42 PST
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.
Comment 3 Antoine Quint 2016-12-06 07:16:08 PST
Cc'ing Sam who implemented the original system to set runtime flags with HTML comments in tests.
Comment 4 Antoine Quint 2016-12-06 07:16:38 PST
Oops, wrong bug :)
Comment 5 Antoine Quint 2016-12-06 11:04:57 PST
Created attachment 296302 [details]
Patch
Comment 6 Dean Jackson 2016-12-06 11:43:26 PST
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.
Comment 7 Antoine Quint 2016-12-06 12:25:53 PST
(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.
Comment 8 WebKit Commit Bot 2016-12-06 12:51:12 PST
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
Comment 9 Antoine Quint 2016-12-06 15:46:01 PST
https://trac.webkit.org/changeset/209431