Use ::-internal-media-controls* instead of ::-webkit-media-controls*

Use ::-internal-media-controls* instead of ::-webkit-media-controls*

BUG=415407

[philipj_slow, 2014-10-22 14:40:23]

philipj@opera.com changed reviewers:
+ jchaffraix@chromium.org

[philipj_slow, 2014-10-22 14:40:24]

jchaffraix, can you PTAL?

I want to do the same thing for pseudo element names that you did for properties
and values in https://codereview.chromium.org/22849019

As you can see, this patch doesn't actually contain the critical bit of code
that makes ::-internal-* only be recognized while parsing user agent style
sheets, and LayoutTests/media/controls-css-overload.html does fail.

CSSSelector seems to lazily extract the type in extractPseudoType(), although in
my gdb session it does in fact happen inside the parseUASheet call. Not sure if
that's reliable.

Some solutions have come to mind:

1. A ParsingUASheetIndicator stack variable with a static
ParsingUASheetIndicator::foo() to be used deep down to check if we're current
parsing user agent style sheets.

2. Arranging to pass CSSParserMode to CSSSelector::extractPseudoType(). It would
probably look a bit funky with pseudoType(CSSParserMode) and
matchesPseudoElement(CSSParserMode), though.

3. Adding CSSSelector::isInternalPseudoElement() and using that at other stage
to prevent page author style containing these pseudos from having any actual
effect. Not sure where.

It would also be great if the solution would incidentally make it easier to add
use counters for non-standard pseudos.

Any feedback would be appreciated.

[rune, 2014-10-22 14:49:10]

On 2014/10/22 at 14:40:24, philipj wrote:

> Any feedback would be appreciated.

I can take a look at this if Julien doesn't. Not in a few hours, though.

[rune, 2014-10-22 17:58:18]

On 2014/10/22 at 14:40:24, philipj wrote:
> jchaffraix, can you PTAL?
> 
> I want to do the same thing for pseudo element names that you did for
properties and values in https://codereview.chromium.org/22849019
> 
> As you can see, this patch doesn't actually contain the critical bit of code
that makes ::-internal-* only be recognized while parsing user agent style
sheets, and LayoutTests/media/controls-css-overload.html does fail.
> 
> CSSSelector seems to lazily extract the type in extractPseudoType(), although
in my gdb session it does in fact happen inside the parseUASheet call. Not sure
if that's reliable.

I don't know why we have this laziness here. We'll need to know the pseudo type
at parse time to figure out if it is a valid selector. We call pseudoType()
which extracts the type in the pseudo productions in CSSGrammar.y. That's where
you should check for internal classes/elements, I think. You should be able to
find if internal pseudo elements/classes should be accepted or not from the
CSSParser/CSSParserContext.


The code in CSSGrammar.y looks like this:

CSSSelector::PseudoType type = $$->pseudoType();
if (type == CSSSelector::PseudoUnknown) {
    parser->reportError($2, InvalidSelectorPseudoCSSError);
    YYERROR;
}

You can do:

CSSSelector::PseudoType type = $$->pseudoType();
if (parser->isUnknownPseudoType(type)) {
    parser->reportError($2, InvalidSelectorPseudoCSSError);
    YYERROR;
}

Where isUnknownPseudoType returns true for :-internal-*/::-internal-* in non-UA
mode.

> Some solutions have come to mind:
> 
> 1. A ParsingUASheetIndicator stack variable with a static
ParsingUASheetIndicator::foo() to be used deep down to check if we're current
parsing user agent style sheets.
> 
> 2. Arranging to pass CSSParserMode to CSSSelector::extractPseudoType(). It
would probably look a bit funky with pseudoType(CSSParserMode) and
matchesPseudoElement(CSSParserMode), though.

Yes, I wouldn't do that.

> 3. Adding CSSSelector::isInternalPseudoElement() and using that at other stage
to prevent page author style containing these pseudos from having any actual
effect. Not sure where.

You need to detect the internal pseudo element as an invalid selector at parsing
time to avoid making it observable (must not create a StyleRule in the dom
either).

> It would also be great if the solution would incidentally make it easier to
add use counters for non-standard pseudos.
> 
> Any feedback would be appreciated.

[Julien - ping for review, 2014-10-22 22:26:02]

