Closed Bug 1310850 Opened 4 years ago Closed 4 years ago

Missing scrollbars under GTK

Categories

(Core :: Widget: Gtk, defect, P1)

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: wgianopoulos, Assigned: jhorak)

References

Details

(Keywords: regression, Whiteboard: tpi:+)

Attachments

(6 files, 2 obsolete files)

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.
I should have mentioned that this is under fedora24 with the default Adwaita theme and gtk version 3.20.9.
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.
(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.
HOLD ON. I cannot duplicate this issue in safe mode.  Let me see what is causing this.
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)
(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.
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.
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.
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.
Attached patch fix-sb-0-size.patch (obsolete) — Splinter Review
Attachment #8802153 - Flags: review?(karlt)
Flags: needinfo?(fred.wang)
(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.
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
Attached 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)
(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.
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
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?
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
Duplicate of this bug: 1312263
(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.
OIC.  The thumb does descend from the scrollbar, it is just not a direct descendant.
Keywords: checkin-needed
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
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/564e554ef491
Backed out changeset 67c74a5cfcda for bustage
(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.
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
Attachment #8802851 - Attachment is obsolete: true
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3a035c4acf60
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.