1343802 - [GTK 3.20] Excessive (fat) scrollbar width KDE breeze GTK theme on Firefox >= 52 (beta)

Status: RESOLVED FIXED

Description

[Rik Mills]

Created attachment 8842774 [details]
96cd277e4f.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170301084303

Steps to reproduce:

1. Switch to Firefox v52 (beta), developer v53 or nightly v54.
2. Using breeze GTK3 theme in KDE plasma desktop.
3. GTK3 version is 3.20 or greater (tested minimum = 3.20.6)

Confirmed on, but not limited to:

- kubuntu zesty 17.04 -  GTK 3.22.7
- kubuntu yakkety 16.10 - GTK 3.20.9

&

- chakra latest packages - GTK 3.20.6

Unaffected:

- Neon developer edition - GTK 3.18.9


Actual results:

The themes scrollbars in Firefox were rendered with excessive width and little trough. See attached screenshot.

This is a regression from Firefox 50 where these scrollbars render as intended in all GTK versions.


Expected results:

The scrollbars should have rendered with the width intended by the theme, as they do correctly in Firefox 50 and in GTK 3.18.

This seems to be a mozilla specific issue, but for reference a link to the KDE bug report is:

https://bugs.kde.org/show_bug.cgi?id=377008

Comment 1

[Karl Tomlinson (:karlt)]

Prior to GTK 3.20, the trough was typically the width of the scrollbar/range
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrange.c?h=3.18.9#n3889
and the range width was calculated by adding trough-border to slider-width
(for the thumb)
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrange.c?h=3.18.9#n3513
Steppers did not influence the width the scrollbar, but their width was
derived from the range width.
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrange.c?h=3.18.9#n3805

Gecko's scrollbar is sized to accommodate the width of the steppers and the
trough.  The steppers and trough then stretch to fill available width inside
the scrollbar border.  The thumb stretches to fills the width of the slider
inside its border.  This model worked with GTK < 3.20 because the steppers
were not wider than the trough.

In GTK 3.20, the steppers and the scrollbar can have their own minimum sizes,
independent of the trough.  The trough gadget does not fill available space
but is centered across the minor axis of the scrollbar:
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrange.c?h=3.20.9#n822

Similarly the slider (thumb) gadget does not fill available space but is
centered across the trough:
https://git.gnome.org/browse/gtk+/tree/gtk/gtkrange.c?h=3.20.9#n3513

Changes for bug 1289148 added consideration of GTK 3.20 thumb and scrollbar
minimum sizes (which made the Breeze scrollbar the correct width), but didn't
make appropriate changes to limit the widths of the trough and the thumb.

Comment 2

[rafjaimes]

I can confirm this is also happening for me.

I'm using openSUSE 42.1 Leap and Firefox 52.0.1. My GTK3 version is 3.16 and I'm on the KDE Plasma 5 desktop.

If you require anything additional, please let me know.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to rafjaimes from comment #2)
> I'm using openSUSE 42.1 Leap and Firefox 52.0.1. My GTK3 version is 3.16 and
> I'm on the KDE Plasma 5 desktop.

Please compare rendering with GTK_OVERLAY_SCROLLING=0 gtk3-demo.
If rendering is the same then you may want bug 1147847.

If not, then please file a separate report, as changes here will not fix an issue with GTK 3.16.
Please include in that report an indication of where to obtain the GTK theme involved.

Comment 4

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1defc4f86e2b3c56aae467e44837ce7ecf482128

Comment 5

[Karl Tomlinson (:karlt)]

Created attachment 8850363 [details]
bug 1343802 remove unused moz_gtk_enable_style_props

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

Comment 6

[Karl Tomlinson (:karlt)]

Created attachment 8850364 [details]
bug 1343802 remove code for native scrollbar thumb border sizes

There is no need to calculate thumb borders because thumb border-box sizes are
determined with GetMinimumWidgetSize, which includes GTK margin, border, and
padding, and the interior border width is irrelevant because thumbs have no
children.

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

Comment 7

[Karl Tomlinson (:karlt)]

Created attachment 8850365 [details]
bug 1343802 calculate scrollbar metrics in one place

