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)

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: dholbert, Assigned: stransky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: stale-bug, Whiteboard: tpi:+)

Attachments

(5 files)

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.)
Attached video screencast of the issue
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.
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)
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.
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
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
Whiteboard: tpi:+
Adding Jan as he does some scrollbar work recently.
Assignee: nobody → jhorak
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: jhorak → stransky
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.
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).
It also means we need to carefully read style from GetStyleContext() - to make sure we have correct direction flags for the style.
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 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 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 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+
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 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+
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
Can anyone from Ubuntu confirm the fix here? I'd like to request uplift to FF60ESR.
Flags: needinfo?(dholbert)
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)
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?
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
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?
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 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+
Attachment #8968837 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8968838 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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)
(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.
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.
(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)
Considering the information from comment 37, I will mark verified for firefox60 too.
Thank you for the clarification.
Considering that this bug is verified I will take out the qe+ tag.
Flags: qe-verify+
Regression: With these patches, arrow buttons on scrollbar ends are invisible. Clicking on them still works, but they are not displayed.
Flags: needinfo?(stransky)
(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)
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)
Okay, I'll check that theme, Thanks.
Flags: needinfo?(stransky)
Do you mind to file another bug for it?
Flags: needinfo?(yurivkhan)
Depends on: 1461312
Looks like Yuri filed bug 1461312 to cover that regression - thanks!
Flags: needinfo?(yurivkhan)
Depends on: 1493399
You need to log in before you can comment on or make changes to this bug.