On 2014/10/22 at 17:58:18, rune wrote:
> On 2014/10/22 at 14:40:24, philipj wrote:
> > jchaffraix, can you PTAL?
> > 
> > I want to do the same thing for pseudo element names that you did for
properties and values in https://codereview.chromium.org/22849019
> > 
> > As you can see, this patch doesn't actually contain the critical bit of code
that makes ::-internal-* only be recognized while parsing user agent style
sheets, and LayoutTests/media/controls-css-overload.html does fail.
> > 
> > CSSSelector seems to lazily extract the type in extractPseudoType(),
although in my gdb session it does in fact happen inside the parseUASheet call.
Not sure if that's reliable.
> 
> I don't know why we have this laziness here. We'll need to know the pseudo
type at parse time to figure out if it is a valid selector. We call pseudoType()
which extracts the type in the pseudo productions in CSSGrammar.y. That's where
you should check for internal classes/elements, I think. You should be able to
find if internal pseudo elements/classes should be accepted or not from the
CSSParser/CSSParserContext.
> 
> 
> The code in CSSGrammar.y looks like this:
> 
> CSSSelector::PseudoType type = $$->pseudoType();
> if (type == CSSSelector::PseudoUnknown) {
>     parser->reportError($2, InvalidSelectorPseudoCSSError);
>     YYERROR;
> }
> 
> You can do:
> 
> CSSSelector::PseudoType type = $$->pseudoType();
> if (parser->isUnknownPseudoType(type)) {
>     parser->reportError($2, InvalidSelectorPseudoCSSError);
>     YYERROR;
> }
> 
> Where isUnknownPseudoType returns true for :-internal-*/::-internal-* in
non-UA mode.

I like rune's idea (and think this is the way to go) with one caveat. I don't
want isUnknownPseudoType() to take the mode into account: it's better to be
explicit and do something like:

if (type == CSSSelector::PseudoUnknown ||
!parser->isSelectorAllowedInParsingMode(type)) {
    ....
}

[philipj_slow, 2014-10-23 11:43:27]

discard -internal-* by string matching

[philipj_slow, 2014-10-23 12:39:09]

introduce PseudoInternalCustomElement

[philipj_slow, 2014-10-23 13:08:05]

What I have now seems to work, but there is an issue.

First, rune discovered that people have actually been using
::-webkit-media-controls in the wild:
http://css-tricks.com/custom-controls-in-html5-video-full-screen/

To make matters worse, it's even precisely the problematic property, display,
that's being overridden.

[Julien - ping for review, 2014-10-23 14:57:11]

On 2014/10/23 at 13:08:05, philipj wrote:
> What I have now seems to work, but there is an issue.
> 
> First, rune discovered that people have actually been using
::-webkit-media-controls in the wild:
http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> 
> To make matters worse, it's even precisely the problematic property, display,
that's being overridden.

Rats! We need to get some numbers from the wild then. How about we just migrate
the non-controversial pseudos right now before someone finds them?

[philipj_slow, 2014-10-23 15:08:26]

On 2014/10/23 14:57:11, Julien Chaffraix - PST wrote:
> On 2014/10/23 at 13:08:05, philipj wrote:
> > What I have now seems to work, but there is an issue.
> > 
> > First, rune discovered that people have actually been using
> ::-webkit-media-controls in the wild:
> http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> > 
> > To make matters worse, it's even precisely the problematic property,
display,
> that's being overridden.
> 
> Rats! We need to get some numbers from the wild then. How about we just
migrate
> the non-controversial pseudos right now before someone finds them?

Yeah, but what are we going to do about
::-webkit-media-controls{display:inline;}, which is the problem in issue 415407?
We could use "display: flex !important" in mediaControls.css but even that would
interfere with the technique in
http://css-tricks.com/custom-controls-in-html5-video-full-screen/

The failing assert is deep down in RenderInline::splitInlines, far from anything
media controls-specific, so I've yet to come up with a workaround that wouldn't
break that use case.

[Julien - ping for review, 2014-10-23 18:07:36]

On 2014/10/23 at 15:08:26, philipj wrote:
> On 2014/10/23 14:57:11, Julien Chaffraix - PST wrote:
> > On 2014/10/23 at 13:08:05, philipj wrote:
> > > What I have now seems to work, but there is an issue.
> > > 
> > > First, rune discovered that people have actually been using
> > ::-webkit-media-controls in the wild:
> > http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> > > 
> > > To make matters worse, it's even precisely the problematic property,
display,
> > that's being overridden.
> > 
> > Rats! We need to get some numbers from the wild then. How about we just
migrate
> > the non-controversial pseudos right now before someone finds them?
> 
> Yeah, but what are we going to do about
::-webkit-media-controls{display:inline;}, which is the problem in issue 415407?
We could use "display: flex !important" in mediaControls.css but even that would
interfere with the technique in
http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> 
> The failing assert is deep down in RenderInline::splitInlines, far from
anything media controls-specific, so I've yet to come up with a workaround that
wouldn't break that use case.

Unless I am missing something, why don't we put a use-counter for
::-webkit-media-controls and -webkit-media-controls-enclosure with an intent to
un-expose them? The other pseudos are not mentioned and can be just un-exposed.

[Julien - ping for review, 2014-10-23 18:09:41]

