Bug 208002 – [WPE] Use custom theme style for media controls

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Carlos Garcia Campos]

Instead of using the base one we currently use.

[Carlos Garcia Campos]

Created attachment 391280 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 391280 [details]
Patch

Why are you not using GTK ones and just extend the CSS to include the icons?

Another thing is that I am no CSS expert and there are probably several things there that can be improved (I see too many !important).

Code seems solid but the other approach seems more logical to me (bonus points would be collapsing base and gtk CSS and JS files in just one. You could name it WPE, actually).

[Xabier Rodríguez Calvar]

Comment on attachment 391280 [details]
Patch

preventive r- because I would like some answers first

[Xabier Rodríguez Calvar]

Another thing is that if you move to fullscreen, I see no controls (I tried http://camendesign.com/code/video_for_everybody/test.html ).

[Adrian Perez]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 391280 [details]
> Patch
> 
> Why are you not using GTK ones and just extend the CSS to include the icons?

The reason is that the plan is to switch GTK to also use the new theming
code which we are landing piecewise for WPE. There will be some PLATFORM(GTK)
guards for a few things like getting background/foreground/selection/etc.
colors and so on; but we would like to get rid of RenderThemeGTK at some
point (which is one of the trickiest “here be dragons” pieces of code of
the GTK port).

> Another thing is that I am no CSS expert and there are probably several
> things there that can be improved (I see too many !important).

So far its only copied with this minor to make sure things keep working
exactly the same once the GTK port starts using the same theming code as
the WPE port.

Improvements can be made later on, I'd say.

> Code seems solid but the other approach seems more logical to me (bonus
> points would be collapsing base and gtk CSS and JS files in just one. You
> could name it WPE, actually).

I think collapsing Base+Adwaita cannot be done because the PlayStation
port uses the Base ones without any additions, so we will need to keep
that split.

[Carlos Garcia Campos]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 391280 [details]
> Patch
> 
> Why are you not using GTK ones and just extend the CSS to include the icons?

Because the gtk one is going to die soon, so I copy-pasted them, added the icons and made some improvements.
 
> Another thing is that I am no CSS expert and there are probably several
> things there that can be improved (I see too many !important).

Sure, I have no idea about CSS, I copy-pasted the gtk css and followed what is there.

> Code seems solid but the other approach seems more logical to me (bonus
> points would be collapsing base and gtk CSS and JS files in just one. You
> could name it WPE, actually).

GTK doesn't use the base css file, and WPE either. In the case of JS it makes sense to have a base impl, to avoid duplicating code and other ports are using only the base one, so we can't delete it.

[Carlos Garcia Campos]

(In reply to Xabier Rodríguez Calvar from comment #4)
> Another thing is that if you move to fullscreen, I see no controls (I tried
> http://camendesign.com/code/video_for_everybody/test.html ).

Worked here with https://www.w3.org/2010/05/video/mediaevents.html I'll try again, maybe I broke it somehow.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Xabier Rodríguez Calvar from comment #4)
> > Another thing is that if you move to fullscreen, I see no controls (I tried
> > http://camendesign.com/code/video_for_everybody/test.html ).
> 
> Worked here with https://www.w3.org/2010/05/video/mediaevents.html I'll try
> again, maybe I broke it somehow.

It works for me in http://camendesign.com/code/video_for_everybody/test.html too.

[Carlos Garcia Campos]

Committed r257131: <https://trac.webkit.org/changeset/257131>