1319353 - CreateCSSNode() contexts don't match classes on ancestor nodes

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

This can be tested with the following in ~/.config/gtk-3.0/gtk.css

tooltip.background label {padding: 100px;}

I think tooltip and its box are currently the only contexts having style
classes, constructed from a path, and having descendant nodes from CreateCSSNode().  However, this may become more important as CreateCSSNode is used more.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1319353 CreateCSSNode: Copy classes from the parent style context to its corresponding node in the path

This is necessary for GTK to match CSS rules against classes on ancestor nodes
of style contexts constructed from paths.

It can be tested with the following in ~/.config/gtk-3.0/gtk.css

tooltip.background label {padding: 100px;}

Review commit: https://reviewboard.mozilla.org/r/94598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94598/

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8813069 [details]
bug 1319353 CreateCSSNode: Copy classes from the parent style context to its corresponding node in the path

https://reviewboard.mozilla.org/r/94598/#review94882

Good, Thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60a3dc6b2c6a
CreateCSSNode: Copy classes from the parent style context to its corresponding node in the path r=stransky+263117

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/60a3dc6b2c6a

Comment 6

[:Gijs (he/him)]

Would it be a good idea to uplift this to 52?

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8813069 [details]
bug 1319353 CreateCSSNode: Copy classes from the parent style context to its corresponding node in the path

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1333746
[User impact if declined]: Tooltips are unreadable on KDE & Gtk < 3.20
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Install any distro with KDE & GTK < 3.20
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Copy classes from parent style should not introduce any regression in styling.
[String changes made/needed]: none

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 8813069 [details]
bug 1319353 CreateCSSNode: Copy classes from the parent style context to its corresponding node in the path

gtk styling fix, beta52+

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ab3c86e35b57

Comment 10

[Karl Tomlinson (:karlt)]

Thank you for uplift and request.

Comment 11

[Andrei Vaida [:avaida]]

Flagging this for verification per Comment 7. 

Karl, Martin, I'm thinking that exploratory testing on general UI might help here -- other than tooltips, is there anything else we should focus on for this particular fix?

Comment 12

[Martin Stránský [:stransky] (ni? me)]

CreateCSSNode() is more or less used to style all elements like scrollbars, frames, text views, check/radio buttons and so. I think a general check of all possible UI elements would be good. I use https://bugzilla.mozilla.org/query.cgi?query_format=advanced for UI testing.

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #11)
> Karl, Martin, I'm thinking that exploratory testing on general UI might help
> here -- other than tooltips, is there anything else we should focus on for
> this particular fix?

What Martin said, but also different GTK themes may interact differently with these code changes.
The key themes to check would be the default themes in Fedora and Ubuntu, but, if known, the themes in use in the duplicate bugs could be used to verify those as duplicates.

Comment 15

[roxleitan]

FF Nightly 53.0a1 (Build ID  20161121074938)
FF Release 51.0.1 (Build ID: 20170125094131)
FF Beta    52.0b2 (Build ID: 20170130065342)

Hi,
I tested this bug on the above Firefox versions and could not reproduce it. 
I have installed:

with gtk 3.18
$ dpkg-query -W libgtk-3-bin
libgtk-3-bin    3.18.9-1ubuntu3.1
and 
KDE Plasma version: 5.5.5
Qt Version 5.5.1
Kernel Version 4.4.0.-57-generic
Ubuntu 16.04 x64


@Karl, Martin, any thoughts on this?

Comment 16

[Martin Stránský [:stransky] (ni? me)]

I used Fedora 21 + KDE + Gtk 3.14.15 to reproduce that. See details on Bug 1333746.

Comment 17

[roxleitan]

Hi Martin,

Since I don't have Fedora 21, could you please try to reproduce this issue using Firefox 52.0b2: http://archive.mozilla.org/pub/firefox/candidates/52.0-candidates/build2/.

Thanks.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Sorry I don't have that anymore. You can use any distro with KDE + Gtk 3.14, for instance Ubuntu 15.04.

Comment 19

[crxssi]

(In reply to [e-mail] from comment #17)

> Since I don't have Fedora 21, could you please try to reproduce this issue
> using Firefox 52.0b2:
> http://archive.mozilla.org/pub/firefox/candidates/52.0-candidates/build2/.

I just verified using that build under Mageia 5 + KDE 4.14 and the problem is now solved!

Comment 20

[Andrei Vaida [:avaida]]

Removing the qe-verify flag since this bug has been verified on Fx52, which has now been released.

Martin, thank you for helping us out :).

Comment 21

[crxssi]

Will this be fixed in Firefox 52 ESR?  It is broken in 52.0.2 and it makes the ESR unusable on LOTS of systems with no workaround.

Comment 22

[:Gijs (he/him)]

(In reply to crxssi from comment #21)
> Will this be fixed in Firefox 52 ESR?  It is broken in 52.0.2 and it makes
> the ESR unusable on LOTS of systems with no workaround.

This is marked as fixed in 52, so if this isn't actually fixed for you you should file a separate bug with more details about why the fix is insufficient...

Comment 23

[crxssi]

(In reply to :Gijs from comment #22)
> (In reply to crxssi from comment #21)
> > Will this be fixed in Firefox 52 ESR?  It is broken in 52.0.2 and it makes
> > the ESR unusable on LOTS of systems with no workaround.
> 
> This is marked as fixed in 52, so if this isn't actually fixed for you you
> should file a separate bug with more details about why the fix is
> insufficient...

Thanks for the response.  52.0.2ESR has same problem in Fedora 20, but it is working correctly in Mageia 5 (I just checked now).  I just created a new bug 1353552 as requested.  It might affect Centos 6, but I have not checked yet.