Bug 142138 - Update positioning and sizing for WebKit inline media controls on OS X
Summary: Update positioning and sizing for WebKit inline media controls on OS X
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:
Depends on:
Blocks:
 
Reported: 2015-02-28 14:19 PST by Roger Fong
Modified: 2015-03-02 14:49 PST (History)
5 users (show)

See Also:


Attachments
patch (24.63 KB, patch)
2015-02-28 15:30 PST, Roger Fong
dino: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (1.49 MB, application/zip)
2015-02-28 16:17 PST, Build Bot
no flags Details
patch (12.00 KB, patch)
2015-03-01 23:33 PST, Roger Fong
dino: review+
Details | Formatted Diff | Diff
patch part 2 (15.88 KB, patch)
2015-03-02 14:25 PST, Roger Fong
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2015-02-28 14:19:39 PST
Need to update media element look for OSX inline controls
Comment 1 Roger Fong 2015-02-28 14:19:55 PST
rdar://problem/19997384
Comment 2 Roger Fong 2015-02-28 15:30:27 PST
Created attachment 247615 [details]
patch
Comment 3 Roger Fong 2015-02-28 15:36:16 PST
Things still left to do for inline controls:
a) Update the icons. rdar://problem/19997484
b) Find out what the size of the time watched/remaining placeholders should be. rdar://problem/19997487
c) Fix the issue of the time jumping backwards while scrubbing. rdar://problem/19997548
d) Update the actual materials (right now I'm using an opaque background with colors provided by the spec)
Comment 4 Dean Jackson 2015-02-28 16:07:00 PST
Comment on attachment 247615 [details]
patch

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

Looks OK. I think Eric is about to land a big change soon so you two should coordinate.

It would be nice if we could use ctx.scale(dpr, dpr) rather than multiply by dpr everywhere, which I think should be possible.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:184
> +    visibility: hidden;

Did you swap to visibility rather than opacity because of hit testing? I wonder if we could just do display none?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:372
> +    background-size: 100% 17px !important;

I think this should be 100% 100%. You've already set height to be 17px above, so you don't want to keep them in sync manually.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:999
> +    addRoundedRect: function(ctx, x, y, width, height, radius) {
> +        ctx.moveTo(x + radius, y);
> +        ctx.arcTo(x + width, y, x + width, y + radius, radius);
> +        ctx.lineTo(x + width, y + height - radius);
> +        ctx.arcTo(x + width, y + height, x + width - radius, y + height, radius);
> +        ctx.lineTo(x + radius, y + height);
> +        ctx.arcTo(x, y + height, x, y + height - radius, radius);
> +        ctx.lineTo(x, y + radius);
> +        ctx.arcTo(x, y, x + radius, y, radius);
> +    },

If this is the same as the one in ControllerIOS then we should be able to remove it from there, since this is the base class.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1025
> +        var midY = height / 2;
> +        var timelineHeight = 3 * dpr;
> +        var trackHeight = dpr;
> +        var scrubberWidth = 3 * dpr;
> +        var scrubberHeight = 15 * dpr;
> +        var borderSize = 2 * dpr;

Rather than multiplying by dpr everywhere, you might be better off applying a scale on the context.
Comment 5 Build Bot 2015-02-28 16:17:46 PST
Comment on attachment 247615 [details]
patch

Attachment 247615 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5792644194107392

New failing tests:
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
fast/hidpi/video-controls-in-hidpi.html
media/video-controls-rendering.html
accessibility/media-element.html
media/controls-without-preload.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/video-empty-source.html
media/audio-delete-while-slider-thumb-clicked.html
media/controls-after-reload.html
Comment 6 Build Bot 2015-02-28 16:17:49 PST
Created attachment 247616 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Dean Jackson 2015-02-28 16:21:30 PST
Comment on attachment 247615 [details]
patch

It seems this breaks a bunch of tests that probably depend on positioning.

Now that I've thought about it a bit more, I think you should split the positioning changes away from the canvas/rendering changes - so two patches.
Comment 8 Roger Fong 2015-02-28 18:17:23 PST
(In reply to comment #4)
> Comment on attachment 247615 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247615&action=review
> 
> Looks OK. I think Eric is about to land a big change soon so you two should
> coordinate.
> 
> It would be nice if we could use ctx.scale(dpr, dpr) rather than multiply by
> dpr everywhere, which I think should be possible.

Ok.

> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:184
> > +    visibility: hidden;

It actually used width: 0 before for accessiblity purposes.
I can switch it back.

> 
> Did you swap to visibility rather than opacity because of hit testing? I
> wonder if we could just do display none?
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:372
> > +    background-size: 100% 17px !important;
> 
Ok

> I think this should be 100% 100%. You've already set height to be 17px
> above, so you don't want to keep them in sync manually.
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:999
> > +    addRoundedRect: function(ctx, x, y, width, height, radius) {
> > +        ctx.moveTo(x + radius, y);
> > +        ctx.arcTo(x + width, y, x + width, y + radius, radius);
> > +        ctx.lineTo(x + width, y + height - radius);
> > +        ctx.arcTo(x + width, y + height, x + width - radius, y + height, radius);
> > +        ctx.lineTo(x + radius, y + height);
> > +        ctx.arcTo(x, y + height, x, y + height - radius, radius);
> > +        ctx.lineTo(x, y + radius);
> > +        ctx.arcTo(x, y, x + radius, y, radius);
> > +    },
> 
> If this is the same as the one in ControllerIOS then we should be able to
> remove it from there, since this is the base class.
> 
Ok

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1025
> > +        var midY = height / 2;
> > +        var timelineHeight = 3 * dpr;
> > +        var trackHeight = dpr;
> > +        var scrubberWidth = 3 * dpr;
> > +        var scrubberHeight = 15 * dpr;
> > +        var borderSize = 2 * dpr;
> 
> Rather than multiplying by dpr everywhere, you might be better off applying
> a scale on the context.
Ok
Comment 9 Roger Fong 2015-02-28 18:21:48 PST
(In reply to comment #7)
> Comment on attachment 247615 [details]
> patch
> 
> It seems this breaks a bunch of tests that probably depend on positioning.

I will skip them for now since my future changes will likely change them more.

> 
> Now that I've thought about it a bit more, I think you should split the
> positioning changes away from the canvas/rendering changes - so two patches.

Ok
Comment 10 Roger Fong 2015-02-28 19:27:52 PST
Bug to keep track of rebaselining media control tests.
https://bugs.webkit.org/show_bug.cgi?id=142142
Comment 11 Roger Fong 2015-03-01 23:33:55 PST
Created attachment 247654 [details]
patch
Comment 12 Dean Jackson 2015-03-02 10:53:00 PST
Comment on attachment 247654 [details]
patch

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

> Source/WebCore/ChangeLog:11
> +        Volume and timeline sliders will be drawn in a separate patch via 2d canvases.
> +        * Modules/mediacontrols/mediaControlsApple.css:

Nit. Separate these with a blank line.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:170
> +    -webkit-transform: rotate(-90deg) translateY(-4px) translateX(-5px);

You should be able to just say translate(-5px, -4px)

> LayoutTests/platform/mac/TestExpectations:1288
> +# Skip media control tests until new controls are finalized
> +webkit.org/b/142142 media/video-no-audio.html [ Skip ]

This is a bit scary.
Comment 13 Roger Fong 2015-03-02 12:49:35 PST
After looking through all the failures, all but one are rendering differences, and one involves clicking on a media control element.
My next two patches, which will be ready shortly, will change the results of these tests yet again so I will hold off on any rebaselining and test adjustment until those are landed as well.

Again, I've created a bug for this task as noted above: https://bugs.webkit.org/show_bug.cgi?id=142142
Comment 14 Roger Fong 2015-03-02 14:25:01 PST
Created attachment 247699 [details]
patch part 2
Comment 15 Dean Jackson 2015-03-02 14:42:27 PST
Comment on attachment 247699 [details]
patch part 2

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

r+, but I think this should be in a separate bug.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1023
> +        width = this.controls.timeline.offsetWidth;
> +        height = this.controls.timeline.offsetHeight;
> +        
> +        ctx.save();
> +        ctx.scale(dpr, dpr);
> +        ctx.clearRect(0, 0, width, height);

i'd do
width /= dpr;
height /= dpr;

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:1088
> +        width = this.controls.volume.offsetWidth;
> +        height = this.controls.volume.offsetHeight;
> +        
> +        ctx.save();
> +        ctx.scale(dpr, dpr);
> +        ctx.clearRect(0, 0, width, height);

Same here.
Comment 16 Roger Fong 2015-03-02 14:48:51 PST
New bug filed for patch part 2 here:
https://bugs.webkit.org/show_bug.cgi?id=142188
Comment 17 Roger Fong 2015-03-02 14:49:31 PST
Patch part 1 landed: http://trac.webkit.org/changeset/180893