Missing scrollbars under GTK

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wgianopoulos, Assigned: jhorak)

Tracking

({regression})

Trunk
mozilla52
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

3 years ago
Scrollbars are often missing on webpages since the landing of bug 1289148.  I identified that bug as the regressor via backout.  Things which exhibit this issue are Google search results, viewing bugs in bugzilla and Facebook timeline among others.
Reporter

Comment 1

3 years ago
I should have mentioned that this is under fedora24 with the default Adwaita theme and gtk version 3.20.9.
Assignee

Comment 2

3 years ago
Thanks for let me know. Do you find it reproducible (ie on some specific web page)?
Don't know if this is the same underlying issue, but just recently my Firefox Nightly's scroll bars on Linux Mint got fatter.
Reporter

Comment 4

3 years ago
(In reply to Jan Horak from comment #2)
> Thanks for let me know. Do you find it reproducible (ie on some specific web
> page)?

The page you are looking at to view/edit this bug is missing the vertical scrollbar for me.  I have tried with both the default and the Dusk Bugzilla theme with no difference.
Reporter

Comment 5

3 years ago
Reporter

Comment 7

3 years ago
HOLD ON. I cannot duplicate this issue in safe mode.  Let me see what is causing this.
Reporter

Comment 8

3 years ago
This seems to be a bad interaction between these patches and the MathML-fonts extension.  I am needino-ing the patch author.
Flags: needinfo?(fred.wang)
Reporter

Comment 9

3 years ago
(In reply to Bill Gianopoulos [:WG9s] from comment #8)
> This seems to be a bad interaction between these patches and the
> MathML-fonts extension.  I am needino-ing the patch author.
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                needinfo-ing the extension author.
Reporter

Comment 11

3 years ago
That all said, the preferences page for add-ons does not display a vertical scrollbar with this patch applied even whenn running in safe-mode.
Reporter

Comment 12

3 years ago
So, it seems disabling the MathML-fonts add-on "fixes" the issue on bugzilla pages and google search results yet we still have the add-ons extensions page and Facebook not displaying the vertical scrollbar, even when in safe-mode.
Assignee

Comment 13

3 years ago
I can reproduce that also. It seems, that: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/widget/gtk/nsNativeThemeGTK.cpp#1466 is also required for GTK 3.20. It's my fault I've not properly tested adwaita theme and claimed the opposite in bug 1289148 to karl.
Assignee

Comment 14

3 years ago
Posted patch fix-sb-0-size.patch (obsolete) — Splinter Review
Attachment #8802153 - Flags: review?(karlt)
Reporter

Updated

3 years ago
Flags: needinfo?(fred.wang)
Reporter

Comment 15

3 years ago
(In reply to Jan Horak from comment #14)
> Created attachment 8802153 [details] [diff] [review]
> fix-sb-0-size.patch

This works for me!  Thanks for fixing so quickly.
Reporter

Updated

3 years ago
Assignee: nobody → jhorak
Status: NEW → ASSIGNED
Comment on attachment 8802153 [details] [diff] [review]
fix-sb-0-size.patch

The borders of the scrollbar and track should be included here, but let's start with this.  Please add a TODO comment for that.

(If specifying the minimum size on the track instead of the scrollbar works, that that may be simpler.)

>       if (gtk_check_version(3,20,0) == nullptr) {
>-          moz_gtk_get_widget_min_size(NativeThemeToGtkTheme(aWidgetType, aFrame),
>-                                      &(aResult->width), &(aResult->height));
>+          if (aWidgetType == NS_THEME_SCROLLBAR_VERTICAL) {
>+            moz_gtk_get_widget_min_size(MOZ_GTK_SCROLLBAR_THUMB_VERTICAL,
>+                                        &(aResult->width), &(aResult->height));
>+          } else {
>+            moz_gtk_get_widget_min_size(MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL,
>+                                        &(aResult->width), &(aResult->height));
>+          }

Please use only a single call to moz_gtk_get_widget_min_size() by adding
        WidgetNodeType thumbType = aWidgetType == NS_THEME_SCROLLBAR_VERTICAL ?
          MOZ_GTK_SCROLLBAR_THUMB_VERTICAL : MOZ_GTK_SCROLLBAR_THUMB_HORIZONTAL;

with two-space indentation.
Attachment #8802153 - Flags: review?(karlt) → review+
Priority: -- → P2
Whiteboard: tpi:+
Priority: P2 → P1
Assignee

Comment 17

3 years ago
Posted patch fix-sb-0-size.patch (obsolete) — Splinter Review
Thanks for the review, I've actually did the changes you mentioned as TODO. Tested with Adwaita and Traditional theme. Looks good. Asking for review again, because of the changes.
Attachment #8802153 - Attachment is obsolete: true
Attachment #8802851 - Flags: review?(karlt)
Reporter

Comment 18

3 years ago
(In reply to Cameron McCormack (:heycam) from comment #3)
> Created attachment 8802040 [details]
> screen shot of fat scroll bars
> 
> Don't know if this is the same underlying issue, but just recently my
> Firefox Nightly's scroll bars on Linux Mint got fatter.

1.  It is not clear that this is an issue.  The issue in bug 1289148 was that the width of scrollbars from the system theme was not being respected and a Mozilla default width was being used instead.  Although it was described as scrollbars are wider than specified by the system theme the opposite of scrollbars are narrower than the default theme could also have been both fixed by the patch in bug 1289148.  If the patch here fixes your issue then I guess it is the same issue.  If not, then either you have a new issue or you are just seeing Firefox respect your system theme settings which results in Firefox displaying the proper width scrollbars according to your system theme choice.
Reporter

Comment 19

3 years ago
BTW you can download a build from http://www.wg9s.com/mozilla/firefox/ to see if this patch fixes, or causes any issues.
Comment on attachment 8802851 [details] [diff] [review]
fix-sb-0-size.patch

Thanks.  The minimum major axis size shouldn't be set to include the thumb when there are scrollbar buttons, as the scrollbar is still useful with just buttons, and so shouldn't be hidden because there is not space for the thumb.  Zero would work fine for the major axis in that situation, and it could be zero even when there are no scrollbar buttons, as discussed in bug 1289148 comment 15.
But fine to land this patch as is, because it is a good step in the right direction.
Attachment #8802851 - Flags: review?(karlt) → review+
Duplicate of this bug: 1311646
Reporter

Comment 22

3 years ago
I think the real issue here is pointed out here:

+      /* While we enforce a minimum size for the thumb, this is ignored
+       * for the some scrollbars if buttons are hidden (bug 513006) because
+       * the thumb isn't a direct child of the scrollbar, unlike the buttons
+       * or track.

So, exactly why is the thumb not a direct child of the scrollbar.  Seems to me we are wasting a lot of effort here trying to kludge around what is not the best original design decision.  Is this a Mozilla design decision or just stupidity in the specification?
Reporter

Comment 23

3 years ago
And don't get me wrong here.  Please land this patch as it finally gets scrollbars displaying correctly for me under GTK 2.20 and above.  It is just, after doing that, if there is no really good reason to not make the thumb descend from the scrollbar then perhaps that is a better path forward.
See Also: → 1312263
Reporter

Updated

3 years ago
Duplicate of this bug: 1312263
Assignee

Comment 25

3 years ago
(In reply to Bill Gianopoulos [:WG9s] from comment #22)
> I think the real issue here is pointed out here:
> 
> +      /* While we enforce a minimum size for the thumb, this is ignored
> +       * for the some scrollbars if buttons are hidden (bug 513006) because
> +       * the thumb isn't a direct child of the scrollbar, unlike the buttons
> +       * or track.
> 
> So, exactly why is the thumb not a direct child of the scrollbar.  Seems to
> me we are wasting a lot of effort here trying to kludge around what is not
> the best original design decision.  Is this a Mozilla design decision or
> just stupidity in the specification?
It's a bit more complicated, the tree is following:
scrollbar
  + button up
  + button down
  + slider
    + thumb
  + button up
  + button down
Some or all buttons can be hidden (depends on theme). Slider occupies the whole size of the scrollbar minus buttons. The tree in Mozilla is basically copying what gtk3 implements, so that's fine.
Reporter

Comment 26

3 years ago
OIC.  The thumb does descend from the scrollbar, it is just not a direct descendant.
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 27

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67c74a5cfcda
Zero scrollbar size could also happen when running GTK 3.20 so we need to set the minimum size of the scrollbar to the minimum size of a thumb. r=karlt
Keywords: checkin-needed

Comment 29

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/564e554ef491
Backed out changeset 67c74a5cfcda for bustage
Reporter

Comment 30

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #28)
> backout for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38166211&repo=mozilla-
> inbound
I have verified that removing those extraneous declarations results in a patch that still applies and fixes the issue.  I wonder why newer versions of gcc are no longer generating a warning on this case.
Assignee

Comment 31

3 years ago
Oh, sorry about that, should have done try run, here's the one with fixed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cf99652175d4a0b862564c0955a750536f7d53c
Flags: needinfo?(jhorak)
Attachment #8804388 - Flags: review+
Duplicate of this bug: 1312876
Assignee

Updated

3 years ago
Attachment #8802851 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 33

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a035c4acf60
Zero scrollbar size could also happen when running GTK 3.20 so we need to set the minimum size of the scrollbar to the minimum size of a thumb. r=karlt
Keywords: checkin-needed

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a035c4acf60
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.