Closed
Bug 1355143
Opened 7 years ago
Closed 6 years ago
GTK scrollbar sits in a track that's twice as thick (top half of which is non-functional)
Categories
(Core :: Widget: Gtk, defect, P1)
Core
Widget: Gtk
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: dholbert, Assigned: stransky)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: stale-bug, Whiteboard: tpi:+)
Attachments
(5 files)
181.10 KB,
video/ogg
|
Details | |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
jhorak
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
298.41 KB,
image/png
|
Details |
STR: 1. Visit https://bug1346961.bmoattachments.org/attachment.cgi?id=8846824 2. Look at scrollbar and its track at the bottom. Try to use that (particularly the top half), to scroll sideways. ACTUAL RESULTS: * The scrollbar's track is twice as thick as the scrollbar. * The top half of the track (the part not covered by the scrollbar) is non-functional. Clicks there have no effect.) EXPECTED RESULTS: * The full scrollbar track that we show should be functional/useful. (Bug 1346961 comment 17 through bug 1346961 comment 21 have some more context here. Before bug 1346961's patches landed, the non-functional piece of the track here was painted as black.)
Reporter | ||
Comment 1•7 years ago
|
||
Here's a screencast showing the issue. You can't tell from the screencast, but assume that I'm clicking (and dragging) pretty frequently. This has no effect on the top half of the scrollbar's track, which is the issue here.
Reporter | ||
Updated•7 years ago
|
Summary: Horizontal GTK scrollbar sits in a track that's twice as thick (half of which is non-functional) → Horizontal GTK scrollbar sits in a track that's twice as thick (top half of which is non-functional)
Reporter | ||
Comment 2•7 years ago
|
||
Right now I'm using Ubuntu 16.10 (latest Ubuntu), and I believe this affects Ubuntu 17.04 (released later this week) based on earlier testing I did on a prerelease of that platform. This does *not* affect Ubuntu 16.04 -- there, the scrollbar *grows* to fill the full track when I hover the scroll track, and the full scroll-track is clickable.
Reporter | ||
Comment 3•7 years ago
|
||
Also: on affected platforms, this is basically a regression from bug 1289148. (Before that bug landed, the scrollbar was the full thickness of its (then-larger) scroll track.)
Blocks: 1289148
Comment 4•7 years ago
|
||
Thank you, Daniel. I suspect the best fix here may be to change nsITheme::WidgetStateChanged to return an nsChangeHint. When the scrollbar hover changes it would return nsChangeHint_NeedReflow | nsChangeHint_ClearDescendantIntrinsics | nsChangeHint_ClearAncestorIntrinsics; Perhaps also some other flags. I don't know. The idea is to reflow and reposition the scrollbar descendants. Both the scrollbar and the descendants may have changes in borders or padding, but the scrollbar dimensions should not change. Please let me know if this is sounding crazy.
Priority: -- → P1
Updated•7 years ago
|
Whiteboard: tpi:+
Assignee | ||
Comment 5•7 years ago
|
||
Adding Jan as he does some scrollbar work recently.
Comment hidden (off-topic) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jhorak
Assignee | ||
Comment 8•6 years ago
|
||
It's caused by this css rule at Ambiance theme: .scrollbar.horizontal:not(:hover):not(.dragging), scrollbar.horizontal:not(:hover):not(.dragging) { margin-top: 7px; } Bug 1451792 is a variant of it but was not visible because horizontal scrollbars have defined direction it their style: .scrollbar.vertical:dir(ltr):not(:hover):not(.dragging), scrollbar.vertical:dir(ltr):not(:hover):not(.dragging) { margin-left: 7px; } .scrollbar.vertical:dir(rtl):not(:hover):not(.dragging), scrollbar.vertical:dir(rtl):not(:hover):not(.dragging) { margin-right: 7px; } so we didn't see that until direction was added at Bug 1433092.
Assignee | ||
Updated•6 years ago
|
Assignee: jhorak → stransky
Assignee | ||
Comment 9•6 years ago
|
||
Bug 1451792 is a variant of this bug. Looks like firefox snap package has this fixed somehow (Bug 1451792 comment 8). I can confirm it works somehow at the snap Firefox.
Assignee | ||
Comment 11•6 years ago
|
||
I'm thinking about some heuristic about the scrollbar size. We know that the hover size may be bigger that a normal one. We can check how big is the difference and maybe adjust the normal size (until we implement overlay scroll bars for Gtk).
Assignee | ||
Comment 12•6 years ago
|
||
It also means we need to carefully read style from GetStyleContext() - to make sure we have correct direction flags for the style.
Assignee | ||
Updated•6 years ago
|
Summary: Horizontal GTK scrollbar sits in a track that's twice as thick (top half of which is non-functional) → GTK scrollbar sits in a track that's twice as thick (top half of which is non-functional)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
status-firefox60:
--- → affected
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8968836 [details] Bug 1355143 - Implement CreateStyleContextWithStates to style with fully stated css path, https://reviewboard.mozilla.org/r/237560/#review243316 Some minor stuff, otherwise looks good! ::: widget/gtk/WidgetStyleCache.h:59 (Diff revision 1) > > +/* > + * Returns a pointer to a style context for the specified node > + * and state applied to all elements at widget style path. > + * > + * The context owned by caller and must be released by g_object_unref(). The context is owned by... ::: widget/gtk/WidgetStyleCache.h:62 (Diff revision 1) > + * and state applied to all elements at widget style path. > + * > + * The context owned by caller and must be released by g_object_unref(). > + */ > +GtkStyleContext* > +GetStyleContextStated(WidgetNodeType aNodeType, Hm, maybe GetStyleContextWithState or GetStyleContextIncludingState Stated means something else.
Attachment #8968836 -
Flags: review?(jhorak) → review+
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8968837 [details] Bug 1355143 - Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, https://reviewboard.mozilla.org/r/237562/#review243368 ::: widget/gtk/gtk3drawing.cpp:2903 (Diff revision 1) > > WidgetNodeType scrollbar = aOrientation == GTK_ORIENTATION_HORIZONTAL ? > MOZ_GTK_SCROLLBAR_HORIZONTAL : MOZ_GTK_SCROLLBAR_VERTICAL; > > gboolean backward, forward, secondary_backward, secondary_forward; > - GtkStyleContext* style = GetStyleContext(scrollbar); > + GtkStyleContext* style = GetStyleContext(scrollbar, GTK_TEXT_DIR_NONE, Can't you pass actual orientation instead of GTK_TEXT_DIR_NONE? Same applies to all calls of GetStyleContextStated(). ::: widget/gtk/gtk3drawing.cpp:3008 (Diff revision 1) > MozGtkSize trackSizeForThumb = metrics->size.thumb + metrics->border.track; > + g_object_unref(style); > + > // button > if (hasButtons) { > - metrics->size.button = GetMinMarginBox(MOZ_GTK_SCROLLBAR_BUTTON); > + style = GetStyleContext(MOZ_GTK_SCROLLBAR_BUTTON, GTK_TEXT_DIR_NONE, Don't you miss ...Stated there? ::: widget/gtk/gtk3drawing.cpp:3012 (Diff revision 1) > if (hasButtons) { > - metrics->size.button = GetMinMarginBox(MOZ_GTK_SCROLLBAR_BUTTON); > + style = GetStyleContext(MOZ_GTK_SCROLLBAR_BUTTON, GTK_TEXT_DIR_NONE, > + aActive ? GTK_STATE_FLAG_PRELIGHT : > + GTK_STATE_FLAG_NORMAL); > + metrics->size.button = GetMinMarginBox(style); > + g_object_unref(style); If not, unref is not corrert there.
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8968838 [details] Bug 1355143 - Size scrollbars with 'hover' Gtk+ state, https://reviewboard.mozilla.org/r/237564/#review243374 ::: commit-message-e8047:1 (Diff revision 1) > +Bug 1355143 - Size scrollbars with 'hover' Gtk+ state, r?jhorak Please describe it as a workaround with some background explanation. Why is it needed.
Attachment #8968838 -
Flags: review?(jhorak) → review+
Assignee | ||
Comment 19•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8968837 [details] Bug 1355143 - Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, https://reviewboard.mozilla.org/r/237562/#review243368 > Don't you miss ...Stated there? Yes, I missed the stated here.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8968837 [details] Bug 1355143 - Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, https://reviewboard.mozilla.org/r/237562/#review243684
Attachment #8968837 -
Flags: review?(jhorak) → review+
Comment 24•6 years ago
|
||
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/8683842b6058 Implement CreateStyleContextWithStates to style with fully stated css path, r=jhorak https://hg.mozilla.org/integration/autoland/rev/111bb4416a85 Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, r=jhorak https://hg.mozilla.org/integration/autoland/rev/4405d1c4673b Size scrollbars with 'hover' Gtk+ state, r=jhorak
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8683842b6058 https://hg.mozilla.org/mozilla-central/rev/111bb4416a85 https://hg.mozilla.org/mozilla-central/rev/4405d1c4673b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 26•6 years ago
|
||
Can anyone from Ubuntu confirm the fix here? I'd like to request uplift to FF60ESR.
Flags: needinfo?(dholbert)
Reporter | ||
Comment 27•6 years ago
|
||
Yes, I've verified that this is fixed. The scrollbar is the full thickness of the track in current Nightly, and this is definitely what fixed it, because this command (launching the build just before the fix landed) produces the old/broken half-width behavior: mozregression --repo autoland --launch 16bb3f3c01cb -a https://bug1346961.bmoattachments.org/attachment.cgi?id=8846824 ...whereas this command (using the final commit in the series here) produces the expected full-width behavior: mozregression --repo autoland --launch 4405d1c4673b -a https://bug1346961.bmoattachments.org/attachment.cgi?id=8846824
Status: RESOLVED → VERIFIED
Flags: needinfo?(dholbert)
Assignee | ||
Comment 28•6 years ago
|
||
Comment on attachment 8968836 [details] Bug 1355143 - Implement CreateStyleContextWithStates to style with fully stated css path, Approval Request Comment [Feature/Bug causing the regression]: Bug 1433092 [User impact if declined]: Linux/Ubuntu scrollbars are difficult to use as the scroll bar thumb is tiny. [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]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no/minor risk [Why is the change risky/not risky?]: Linux specific theme fix. [String changes made/needed]: none
Attachment #8968836 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 29•6 years ago
|
||
Comment on attachment 8968837 [details] Bug 1355143 - Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, Approval Request Comment [Feature/Bug causing the regression]: Bug 1433092 [User impact if declined]: Linux/Ubuntu scrollbars are difficult to use as the scroll bar thumb is tiny. [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]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no/minor risk [Why is the change risky/not risky?]: Linux specific theme fix. [String changes made/needed]: none
Assignee | ||
Comment 30•6 years ago
|
||
Comment on attachment 8968838 [details] Bug 1355143 - Size scrollbars with 'hover' Gtk+ state, Approval Request Comment [Feature/Bug causing the regression]: Bug 1433092 [User impact if declined]: Linux/Ubuntu scrollbars are difficult to use as the scroll bar thumb is tiny. [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]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no/minor risk [Why is the change risky/not risky?]: Linux specific theme fix. [String changes made/needed]: none
Attachment #8968838 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 31•6 years ago
|
||
Comment on attachment 8968837 [details] Bug 1355143 - Provide ScrollbarGTKMetrics for active (mouse over) scrollbars, Approval Request Comment [Feature/Bug causing the regression]: Bug 1433092 [User impact if declined]: Linux/Ubuntu scrollbars are difficult to use as the scroll bar thumb is tiny. [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]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no/minor risk [Why is the change risky/not risky?]: Linux specific theme fix. [String changes made/needed]: none
Attachment #8968837 -
Flags: approval-mozilla-beta?
Comment 32•6 years ago
|
||
Comment on attachment 8968836 [details] Bug 1355143 - Implement CreateStyleContextWithStates to style with fully stated css path, Regression in 60, fix verified in nightly. Let's uplift for beta 16.
Attachment #8968836 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8968837 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8968838 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Comment 33•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/eede6015f82c https://hg.mozilla.org/releases/mozilla-beta/rev/48abba26dc25 https://hg.mozilla.org/releases/mozilla-beta/rev/8aabde7e8df2
Updated•6 years ago
|
Flags: qe-verify+
Comment 34•6 years ago
|
||
I managed to partially reproduce the issue using Ubuntu 16.04 x64 on an older version of Nightly (2017-04-10). The scrollbar's track was twice as thick as the scrollbar, but it was functional. I couldn't find a place where to click and it didn't work. I retested everything using the same platform on Latest Nightly and beta 60.0b16. On Nightly the bug is fixed, but on beta I still get the scrollbar thinner than the track.
Flags: needinfo?(stransky)
Comment 35•6 years ago
|
||
(In reply to Oana Botisan from comment #34) > I managed to partially reproduce the issue using Ubuntu 16.04 x64 From comment #2: > This does *not* affect Ubuntu 16.04 -- there, the scrollbar *grows* to fill the full track when I hover the scroll track, and the full scroll-track is clickable.
Comment 36•6 years ago
|
||
I retested everything on Ubuntu 17.04 x64 using latest Nightly 61.0a1 and beta 60.0b16. I got two different behaviours. - on beta 60.0b16: The scrollbar had the width of the track. - on Nightly 61.0a1: The scrollbar was animated, it had the width of the track only when you hover the mouse over it. What is the expected behaviour? On the other hand, the scrollbar was functional no matter where you clicked on the track.
Assignee | ||
Comment 37•6 years ago
|
||
(In reply to Oana Botisan from comment #36) > I retested everything on Ubuntu 17.04 x64 using latest Nightly 61.0a1 and > beta 60.0b16. I got two different behaviours. > - on beta 60.0b16: The scrollbar had the width of the track. > - on Nightly 61.0a1: The scrollbar was animated, it had the width of the > track only when you hover the mouse over it. > What is the expected behaviour? Yes, that's expected as Nightly 61.0a1 caries patches for Bug 1454897 which emulates the scrollbar thumb animation. > On the other hand, the scrollbar was functional no matter where you clicked > on the track. Good, that's main part of fix for this bug.
Flags: needinfo?(stransky)
Comment 38•6 years ago
|
||
Considering the information from comment 37, I will mark verified for firefox60 too. Thank you for the clarification.
Comment 39•6 years ago
|
||
Considering that this bug is verified I will take out the qe+ tag.
Flags: qe-verify+
Comment 40•6 years ago
|
||
Regression: With these patches, arrow buttons on scrollbar ends are invisible. Clicking on them still works, but they are not displayed.
Flags: needinfo?(stransky)
Assignee | ||
Comment 41•6 years ago
|
||
(In reply to Yuri Khan from comment #40) > Created attachment 8975310 [details] > 20180513-firefox-scrollbar-arrows.png > > Regression: With these patches, arrow buttons on scrollbar ends are > invisible. Clicking on them still works, but they are not displayed. Thanks. Which theme do you use? I know we handle scrollbar buttons differently here but it should affects only themes which have different scrollbar sizes in active/inactive state and also uses buttons - and I don't know such theme.
Flags: needinfo?(stransky) → needinfo?(yurivkhan)
Comment 42•6 years ago
|
||
I am using Clearlooks-Phenix (package clearlooks-phenix-theme) on Ubuntu 16.04. I do not think it has different scrollbar sizes when active or inactive; in my configuration, they look pretty constant to me at all times. But I might have removed the overlay-scrollbar package because I find it distracting and annoying when things appear and disappear dynamically.
Flags: needinfo?(yurivkhan) → needinfo?(stransky)
Assignee | ||
Comment 44•6 years ago
|
||
Do you mind to file another bug for it?
Flags: needinfo?(yurivkhan)
Reporter | ||
Comment 45•6 years ago
|
||
Looks like Yuri filed bug 1461312 to cover that regression - thanks!
Flags: needinfo?(yurivkhan)
Updated•6 years ago
|
status-firefox55:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•