Bug 142138 – Update positioning and sizing for WebKit inline media controls on OS X

• Component: Media
• Status: RESOLVED FIXED

[Roger Fong]

Need to update media element look for OSX inline controls

[Roger Fong]

rdar://problem/19997384

[Roger Fong]

Created attachment 247615 [details]
patch

[Roger Fong]

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)

[Dean Jackson]

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.

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

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.

[Roger Fong]

(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

[Roger Fong]

(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

[Roger Fong]

Bug to keep track of rebaselining media control tests.
https://bugs.webkit.org/show_bug.cgi?id=142142

[Roger Fong]

Created attachment 247654 [details]
patch

[Dean Jackson]

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.

[Roger Fong]

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

[Roger Fong]

Created attachment 247699 [details]
patch part 2

[Dean Jackson]

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.

[Roger Fong]

New bug filed for patch part 2 here:
https://bugs.webkit.org/show_bug.cgi?id=142188

[Roger Fong]

Patch part 1 landed: http://trac.webkit.org/changeset/180893