Closed Bug 1335705 Opened 7 years ago Closed 7 years ago

GTK 3.20: Incorrect render <input type=range> - no thumb

Categories

(Core :: Widget: Gtk, defect, P2)

54 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: striptm, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, Whiteboard: tpi:+)

Attachments

(6 files, 2 obsolete files)

In Ubuntu 16.10 with themes Radiance and Ambiance, the <input type=range> controls is not displayed correctly.

A rectangle is displayed but the slider (drag control) is not present.

On another computer with Ubuntu 16.04 is correct.
Attached file testcase (obsolete) —
Attachment #8832453 - Attachment mime type: text/plain → text/html
Attached file tescase as raw html
Attachment #8832453 - Attachment is obsolete: true
Attachment #8832454 - Attachment mime type: text/plain → text/html
Confirmed by another 16.10 user, I think we use native GTK widgets for this and 16.10 uses GTK 3.20 vs 3.18 for 16.04, so CCing people with Linux skills.
That's because MOZ_GTK_SCALE_HORIZONTAL has "min-width" style property 0 and we rely on that to derive slider min size. We may check that value from some minimal size.
Component: Layout: Form Controls → Widget: Gtk
Well, looks like we need to fix scale rendering completely on Gtk > 3.20. Looking into it.
Attachment #8833304 - Attachment is obsolete: true
Attachment #8833304 - Flags: review?(karlt)
Comment on attachment 8833304 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

https://reviewboard.mozilla.org/r/109564/#review111032
Attachment #8833304 - Attachment is obsolete: false
Comment on attachment 8833304 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

Looks like another review request discarded this one. Trying to restore, if that does not work I'll submit another one.
Attachment #8833304 - Flags: review?(karlt)
Priority: -- → P2
Whiteboard: tpi:+
Comment on attachment 8833306 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

https://reviewboard.mozilla.org/r/109570/#review113998

This is pretty much what moz_gtk_scale_paint() wants, thanks.  I'm not sure whether or not nsNativeThemeGTK::GetMinimumWidgetSize() should also consider the scale widget minsize.

Thank you for using mozreview.  It makes review much easier.

::: widget/gtk/gtk3drawing.cpp:2504
(Diff revision 1)
>      gtk_style_context_get_padding(style, GTK_STATE_FLAG_NORMAL, &padding);
>  
>      *height += (border.top + border.bottom + padding.top + padding.bottom);
>      ReleaseStyleContext(style);
>  }
>  

> Bug 1335705 - For Gtk > 3.20 determine scale through size from trough CSS node and also add border/padding/margin, r?karlt

ITYM "scale trough size".
Attachment #8833306 - Flags: review?(karlt) → review+
Comment on attachment 8833304 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

https://reviewboard.mozilla.org/r/109564/#review113980

Took me a while to understand that the negative margin affects the minimum border box size because the margin box cannot be less than zero, but looks good thanks.

::: widget/gtk/gtk3drawing.cpp:2564
(Diff revision 1)
>        gtk_style_context_get(style, gtk_style_context_get_state(style),
> -                            "min-width", thumb_length,
> -                            "min-height", thumb_height,
> +                            "min-width", &min_width,
> +                            "min-height", &min_height,
>                               nullptr);
> +      GtkBorder margin;
> +      gtk_style_context_get_margin(style, gtk_style_context_get_state(style),

Please store and use the result of the previous gtk_style_context_get_state(style) call instead of calling again.

::: widget/gtk/gtk3drawing.cpp:2571
(Diff revision 1)
> +      if (margin_width < 0 && min_width < - margin_width)
> +          min_width = - margin_width;
> +      if (margin_height < 0 && min_height < - margin_height)
> +          min_height = - margin_height;

Gecko style is no space between unary operators ('-' here) and their operands.

No need for the margin_width/height < 0 checks, as the max of min_width and -margin_width is sufficient with gint arithmetic.
Comment on attachment 8833304 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

(In reply to Martin Stránský from comment #11)
> Looks like another review request discarded this one. Trying to restore, if
> that does not work I'll submit another one.

All patches for a bug need to be push together or mozreview assumes that patches not pushed again have been discarded.  I assume that is what happened at least.

It is possible to preview changes before publishing.
I use this in ~/.hgrc so that I don't need to reply to the prompt and publish after preview.

[reviewboard]
autopublish = False

r+ thanks with issues in reviewboard addressed.
Attachment #8833304 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #13)
> I'm not sure
> whether or not nsNativeThemeGTK::GetMinimumWidgetSize() should also consider
> the scale widget minsize.

I think nsRangeFrame::GetMinISize() is depending on GetMinimumWidgetSize() to return the minimum size of the whole range (scale widget), so that extending thumbs don't overlap other content, but that's a lesser bug than this one.

http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/layout/forms/nsRangeFrame.cpp#809
Blocks: gtk-3.20
(In reply to Karl Tomlinson (:karlt) from comment #15)
> All patches for a bug need to be push together or mozreview assumes that
> patches not pushed again have been discarded.  I assume that is what
> happened at least.

I think I mixed patches for more bugs together which confuses mozreview. Thanks for the hint I'll try that.
Comment on attachment 8839855 [details]
Bug 1335705 - For Gtk > 3.20 determine scale trough size from trough CSS node and also add border/padding/margin,

https://reviewboard.mozilla.org/r/114444/#review115886

::: widget/gtk/gtk3drawing.cpp:2508
(Diff revision 1)
>  }
>  
>  void
>  moz_gtk_get_scale_metrics(GtkOrientation orient, gint* scale_width,
>                            gint* scale_height)
>  {

This is the same patch as the previous one but with fixed description. Mozreview seems to lose track because of missing mozreview ID.
Comment on attachment 8839855 [details]
Bug 1335705 - For Gtk > 3.20 determine scale trough size from trough CSS node and also add border/padding/margin,

https://reviewboard.mozilla.org/r/114444/#review116202
Attachment #8839855 - Flags: review?(karlt) → review+
Comment on attachment 8833306 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

https://reviewboard.mozilla.org/r/109570/#review116200

::: widget/gtk/gtk3drawing.cpp:2571
(Diff revision 2)
> +      if (margin_width < 0 && min_width < - margin_width)
> +          min_width = -margin_width;
> +      if (margin_height < 0 && min_height < - margin_height)

Gecko style is no space between unary operators ('-' here) and their operands margin_width/height (in the boolean condition for the if block too).

No need for the margin_width/height < 0 checks, as the max of min_width and -margin_width is sufficient with gint arithmetic.
(In reply to Martin Stránský from comment #21)
> Mozreview seems to lose track because of missing mozreview ID.

Yes.  I suspect it uses some heuristics when there is no mozreview ID, but I guess differing number of patches and differing commit messages confuse those.
Attachment #8839855 - Attachment is obsolete: true
Comment on attachment 8833306 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

https://reviewboard.mozilla.org/r/109570/#review116200

> Gecko style is no space between unary operators ('-' here) and their operands margin_width/height (in the boolean condition for the if block too).
> 
> No need for the margin_width/height < 0 checks, as the max of min_width and -margin_width is sufficient with gint arithmetic.

Sorry I overlooked the "-" in the condition.
Attachment #8839855 - Attachment is obsolete: false
Attachment #8833304 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c2b324b9afb
Consider also margin when determine range widget slider size, r=karlt
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41a156cf149e
For Gtk > 3.20 determine scale trough size from trough CSS node and also add border/padding/margin, r=karlt
Summary: Incorrect render <input type=range> → GTK 3.20: Incorrect render <input type=range> - no thumb
You need to log in before you can comment on or make changes to this bug.