On 2014/10/23 at 18:07:36, Julien Chaffraix - PST wrote:
> On 2014/10/23 at 15:08:26, philipj wrote:
> > On 2014/10/23 14:57:11, Julien Chaffraix - PST wrote:
> > > On 2014/10/23 at 13:08:05, philipj wrote:
> > > > What I have now seems to work, but there is an issue.
> > > > 
> > > > First, rune discovered that people have actually been using
> > > ::-webkit-media-controls in the wild:
> > > http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> > > > 
> > > > To make matters worse, it's even precisely the problematic property,
display,
> > > that's being overridden.
> > > 
> > > Rats! We need to get some numbers from the wild then. How about we just
migrate
> > > the non-controversial pseudos right now before someone finds them?
> > 
> > Yeah, but what are we going to do about
::-webkit-media-controls{display:inline;}, which is the problem in issue 415407?
We could use "display: flex !important" in mediaControls.css but even that would
interfere with the technique in
http://css-tricks.com/custom-controls-in-html5-video-full-screen/
> > 
> > The failing assert is deep down in RenderInline::splitInlines, far from
anything media controls-specific, so I've yet to come up with a workaround that
wouldn't break that use case.
> 
> Unless I am missing something, why don't we put a use-counter for
::-webkit-media-controls and -webkit-media-controls-enclosure with an intent to
un-expose them? The other pseudos are not mentioned and can be just un-exposed.

Btw, I forgot (twice) but for un-exposing we should open an "intent to unship"
Blink thread.

[philipj_slow, 2014-10-23 18:29:13]

I'd love to unexpose these, but what the CL descrption doesn't say is that the
motivation for this change is the assert reported in https://crbug.com/415407

We can't wait for use counter data to fix that bug, I've gotten nag-mail since
it's an M39 blocker.

Assuming we don't just unexpose ::-webkit-media-controls now, do we have any
mechanism that would allow ::-webkit-media-controls { display:none; } to be
specified by author style while use counter data is collected, while still not
honoring :-webkit-media-controls { display:inline; } which causes the assert?

[rune, 2014-10-23 22:31:12]

On 2014/10/23 at 18:29:13, philipj wrote:
> I'd love to unexpose these, but what the CL descrption doesn't say is that the
motivation for this change is the assert reported in https://crbug.com/415407
> 
> We can't wait for use counter data to fix that bug, I've gotten nag-mail since
it's an M39 blocker.
> 
> Assuming we don't just unexpose ::-webkit-media-controls now, do we have any
mechanism that would allow ::-webkit-media-controls { display:none; } to be
specified by author style while use counter data is collected, while still not
honoring :-webkit-media-controls { display:inline; } which causes the assert?

There's the PropertyWhitelistType filtering currently used for ::first-letter
and ::cue pseudo elements allowing a subset of properties. I haven't looked at
the details, though. Seems like you would need separate enums for the pseudo
element types, not the shared PseudoWebKitCustomElement that is currently used.
Also, that mechanism doesn't filter values, only properties.

[philipj_slow, 2014-10-24 08:23:07]

On 2014/10/23 22:31:12, rune wrote:
> On 2014/10/23 at 18:29:13, philipj wrote:
> > I'd love to unexpose these, but what the CL descrption doesn't say is that
the
> motivation for this change is the assert reported in https://crbug.com/415407
> > 
> > We can't wait for use counter data to fix that bug, I've gotten nag-mail
since
> it's an M39 blocker.
> > 
> > Assuming we don't just unexpose ::-webkit-media-controls now, do we have any
> mechanism that would allow ::-webkit-media-controls { display:none; } to be
> specified by author style while use counter data is collected, while still not
> honoring :-webkit-media-controls { display:inline; } which causes the assert?
> 
> There's the PropertyWhitelistType filtering currently used for ::first-letter
> and ::cue pseudo elements allowing a subset of properties. I haven't looked at
> the details, though. Seems like you would need separate enums for the pseudo
> element types, not the shared PseudoWebKitCustomElement that is currently
used.
> Also, that mechanism doesn't filter values, only properties.

OK, that doesn't sound like it will help, then.

[philipj_slow, 2014-10-24 08:34:22]

philipj@opera.com changed reviewers:
+ pdr@chromium.org

[philipj_slow, 2014-10-24 08:34:22]

Philip, I need your input as an API OWNER.

Comment #8 describes a problem with my solution. Do you think it's acceptable to
unexpose ::-webkit-media-controls without any supporting use counter data in
order to fix this bug?

[Mike West, 2014-10-24 10:44:48]

On 2014/10/24 08:34:22, philipj wrote:
> Philip, I need your input as an API OWNER.
> 
> Comment #8 describes a problem with my solution. Do you think it's acceptable
to
> unexpose ::-webkit-media-controls without any supporting use counter data in
> order to fix this bug?

I'd suggest poking blink-dev@.

[philipj_slow, 2014-10-29 10:15:26]

blink-dev said no, closing this in favor of
https://codereview.chromium.org/689613002/