Bug 131951 – [Media] Use classes instead of multiple pseudo attributes

• Component: Media
• Status: NEW

[Dean Jackson]

The Apple media controls shadow DOM uses pseudo attributes a lot. This exposes some limitations in our CSS selector engine that will eventually be fixed, but can be easily worked around now by using CSS classes.

[Radar WebKit Bug Importer]

<rdar://problem/16679113>

[Dean Jackson]

Created attachment 229845 [details]
Patch

[Build Bot]

Comment on attachment 229845 [details]
Patch

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

New failing tests:
fullscreen/video-controls-drag.html
fullscreen/video-controls-timeline.html
fullscreen/video-controls-override.html

[Build Bot]

Created attachment 229855 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229845 [details]
Patch

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

New failing tests:
fullscreen/video-controls-drag.html
fullscreen/video-controls-timeline.html
fullscreen/video-controls-override.html

[Build Bot]

Created attachment 229857 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229845 [details]
Patch

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

New failing tests:
fullscreen/video-controls-drag.html
fullscreen/video-controls-timeline.html
fullscreen/video-controls-override.html

[Build Bot]

Created attachment 229858 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229845 [details]
Patch

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

New failing tests:
fullscreen/video-controls-drag.html
fullscreen/video-controls-timeline.html
fullscreen/video-controls-override.html

[Build Bot]

Created attachment 229862 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Xabier Rodríguez Calvar]

Comment on attachment 229845 [details]
Patch

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

I guess you have a good reason in mind for these changes. Probably due to my own knowledge I fail to see their future benefits so I'd recommend not to do these change as it would require some crossplatform work that we haven't agreed yet to do. As I am not a reviewer, I cannot prevent you to do it, but I request you consider my humble opinion.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-259
> -        panel.setAttribute('pseudo', '-webkit-media-controls-panel');

Please, do not remove the pseudos as they are used at least by GTK port and we inherit from this. If you prefer to remove the pseudos for some reason, please add them at mediaControlsGtk.js so that our controls don't break.

