Intent to Deprecate and Remove: ::-webkit-media-controls* pseudo-element selectors

184 views
Skip to first unread message

Philip Jägenstedt

unread,
Oct 27, 2014, 11:06:17 AM10/27/14
to blink-dev

Primary eng (and PM) emails

phi...@opera.com


Summary

The UI for <video controls> is implemented using shadow DOM elements, which are styled using ::-webkit-media-controls* pseudo-element selectors in the user agent stylesheet, mediaControls.css. Unfortunately, page author stylesheets can also use these selectors, overriding parts of the user agent style.


Rename ::-webkit-media-controls* to ::-internal-media-controls* and make them accessible only to the user agent stylesheet.


Motivation

In https://crbug.com/415407 (restricted, sorry) a fuzzer has found a problem where a page overrides ::-webkit-media-controls { display: ... }, hitting an assert.


https://codereview.chromium.org/662243003/ is the review, and I've been unable to find a fix which actually gets at the root of the problem other than simply unexposing the pseudo-elements to the Web. (If I had a better grip of style/layout/rendering maybe I could, who knows.)

Compatibility Risk

Unfortunately, there is some:

http://css-tricks.com/custom-controls-in-html5-video-full-screen/


This combines ::-webkit-media-controls { display: none; } with a bug in our fullscreen implementation, namely that z-index on non-fullscreen elements can allow them to appear above the fullscreen element.


Note that fixing fullscreen to use the top-layer <https://crbug.com/240576> would also break this method.

Alternative implementation suggestion for web developers

To get rid of native fullscreen controls, use a parent of the <video> element as the fullscreen element, don't use <video controls> and place things on top of the <video> as usual.


Usage information from UseCounter

None, and issue 41540 is targeted for M39 so we can't add a counter and wait. If this is unacceptable some temporary workaround in C++ will likely be needed.


Entry on chromestatus.com, crbug.com, or MDN

None.


Requesting approval to remove too?

Yes, although it's removal by renaming.

Dimitri Glazkov

unread,
Oct 27, 2014, 1:36:08 PM10/27/14
to Philip Jägenstedt, blink-dev
On Mon, Oct 27, 2014 at 8:06 AM, Philip Jägenstedt <phi...@opera.com> wrote:

Primary eng (and PM) emails

phi...@opera.com


Summary

The UI for <video controls> is implemented using shadow DOM elements, which are styled using ::-webkit-media-controls* pseudo-element selectors in the user agent stylesheet, mediaControls.css. Unfortunately, page author stylesheets can also use these selectors, overriding parts of the user agent style.


Rename ::-webkit-media-controls* to ::-internal-media-controls* and make them accessible only to the user agent stylesheet.


I think we should be really careful about this. A simple query:


shows a bunch of developer enthusiasm around these pseudoelements, and shutting the door will probably break some fingers. And then you'll be inundated with emotional emails from the injured :)


Motivation

In https://crbug.com/415407 (restricted, sorry) a fuzzer has found a problem where a page overrides ::-webkit-media-controls { display: ... }, hitting an assert.


Can you cc me on that bug? Maybe there's a solution that's less violent than finger-breaking.

Elliott Sprehn

unread,
Oct 27, 2014, 2:28:40 PM10/27/14
to Dimitri Glazkov, Philip Jägenstedt, blink-dev
It sounds like you just need a bunch of !important's in the UA sheet so the authors can't override style in unexpected ways. Also RenderMedia does sketchy stuff, we should fix it: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/rendering/RenderMedia.cpp&q=RenderMedia.cpp&sq=package:chromium&l=62

That's blindly casting to RenderBox, but if you set the style you can make it do something bad. I suspect there's more of that kind of thing in the media code.

Julien Chaffraix

unread,
Oct 28, 2014, 1:51:41 PM10/28/14
to Elliott Sprehn, Dimitri Glazkov, Philip Jägenstedt, blink-dev
On Mon, Oct 27, 2014 at 11:27 AM, Elliott Sprehn <esp...@chromium.org> wrote:
> It sounds like you just need a bunch of !important's in the UA sheet so the
> authors can't override style in unexpected ways. Also RenderMedia does
> sketchy stuff, we should fix it:
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/rendering/RenderMedia.cpp&q=RenderMedia.cpp&sq=package:chromium&l=62
>
> That's blindly casting to RenderBox, but if you set the style you can make
> it do something bad. I suspect there's more of that kind of thing in the
> media code.

That seems orthogonal to this thread. There will be a need for a C++
solution - as Philip pointed out - but the inter-operability harm is
still there. While we should investigate another fix, we should still
try to see if we can unexposed these APIs.