Changes in behavior are intended to be minimal, but this adds distinct
metrics for horizontal and vertical scrollbars even with GTK versions < 3.20.

Updates on theme changes will be restored in a subsequent patch.

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

Comment 8

[Karl Tomlinson (:karlt)]

Created attachment 8850366 [details]
bug 1343802 remove moz_gtk_get_scrollbar_widget()

This was used with GTK2, but is now unnecessary as discussed in bug 1278282.

moz_gtk_init() is now called from only one place and so will be called only
once.

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

Comment 9

[Karl Tomlinson (:karlt)]

Created attachment 8850367 [details]
bug 1343802 remove have_arrow_scaling, which was always true

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

Comment 10

[Karl Tomlinson (:karlt)]

Created attachment 8850368 [details]
bug 1343802 update notebook_has_tab_gap and scrollbar metrics after theme changes

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

Comment 11

[Karl Tomlinson (:karlt)]

Created attachment 8850369 [details]
bug 1343802 adjust scrollbar track border to prevent thumb expanding to fill available breadth +328198

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

Comment 12

[Karl Tomlinson (:karlt)]

Created attachment 8850370 [details]
bug 1343802 swap vertical scrollbar button dimensions for horizontal scrollbars

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

Comment 13

[Karl Tomlinson (:karlt)]

Created attachment 8850371 [details]
bug 1343802 consider scrollbar trough min-width/min-height properties

in determining breadth of trough and scrollbar.

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

Comment 14

[Karl Tomlinson (:karlt)]

Created attachment 8850372 [details]
bug 1343802 draw trough centered instead of filling the scrollbar

to follow the behavior of version 3.20 GtkRange's contents_gadget.

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

Comment 15

[Jan Horak [:jhorak]]

Comment on attachment 8850363 [details]
bug 1343802 remove unused moz_gtk_enable_style_props

https://reviewboard.mozilla.org/r/122930/#review125856

Comment 16

[Jan Horak [:jhorak]]

Comment on attachment 8850372 [details]
bug 1343802 draw trough centered instead of filling the scrollbar

https://reviewboard.mozilla.org/r/122948/#review125858

Comment 17

[Jan Horak [:jhorak]]

Comment on attachment 8850371 [details]
bug 1343802 consider scrollbar trough min-width/min-height properties

https://reviewboard.mozilla.org/r/122946/#review125862

Comment 18

[Jan Horak [:jhorak]]

Comment on attachment 8850370 [details]
bug 1343802 swap vertical scrollbar button dimensions for horizontal scrollbars

https://reviewboard.mozilla.org/r/122944/#review125864

Comment 19

[Jan Horak [:jhorak]]

Comment on attachment 8850368 [details]
bug 1343802 update notebook_has_tab_gap and scrollbar metrics after theme changes

https://reviewboard.mozilla.org/r/122940/#review125868

Comment 20

[Jan Horak [:jhorak]]

Comment on attachment 8850367 [details]
bug 1343802 remove have_arrow_scaling, which was always true

https://reviewboard.mozilla.org/r/122938/#review125872

Comment 21

[Jan Horak [:jhorak]]

Comment on attachment 8850366 [details]
bug 1343802 remove moz_gtk_get_scrollbar_widget()

https://reviewboard.mozilla.org/r/122936/#review125874

Comment 22

[Jan Horak [:jhorak]]

Comment on attachment 8850364 [details]
bug 1343802 remove code for native scrollbar thumb border sizes

https://reviewboard.mozilla.org/r/122932/#review125878

Comment 23

[Jan Horak [:jhorak]]

Comment on attachment 8850369 [details]
bug 1343802 adjust scrollbar track border to prevent thumb expanding to fill available breadth +328198

https://reviewboard.mozilla.org/r/122942/#review125882

Comment 24

[Jan Horak [:jhorak]]

Comment on attachment 8850365 [details]
bug 1343802 calculate scrollbar metrics in one place

https://reviewboard.mozilla.org/r/122934/#review125888

