Andrew Comminos [:acomminos] Assignee
	Description • 9 years ago

Currently we draw scrollbar troughs on GTK3 even if the themes do not have arrows and cannot fit a slider.

	Andrew Comminos [:acomminos] Assignee
	Comment 1 • 9 years ago

This has the added benefit of fixing some of the W3C platform tests for quirks-mode and 100% height.

	Karl Tomlinson (:karlt)
	Comment 2 • 9 years ago

Comment on attachment 8615591 [details] [diff] [review]
Require room for scrollbar sliders to draw troughs on GTK3.

This is more complicated, I think.

If it weren't for the issue worked around in bug 513006, perhaps we wouldn't
need to do this because the thumbs request a minimum size, but I'm not
confident in the assumption there that the parent frame size is already known.
Does removing the parent logic for the thumbs fix this bug?

If we do need to specify the minimum size on the track, then trough_border
should be considered.

But don't we want the minimum slider size to only be considered if there are
no scroll buttons?

>     gtk_widget_style_get (gHorizScrollbarWidget,
>                           "slider_width", &metrics->slider_width,
>                           "trough_border", &metrics->trough_border,
>                           "stepper_size", &metrics->stepper_size,
>                           "stepper_spacing", &metrics->stepper_spacing,
>+                          "min_slider_length", &metrics->min_slider_size,
>                           NULL);
> 
>-    metrics->min_slider_size = 
>-        gtk_range_get_min_slider_size(GTK_RANGE(gHorizScrollbarWidget));

Please keep gtk_range_get_min_slider_size() here.
Although gtk_widget_style_get() would be simpler, the varargs function does no
type checking, so I'd prefer to avoid gtk_widget_style_get() where possible.
The two methods should give the same answer AIUI.

	Andrew Comminos [:acomminos] Assignee
	Comment 3 • 9 years ago

(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> Does removing the parent logic for the thumbs fix this bug?
If we simply set the minimum thumb size to the minimum slider width, it wouldn't work unless we changed nsSliderFrame to respect the minimum size of its children (the thumb). Currently, the layout hierarchy looks like this;

nsScrollbarFrame (NS_THEME_SCROLLBAR_TRACK)
\-> nsSliderFrame
 \-> nsButtonBoxFrame (NS_THEME_SCROLLBAR_THUMB)

The intermediate nsSliderFrame currently only respects its own border when calculating its minimum size (https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsSliderFrame.cpp#1226), ignoring the size of the child.

> But don't we want the minimum slider size to only be considered if there are
> no scroll buttons?
Correct; I (incorrectly) thought the track was a sibling of the scroll buttons, not the parent.

I think the correct way to handle this would be to specify the minimum size on the track depending on whether or not the theme has scrollbar buttons.

	Andrew Comminos [:acomminos] Assignee
	Comment 4 • 9 years ago

This patch adds support for querying the presence of scrollbar buttons, and adds a size requirement to the trough accordingly. It removes the old parent logic that is not as useful considering it basically bypassed GTK's request for a minimum slider size.

	Andrew Comminos [:acomminos] Assignee
	Comment 5 • 9 years ago

Whoops, wrong patch format.

	Andrew Comminos [:acomminos] Assignee
	Comment 6 • 9 years ago

Adhere to C90 standards, fixes builds on try.

	Karl Tomlinson (:karlt)
	Comment 7 • 9 years ago

Comment on attachment 8621137 [details] [diff] [review]
Require room for scrollbar sliders to draw troughs on GTK themes without scroll buttons.

(In reply to Andrew Comminos [:acomminos] from comment #4)
> Created attachment 8621059 [details] [diff] [review]
> Require room for scrollbar sliders to draw troughs on GTK themes without
> scroll buttons.

> It removes the old parent
> logic that is not as useful considering it basically bypassed GTK's request
> for a minimum slider size.

I like the way removing the parent logic means that the slider is not drawn
when there is not enough room for it, instead of the current behavior of
trying to fit it in even there is not enough room to draw or slide.

Please adjust the checkin comment to include mention of this.

Perhaps "Require room for scrollbar sliders and draw troughs only when there
is a slider or buttons".

	Andrew Comminos [:acomminos] Assignee
	Comment 9 • 9 years ago

Try (GTK2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=94f354947c01

	Andrew Comminos [:acomminos] Assignee
	Comment 10 • 9 years ago

Looks like this causes a reftest failure for layout/reftests/invalidation/image-scrolling-zoom-1.html with the GTK2 theme on the test infrastructure. Will look into this, could possibly be revealing a bug in the widget layout post-invalidation with this patch.

	Andrew Comminos [:acomminos] Assignee
	Comment 11 • 9 years ago

This patch removes the intersection of the thumb rect with the slider frame, preventing us from accidentally displaying the thumb when there's not enough room for it on a position update.

	Markus Stange [:mstange]
	Comment 12 • 9 years ago

Comment on attachment 8624428 [details] [diff] [review]
Don't resize scrollbar thumb when updating its position.

Review of attachment 8624428 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/nsSliderFrame.cpp
@@ +726,5 @@
>       newThumbRect.y = clientRect.y + NSToCoordRound(pos * mRatio);
>  
>    // avoid putting the scroll thumb at subpixel positions which cause needless invalidations
>    nscoord appUnitsPerPixel = PresContext()->AppUnitsPerDevPixel();
>    nsRect snappedThumbRect = ToAppUnits(newThumbRect.ToNearestPixels(appUnitsPerPixel), appUnitsPerPixel);

Please call ToNearestPixels on newThumbRect.TopLeft(), and make the result an nsPoint snappedThumbLocation.

	Andrew Comminos [:acomminos] Assignee
	Comment 13 • 9 years ago

Thanks, updated to only round location.

Try (GTK2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a74895d6ded

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7925e9fee4
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca22a9aca57

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7f7925e9fee4
https://hg.mozilla.org/mozilla-central/rev/eca22a9aca57
https://hg.mozilla.org/mozilla-central/rev/395099ed230c

	Karl Tomlinson (:karlt)
	Comment 16 • 8 years ago

(In reply to Andrew Comminos [:acomminos] from comment #3)
> The intermediate nsSliderFrame currently only respects its own border when
> calculating its minimum size
> (https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsSliderFrame.
> cpp#1226), ignoring the size of the child.

https://hg.mozilla.org/mozilla-central/annotate/94eb248b77b0/layout/xul/nsSliderFrame.cpp#l1226