Closed Bug 1319353 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Widget: Gtk, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox52 --- verified
firefox53 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #8813069 - Flags: review?(stransky)
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!
Attachment #8813069 - Flags: review?(stransky) → review+
Pushed by ktomlinson@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/60a3dc6b2c6a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1333746
Duplicate of this bug: 1333746
Would it be a good idea to uplift this to 52?
Flags: needinfo?(karlt)
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
Attachment #8813069 - Flags: approval-mozilla-beta?
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+
Attachment #8813069 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thank you for uplift and request.
Flags: needinfo?(karlt)
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?
Flags: qe-verify+
Flags: needinfo?(stransky)
Flags: needinfo?(karlt)
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.
Flags: needinfo?(stransky)
Duplicate of this bug: 1336748
(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.
Flags: needinfo?(karlt)
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?
Flags: needinfo?(karlt)
Flags: needinfo?(stransky)
I used Fedora 21 + KDE + Gtk 3.14.15 to reproduce that. See details on Bug 1333746.
Flags: needinfo?(stransky)
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.
Flags: needinfo?(stransky)
Sorry I don't have that anymore. You can use any distro with KDE + Gtk 3.14, for instance Ubuntu 15.04.
Flags: needinfo?(stransky)
(In reply to roxana.leitan@softvision.ro 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!
Status: RESOLVED → VERIFIED
Flags: needinfo?(karlt)
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 :).
Flags: qe-verify+
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.
(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...
(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.
Depends on: 1353552
You need to log in before you can comment on or make changes to this bug.