Thanks for looking into it. That was calling for real refactoring long time.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc81d40f3290
remove unused moz_gtk_enable_style_props r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/5dc7564d651b
remove code for native scrollbar thumb border sizes r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/8c45140a2819
calculate scrollbar metrics in one place r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/af279778b154
remove moz_gtk_get_scrollbar_widget() r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/88ea23ae0627
remove have_arrow_scaling, which was always true r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/45330fa58671
update notebook_has_tab_gap and scrollbar metrics after theme changes r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/9abd87cd2dff
adjust scrollbar track border to prevent thumb expanding to fill available breadth r=jhorak+328198+328198
https://hg.mozilla.org/integration/autoland/rev/7abbf6d092eb
swap vertical scrollbar button dimensions for horizontal scrollbars r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/22f4dc6bb4db
consider scrollbar trough min-width/min-height properties r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/db84a68eaaec
draw trough centered instead of filling the scrollbar r=jhorak+328198

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc81d40f3290
https://hg.mozilla.org/mozilla-central/rev/5dc7564d651b
https://hg.mozilla.org/mozilla-central/rev/8c45140a2819
https://hg.mozilla.org/mozilla-central/rev/af279778b154
https://hg.mozilla.org/mozilla-central/rev/88ea23ae0627
https://hg.mozilla.org/mozilla-central/rev/45330fa58671
https://hg.mozilla.org/mozilla-central/rev/9abd87cd2dff
https://hg.mozilla.org/mozilla-central/rev/7abbf6d092eb
https://hg.mozilla.org/mozilla-central/rev/22f4dc6bb4db
https://hg.mozilla.org/mozilla-central/rev/db84a68eaaec

Comment 27

[Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)]

Backed out for likely causing bug 1350875 and reviving bug 1332040:

https://hg.mozilla.org/integration/autoland/rev/eeabb6e839e4c6abd473bb95c188fe3ea5995344
https://hg.mozilla.org/integration/autoland/rev/764ff98e83fce4d52d866751d5fb36c8e61c2cad
https://hg.mozilla.org/integration/autoland/rev/7f86a3c1f65d5677248cfebee1afe3927de7fdae
https://hg.mozilla.org/integration/autoland/rev/cd0123859db70d7054640336839b285374eb58cb
https://hg.mozilla.org/integration/autoland/rev/41ea7f3b8d16291acd0dc8b682f6e6f3242e9a39
https://hg.mozilla.org/integration/autoland/rev/d55285edcc49009c42eab6d03735138fc9775d9e
https://hg.mozilla.org/integration/autoland/rev/f387dcfd940fc00c1d47317b2d873ac0a33fb579
https://hg.mozilla.org/integration/autoland/rev/01c689ae86dabd1941e96766678a708af5dde7d2
https://hg.mozilla.org/integration/autoland/rev/133d02c94803be19b6134912172bb14eb0901c17
https://hg.mozilla.org/integration/autoland/rev/b099c950706cb62f237c06844d887399d0850463

