Bug 208002 - [WPE] Use custom theme style for media controls
Summary: [WPE] Use custom theme style for media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-20 05:10 PST by Carlos Garcia Campos
Modified: 2020-02-21 03:05 PST (History)
13 users (show)

See Also:


Attachments
Patch (41.11 KB, patch)
2020-02-20 05:45 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-02-20 05:10:40 PST
Instead of using the base one we currently use.
Comment 1 Carlos Garcia Campos 2020-02-20 05:45:08 PST
Created attachment 391280 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2020-02-20 09:28:51 PST
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).
Comment 3 Xabier Rodríguez Calvar 2020-02-20 09:31:41 PST
Comment on attachment 391280 [details]
Patch

preventive r- because I would like some answers first
Comment 4 Xabier Rodríguez Calvar 2020-02-20 09:32:25 PST
Another thing is that if you move to fullscreen, I see no controls (I tried http://camendesign.com/code/video_for_everybody/test.html ).
Comment 5 Adrian Perez 2020-02-20 09:42:21 PST
(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.
Comment 6 Carlos Garcia Campos 2020-02-20 23:02:21 PST
(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.
Comment 7 Carlos Garcia Campos 2020-02-20 23:03:40 PST
(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.
Comment 8 Carlos Garcia Campos 2020-02-20 23:06:00 PST
(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.
Comment 9 Carlos Garcia Campos 2020-02-21 02:26:42 PST
Committed r257131: <https://trac.webkit.org/changeset/257131>