> LayoutTests/media/controls-css-overload.html:9
> -            .nocontrols::-webkit-media-controls-panel{
> -                display:none;
> +            .nocontrols.webkit-media-controls-panel {
> +                display: none;
>              }
> -            .notimeline::-webkit-media-controls-timeline-container{
> -                display:none;
> +            .notimeline.webkit-media-controls-timeline-container {
> +                display: none;

These changes are very probably going to break EFL and GTK.

> LayoutTests/media/media-controls.js:8
> +        // FIXME: Could this be replaced by querySelector?
> +        if (element.classList && element.classList.contains(id))
> +            return element;
> +

If you are doing all those changes in the tests to remove the pseudo -, I think you are going to break, at least EFL tests that are not implemented in JS yet. I suggest that either you avoid all those changes to remove the - and remove it when checking for the class name (which would also avoid all in the tests and rebaselines) or you prepend it back to the id just before checking for pseudos (and maybe some other things I couldn't figure).

> LayoutTests/media/track/track-cue-rendering-with-padding.html:8
> -        video::-webkit-media-text-track-display {
> -          padding: 15px;
> +        video::-webkit-media-controls .webkit-media-text-track-display {
> +            padding: 15px;

These changes are probably going to break EFL and GTK.

[Dean Jackson]

(In reply to comment #11)

> (From update of attachment 229845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229845&action=review
> 
> I guess you have a good reason in mind for these changes. Probably due to my own knowledge I fail to see their future benefits so I'd recommend not to do these change as it would require some crossplatform work that we haven't agreed yet to do. As I am not a reviewer, I cannot prevent you to do it, but I request you consider my humble opinion.

Sure - I don't want to break anyone.

There are two main reasons why I want to do this:

- there really isn't any point in having this many pseudo classes for all the children elements. It's a slightly weird way to design content, sort of like putting id attributes on everything.

- there are some specific styles we want to apply in the Apple port that require a rule that matches something of the form [.classname element-with-pseudo.classname]. We have a bug that prevents it from working at the moment. I could try to fix that bug, but the entire way pseudo attributes select in the shadow DOM seems to be hacked on. Back to point 1, it seems much better to me to have a single "special" insertion point "::webkit-media-controls" and then everything under that is normal content.

I realise some people might not think these are particularly good reasons (or outright disagree).

> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-259
> > -        panel.setAttribute('pseudo', '-webkit-media-controls-panel');
> 
> Please, do not remove the pseudos as they are used at least by GTK port and we inherit from this. If you prefer to remove the pseudos for some reason, please add them at mediaControlsGtk.js so that our controls don't break.

Whoa. Why do you inherit from mediaControlsApple.js? I'm almost certainly going to break you at some point due to changes in our design. A major goal of having the controls done in script rather than C++ was to separate the port-specific code into something more manageable, without having this extremely complex inheritance between the ports.

I don't think mediaControlsPortA should ever be concerned that a change will break mediaControlsPortB. 

As for the tests... yeah, I definitely need to make sure I don't break that. Although again, I can imagine a comprehensive redesign of the Apple controls that would break a lot of tests no matter what.

I think maybe what I should do is copy mediaControlsApple.js to mediaControlsGTKBase.js (or something), then make the changes.

[Dean Jackson]

(In reply to comment #11)

> > LayoutTests/media/controls-css-overload.html:9
> > -            .nocontrols::-webkit-media-controls-panel{
> > -                display:none;
> > +            .nocontrols.webkit-media-controls-panel {
> > +                display: none;
> >              }
> > -            .notimeline::-webkit-media-controls-timeline-container{
> > -                display:none;
> > +            .notimeline.webkit-media-controls-timeline-container {
> > +                display: none;
> 
> These changes are very probably going to break EFL and GTK.

I'll keep the old rules around so that those ports don't break.

> > LayoutTests/media/media-controls.js:8
> > +        // FIXME: Could this be replaced by querySelector?
> > +        if (element.classList && element.classList.contains(id))
> > +            return element;
> > +
> 
> If you are doing all those changes in the tests to remove the pseudo -, I think you are going to break, at least EFL tests that are not implemented in JS yet.

The code above falls back to looking for the pseudo - it just prefers the class name. However, I did remove the "-" prefix, so I'll have to be a bit smarter about how that method works.

> I suggest that either you avoid all those changes to remove the - and remove it when checking for the class name (which would also avoid all in the tests and rebaselines)

Yes. I agree.

> or you prepend it back to the id just before checking for pseudos (and maybe some other things I couldn't figure).
> 
> > LayoutTests/media/track/track-cue-rendering-with-padding.html:8
> > -        video::-webkit-media-text-track-display {
> > -          padding: 15px;
> > +        video::-webkit-media-controls .webkit-media-text-track-display {
> > +            padding: 15px;
> 
> These changes are probably going to break EFL and GTK.

I'll keep the old rules around.

I'll definitely hold off on this change until we have something that works on all ports. Step one would be to break the link between mediaControlsGTK.js and mediaControlsApple.js

[Dean Jackson]

I forgot a third reason behind this change, which is that it makes the shadow content much more like a regular web page that can be developed externally from WebKit. Then you'll get full developer tools like the inspector and debugger.

Once you have working code you can push it back into WebKit.

[Xabier Rodríguez Calvar]

(In reply to comment #12)
> Whoa. Why do you inherit from mediaControlsApple.js? I'm almost certainly going to break you at some point due to changes in our design. A major goal of having the controls done in script rather than C++ was to separate the port-specific code into something more manageable, without having this extremely complex inheritance between the ports.

IMHO, style is something and code is a different thing. From my POV, creating the controls and the way they behave is common in most cases and I think it should be shared among different ports. That's the main reason why I decided to inherit from Apple controls, to share as much code as possible as the WebKit spirit suggests.

The inheritance is not complex, IMHO. Actually, the design is quite extensible and that's why inheriting for  the specifics was not complicated.

> I don't think mediaControlsPortA should ever be concerned that a change will break mediaControlsPortB. 

That's something I can agree up to a certain point (and of course I considered the idea when beginning the GTK JS controls, though I disregarded it for the reasons I state in this comment), but there's a gray area in where you can want to set the border. IMHO, style is one thing that I perfectly understand, but behavior should be shared as much as possible.

> As for the tests... yeah, I definitely need to make sure I don't break that. Although again, I can imagine a comprehensive redesign of the Apple controls that would break a lot of tests no matter what.

As this is a different story, I suggest we have a look at this when the problem appears.

> I think maybe what I should do is copy mediaControlsApple.js to mediaControlsGTKBase.js (or something), then make the changes.

I really think it is not necessary because of the reasons I stated above. I am subscribed to the changes and that's why I detected that this patch was a potential risk for the GTK port (and others in the future), not only because of the changes in the JS code that can be easily solved with my suggestions without having to separate the whole JS codebase, but for the other affected parts.

With the new JS controls I didn't have the chance to solve any crossplatform bug, I already tweaked some bits in the code that improved minimally Apple port and I actually remember solving some crossplatform bug with the old C++ controls. Keeping a common codebase helps everybody more in long term.

[Sam Weinig]

Comment on attachment 229845 [details]
Patch

r- due to bot breakage.