>
>
> On Mon, Oct 27, 2014 at 10:36 AM, Dimitri Glazkov <dgla...@chromium.org>
> wrote:
>>
>>
>>
>> On Mon, Oct 27, 2014 at 8:06 AM, Philip Jägenstedt <phi...@opera.com>
>> wrote:
>>>
>>> Primary eng (and PM) emails
>>>
>>> phi...@opera.com
>>>
>>>
>>> Summary
>>>
>>> The UI for <video controls> is implemented using shadow DOM elements,
>>> which are styled using ::-webkit-media-controls* pseudo-element selectors in
>>> the user agent stylesheet, mediaControls.css. Unfortunately, page author
>>> stylesheets can also use these selectors, overriding parts of the user agent
>>> style.
>>>
>>>
>>> Rename ::-webkit-media-controls* to ::-internal-media-controls* and make
>>> them accessible only to the user agent stylesheet.
>>
>>
>> I think we should be really careful about this. A simple query:
>>
>> https://www.google.com/?#q=webkit-media-controls
>>
>> shows a bunch of developer enthusiasm around these pseudoelements, and
>> shutting the door will probably break some fingers. And then you'll be
>> inundated with emotional emails from the injured :)

It seems like we need better data to make this call. Without knowing
how popular these pseudos are, it's difficult to know how many fingers
we break.

Also the problem is that the more we wait, the more people will rely
on these non-standard and not meant to be exposed extensions, which is
why I am in favor of at least deprecating with the intent to un-expose
(once we have more data). Stating that they are deprecated at least
indicate our intent that you're using them at your own risk.

Julien

Philip Jägenstedt

unread,
Oct 28, 2014, 3:22:17 PM10/28/14
to Dimitri Glazkov, blink-dev
On Mon, Oct 27, 2014 at 6:36 PM, Dimitri Glazkov <dgla...@chromium.org> wrote:
>
>
> On Mon, Oct 27, 2014 at 8:06 AM, Philip Jägenstedt <phi...@opera.com>
> wrote:
>>
>> Primary eng (and PM) emails
>>
>> phi...@opera.com
>>
>>
>> Summary
>>
>> The UI for <video controls> is implemented using shadow DOM elements,
>> which are styled using ::-webkit-media-controls* pseudo-element selectors in
>> the user agent stylesheet, mediaControls.css. Unfortunately, page author
>> stylesheets can also use these selectors, overriding parts of the user agent
>> style.
>>
>>
>> Rename ::-webkit-media-controls* to ::-internal-media-controls* and make
>> them accessible only to the user agent stylesheet.
>
>
> I think we should be really careful about this. A simple query:
>
> https://www.google.com/?#q=webkit-media-controls
>
> shows a bunch of developer enthusiasm around these pseudoelements, and
> shutting the door will probably break some fingers. And then you'll be
> inundated with emotional emails from the injured :)
>
>>
>> Motivation
>>
>> In https://crbug.com/415407 (restricted, sorry) a fuzzer has found a
>> problem where a page overrides ::-webkit-media-controls { display: ... },
>> hitting an assert.
>
>
> Can you cc me on that bug? Maybe there's a solution that's less violent than
> finger-breaking.

Done.

Philip

Philip Jägenstedt

unread,
Oct 28, 2014, 3:27:40 PM10/28/14
to Elliott Sprehn, Dimitri Glazkov, blink-dev
On Mon, Oct 27, 2014 at 7:27 PM, Elliott Sprehn <esp...@chromium.org> wrote:
> It sounds like you just need a bunch of !important's in the UA sheet so the
> authors can't override style in unexpected ways.

::-webkit-media-controls { display: flex !important; } in
mediaControls.css does fix the problem, but since it is exactly the
selector and property that
http://css-tricks.com/custom-controls-in-html5-video-full-screen/
depends on, it's not actually any better than unexposing
::-webkit-media-controls completely IMHO. There are other properties
on ::-webkit-media-controls, but none that seems useful at all at
first glance.

> Also RenderMedia does sketchy stuff, we should fix it:
> https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/rendering/RenderMedia.cpp&q=RenderMedia.cpp&sq=package:chromium&l=62
>
> That's blindly casting to RenderBox, but if you set the style you can make
> it do something bad. I suspect there's more of that kind of thing in the
> media code.

I've investigated that particular instance and it doesn't seem
relevant to the problem. I think if it were involved the cast would
assert in debug builds. I couldn't find any other unchecked casts in
RenderMedia*, but I may have missed something.

Philip

Philip Jägenstedt

unread,
Oct 29, 2014, 9:10:27 AM10/29/14
to Elliott Sprehn, Dimitri Glazkov, blink-dev
API OWNERS, you can ignore this thread now. With the help of Rune and
Morten, I found another solution:
https://codereview.chromium.org/689613002/

I think we should get rid of ::-webkit-media-controls*, but not today.

Philip
Reply all
Reply to author
Forward
0 new messages