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

Status: RESOLVED FIXED

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

Description

[Fernando García Gómez, stripTM]

Attachment: Ubuntu 16.10 Ambiance theme

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.

Comment 1

[Fernando García Gómez, stripTM]

Attachment: Ubuntu 16.04 Radiance theme

Comment 2

[Fernando García Gómez, stripTM]

Attachment: Ubuntu 16.10 Ambiance theme Chrome

Comment 3

[Pascal Chevrel:pascalc]

Attachment: testcase

Comment 4

[Pascal Chevrel:pascalc]

Attachment: tescase as raw html

Comment 5

[Pascal Chevrel:pascalc]

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.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Well, looks like we need to fix scale rendering completely on Gtk > 3.20. Looking into it.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1335705 - Consider also margin when determine range widget slider size,

Review commit: https://reviewboard.mozilla.org/r/109564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109564/

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1335705 - Consider also margin when determine range widget slider size,

Review commit: https://reviewboard.mozilla.org/r/109570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109570/

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8833304 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

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

Comment 11

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 13

[Karl Tomlinson (:karlt)]

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".

Comment 14

[Karl Tomlinson (:karlt)]

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 15

[Karl Tomlinson (:karlt)]

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.

Comment 16

[Karl Tomlinson (:karlt)]

(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

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1335705 - For Gtk > 3.20 determine scale trough size from trough CSS node and also add border/padding/margin,

Review commit: https://reviewboard.mozilla.org/r/114444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114444/

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8833306 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109570/diff/1-2/

Comment 20

[Martin Stránský [:stransky] (ni? me)]

(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 21

[Martin Stránský [:stransky] (ni? me)]

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 22

[Karl Tomlinson (:karlt)]

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

Comment 23

[Karl Tomlinson (:karlt)]

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.

Comment 24

[Karl Tomlinson (:karlt)]

(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.

Comment 25

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8833306 [details]
Bug 1335705 - Consider also margin when determine range widget slider size,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109570/diff/2-3/

Comment 26

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c2b324b9afb
Consider also margin when determine range widget slider size, r=karlt

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/2c2b324b9afb
https://hg.mozilla.org/mozilla-central/rev/41a156cf149e