An affected push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=88c9c2bcfaf9714e4194ad92d1a7d8075cfda738&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86612972&repo=autoland
Failure list: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1350875
[task 2017-03-27T11:15:46.255915Z] 11:15:46     INFO - TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar 
[task 2017-03-27T11:15:46.258167Z] 11:15:46     INFO - TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar 
[task 2017-03-27T11:15:46.263601Z] 11:15:46     INFO - [scrollbar] Entering fullscreen on root
[task 2017-03-27T11:15:46.265653Z] 11:15:46     INFO - Buffered messages logged at 11:15:44
[task 2017-03-27T11:15:46.271335Z] 11:15:46     INFO - TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should only check the current fullscreen element 
[task 2017-03-27T11:15:46.273520Z] 11:15:46     INFO - Buffered messages finished
[task 2017-03-27T11:15:46.276094Z] 11:15:46     INFO - TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 487, expected 1587
[task 2017-03-27T11:15:46.278071Z] 11:15:46     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-03-27T11:15:46.279952Z] 11:15:46     INFO -     is@dom/html/test/file_fullscreen-scrollbar.html:30:25
[task 2017-03-27T11:15:46.281999Z] 11:15:46     INFO -     assertHasScrollbars@dom/html/test/file_fullscreen-scrollbar.html:54:3
[task 2017-03-27T11:15:46.284165Z] 11:15:46     INFO -     checkScrollbars@dom/html/test/file_fullscreen-scrollbar.html:73:3
[task 2017-03-27T11:15:46.288019Z] 11:15:46     INFO -     enteredFullscreenOnRoot@dom/html/test/file_fullscreen-scrollbar.html:100:3
[task 2017-03-27T11:15:46.290035Z] 11:15:46     INFO -     invokeCallback/</<@dom/html/test/file_fullscreen-utils.js:54:50
[task 2017-03-27T11:15:46.292265Z] 11:15:46     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:672:12
[task 2017-03-27T11:15:46.294219Z] 11:15:46     INFO -     invokeCallback/<@dom/html/test/file_fullscreen-utils.js:54:33
[task 2017-03-27T11:15:46.298079Z] 11:15:46     INFO -     FrameRequestCallback*invokeCallback@dom/html/test/file_fullscreen-utils.js:54:5
[task 2017-03-27T11:15:46.300260Z] 11:15:46     INFO -     onFullscreenChange@dom/html/test/file_fullscreen-utils.js:59:7
[task 2017-03-27T11:15:46.302220Z] 11:15:46     INFO -     EventListener.handleEvent*addFullscreenChangeContinuation@dom/html/test/file_fullscreen-utils.js:70:3
[task 2017-03-27T11:15:46.304092Z] 11:15:46     INFO -     begin@dom/html/test/file_fullscreen-scrollbar.html:95:3
[task 2017-03-27T11:15:46.306051Z] 11:15:46     INFO -     focusedOrLoaded/<@SimpleTest/SimpleTest.js:791:59

Please also have a look at this timeout in the same test: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1332040
They all only affect Linux.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/858dfd6b90fc
remove unused moz_gtk_enable_style_props r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/9d2a1059675c
remove code for native scrollbar thumb border sizes r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/6bb2674c5ecb
calculate scrollbar metrics in one place r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/9181b8953b46
remove moz_gtk_get_scrollbar_widget() r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/5f31d05b6a70
remove have_arrow_scaling, which was always true r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/0c9392847e7f
update notebook_has_tab_gap and scrollbar metrics after theme changes r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/ccb8ca1eecd1
adjust scrollbar track border to prevent thumb expanding to fill available breadth r=jhorak+328198+328198
https://hg.mozilla.org/integration/autoland/rev/ec09b72a0975
swap vertical scrollbar button dimensions for horizontal scrollbars r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/627ff46c8f16
consider scrollbar trough min-width/min-height properties r=jhorak+328198
https://hg.mozilla.org/integration/autoland/rev/84c553eab3d8
draw trough centered instead of filling the scrollbar r=jhorak+328198

Comment 29

[Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)]

Relanded it. The failures seems to be regressions from bug 1194751. Sorry for the adrenalin rush.

Comment 30

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/858dfd6b90fc
https://hg.mozilla.org/mozilla-central/rev/9d2a1059675c
https://hg.mozilla.org/mozilla-central/rev/6bb2674c5ecb
https://hg.mozilla.org/mozilla-central/rev/9181b8953b46
https://hg.mozilla.org/mozilla-central/rev/5f31d05b6a70
https://hg.mozilla.org/mozilla-central/rev/0c9392847e7f
https://hg.mozilla.org/mozilla-central/rev/ccb8ca1eecd1
https://hg.mozilla.org/mozilla-central/rev/ec09b72a0975
https://hg.mozilla.org/mozilla-central/rev/627ff46c8f16
https://hg.mozilla.org/mozilla-central/rev/84c553eab3d8

Comment 32

[javascriptjedi]

It's fixed in FF55+ only??? Will a fix be ported for FF52ESR which is just starting its run?  Otherwise it will be broken for its entire lifespan.  Reproduces on OpenSUSE 42.2, KDE 5.  That's an LTS release, so it probably won't get the GTK update which is the alternative fix.

Please fix in FF52ESR!  Thanks.