1187237 - We see no "track" on "Range"-type input with GTK3 builds

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Julien Wajsberg [:julienw]]

Attachment: Capture

STR:
* open data:text/html;charset=utf-8,<input type%3Drange>

Expected:
* we see a track on this range input.

Actual:
* we only see the thumb.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: Capture on Aurora

Here is the same thing on Aurora.

Comment 2

[Andrew Comminos [:acomminos]]

After some investigation, there appear to be several issues at fault here;

- The range's background is not drawn with gtk_render_background.
- There is no minimum size set for the range, nor does the range inherit the minimum size of the range thumb.
- The range trough is not drawn with GTK_STYLE_CLASS_TROUGH as per gtk_range_draw.
- The range trough is not drawn within the range's margins.
- The range trough may extend unbounded in the direction perpendicular to the slider- it should be clamped to the minimum size of the trough.

I'll be working on a patch to correct these issues.

Comment 3

[Andrew Comminos [:acomminos]]

Attachment: Correct range slider drawing on GTK3.

This patch puts rendering in line with upstream GTK's gtk_range_draw, and adds minimum and maximum size bounds like GTK as well. Thanks!

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: Correct range slider drawing on GTK3.

Whoops, committed an accidental test change. Fixed.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8640609 [details] [diff] [review]
Correct range slider drawing on GTK3.

Most of this looks good, but I have some questions.

(In reply to Andrew Comminos [:acomminos] from comment #2)
> - There is no minimum size set for the range, nor does the range inherit the
> minimum size of the range thumb.

It is not usually the role of widget code to set the minimum size for
container widgets.  Usually container widgets set only the border and/or padding.  Are you sure that the code using NS_THEME_RANGE has a similar bug
to the code using NS_THEME_SCROLLBAR_TRACK_*?

I suspect moz_gtk_get_widget_border should be modified to use trough-border
and wonder whether that would make the minimum size unnecessary?

> gint
>+moz_gtk_get_scale_metrics(GtkOrientation orient, gint* scale_width,
>+                          gint* scale_height)

Given this always returns the same value, and the return value is not checked,
just return void.

>+  GdkRectangle rect;

Unused.

>+    case NS_THEME_RANGE:

>+        if (IsRangeHorizontal(aFrame)) {
>+          moz_gtk_get_scale_metrics(GTK_ORIENTATION_HORIZONTAL, &scale_width, &scale_height);
>+        } else {
>+          moz_gtk_get_scale_metrics(GTK_ORIENTATION_VERTICAL, &scale_width, &scale_height);
>+        }

Perhaps ?: would be useful.

>+        *aIsOverridable = true;

NS_THEME_SCALE_THUMB_*, NS_THEME_SCROLLBAR_TRACK_*, and most other widgets set
*aIsOverridable = false.  Is there a reason to use true here?

If the width across the slider is less, then moz_gtk_scale_paint()
may draw outside its rect, which could cause rendering problems.
If *aIsOverridable = false then that won't happen.

Comment 6

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #5)
> (In reply to Andrew Comminos [:acomminos] from comment #2)
> > - There is no minimum size set for the range, nor does the range inherit the
> > minimum size of the range thumb.
> 
> It is not usually the role of widget code to set the minimum size for
> container widgets.  Usually container widgets set only the border and/or
> padding.  Are you sure that the code using NS_THEME_RANGE has a similar bug
> to the code using NS_THEME_SCROLLBAR_TRACK_*?

Fairly sure. nsRangeFrame doesn't seem to respect the (anonymously created) thumb's size as far as I can tell. Other platforms' implementations also provide size constraints for NS_THEME_RANGE.

> I suspect moz_gtk_get_widget_border should be modified to use trough-border
> and wonder whether that would make the minimum size unnecessary?

I don't think so, not as long as NS_THEME_RANGE does not accommodate the thumb's size.

> NS_THEME_SCALE_THUMB_*, NS_THEME_SCROLLBAR_TRACK_*, and most other widgets
> set
> *aIsOverridable = false.  Is there a reason to use true here?

Nope, this was part of an older solution I was working on- I'll fix that up to false. Thanks!

Comment 7

[Andrew Comminos [:acomminos]]

Sorry, I was wrong; we do need aIsOverridable = true to override the minimum size set for the range in the browser theme's CSS. nsNativeThemeCocoa does the same thing.

Comment 8

[Andrew Comminos [:acomminos]]

Attachment: Correct range slider drawing on GTK3.

Here's an updated patch with your suggestions, thanks.

Comment 9

[Andrew Comminos [:acomminos]]

Attachment: Correct range slider drawing on GTK3.

Mark aIsOverridable = true.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #5)
> If the width across the slider is less, then moz_gtk_scale_paint()
> may draw outside its rect, which could cause rendering problems.
> If *aIsOverridable = false then that won't happen.

Sorry, I misunderstood the meaning of aIsOverridable, assuming true permitted
overriding the minimum size, but GetMinimumWidgetSize() documentation says it
just permits using a larger size.

(In reply to Andrew Comminos [:acomminos] from comment #6)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #5)
> > It is not usually the role of widget code to set the minimum size for
> > container widgets.  Usually container widgets set only the border and/or
> > padding.  Are you sure that the code using NS_THEME_RANGE has a similar bug
> > to the code using NS_THEME_SCROLLBAR_TRACK_*?
> 
> Fairly sure. nsRangeFrame doesn't seem to respect the (anonymously created)
> thumb's size as far as I can tell. Other platforms' implementations also
> provide size constraints for NS_THEME_RANGE.

I don't see this in nsNativeThemeWin::GetMinimumWidgetSize() but 
nsRangeFrame::GetMinISize() doesn't consider its children.

https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/layout/forms/nsRangeFrame.cpp#l763

Comment 11

[Andrew Comminos [:acomminos]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7edb7b53d5fe

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2162bd0f0411

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2162bd0f0411