Closed
Bug 1310850
Opened 8 years ago
Closed 8 years ago
Missing scrollbars under GTK
Categories
(Core :: Widget: Gtk, defect, P1)
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.
Reporter | ||
Comment 1•8 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•8 years ago
|
||
Thanks for let me know. Do you find it reproducible (ie on some specific web page)?
Comment 3•8 years ago
|
||
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•8 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•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
Reporter | ||
Comment 7•8 years ago
|
||
HOLD ON. I cannot duplicate this issue in safe mode. Let me see what is causing this.
Reporter | ||
Comment 8•8 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•8 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 10•8 years ago
|
||
Reporter | ||
Comment 11•8 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•8 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•8 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•8 years ago
|
||
Attachment #8802153 -
Flags: review?(karlt)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(fred.wang)
Reporter | ||
Comment 15•8 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•8 years ago
|
Assignee: nobody → jhorak
Status: NEW → ASSIGNED
Comment 16•8 years ago
|
||
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+
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: tpi:+
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 17•8 years ago
|
||
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•8 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•8 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 20•8 years ago
|
||
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+
Reporter | ||
Comment 22•8 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•8 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.
Assignee | ||
Comment 25•8 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•8 years ago
|
||
OIC. The thumb does descend from the scrollbar, it is just not a direct descendant.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 27•8 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 28•8 years ago
|
||
backout for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=38166211&repo=mozilla-inbound
Flags: needinfo?(jhorak)
Comment 29•8 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•8 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•8 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+
Assignee | ||
Updated•8 years ago
|
Attachment #8802851 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 33•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a035c4acf60
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•