Bug 175067

Summary: [GTK] Spin buttons are drawn too large
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, cadubentzen, calvaris, cgarcia, hoboprimate, mcatanzaro, nekohayo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugzilla.gnome.org/show_bug.cgi?id=785578
https://bugs.webkit.org/show_bug.cgi?id=208129
Bug Depends on:    
Bug Blocks: 173572    
Attachments:
Description Flags
Spin buttons
none
WIP none

Description Carlos Alberto Lopez Perez 2017-08-02 04:31:21 PDT
This bug is has been moved from https://bugzilla.gnome.org/show_bug.cgi?id=785578

Original report:

"
This happened in the last few days, with an update to the site.

See screenshots comparing the result in epiphany and chrome, epiphany's number controls are too big hiding the result area.
"
Comment 1 Carlos Alberto Lopez Perez 2017-08-02 04:40:40 PDT
Here are some screen-shots showing how chrome and webkitgtk renders the generate random number table at https://www.random.org/

 * chrome: https://people.igalia.com/clopez/wkbug/175067/chrome.png
 * webkitgtk before applying fix for bug 173572 : https://people.igalia.com/clopez/wkbug/175067/webkitgtk_2.16.3.png
 * webkitgtk after applying fix for bug 173572 : https://people.igalia.com/clopez/wkbug/175067/webkitgtk_2.16.6.png


As you can see, after fix for bug 173572 the size of the input type was not big enough to draw the value and the spin-buttons at the same time, so the value of the input type was covered (In the Max: it shows 1 instead of 100.. the "00" are covered by the spin buttons)

After fix for bug 173572 the width of the input size choose by the web designer is overriden and grow to accommodate the spin-buttons without covering the input values). So now on "Max" you an see the "100" value correctly.

However, this causes the inputs to reflow vertically as the table where they are contained is not big enough to draw both the input and the Max/Min text in the same line.

And this ends causing that the value for the generated random number to fall below the visible area of that table.

Note that this was already happening before (as you can see the value on https://people.igalia.com/clopez/wkbug/175067/webkitgtk_2.16.3.png is not draw 100% correctly, its cut at the half). But now is exaggerated more so its not even appearing.
Comment 2 Carlos Alberto Lopez Perez 2017-08-02 04:52:14 PDT
And the thing is that I don't know how we can fix this.

I already felt that overriding the size of the input numbers that was choose by the web designer was maybe going to cause some issues on some sites when I proposed that solution, but I don't know how to fix this issue without doing that.

The only sane thing I thing we can do is to go back to the spin buttons that we had some years ago:

Now we have this: https://trac.webkit.org/export/217674/webkit/trunk/LayoutTests/platform/gtk/fast/forms/number/number-appearance-spinbutton-layer-expected.png

Before we had this: https://trac.webkit.org/export/148821/webkit/trunk/LayoutTests/platform/gtk/fast/forms/number/number-appearance-spinbutton-layer-expected.png


Web designers are going to keep testing their sites only with chrome/firefox/safari, and all of this browsers have small spinbuttons likes the ones we had before. Either we go back to small spinbuttons or we will breaking some sites like this one.
Comment 3 Adrian Perez 2017-08-02 08:55:08 PDT
(In reply to Carlos Alberto Lopez Perez from comment #2)
> And the thing is that I don't know how we can fix this.
> 
> I already felt that overriding the size of the input numbers that was choose
> by the web designer was maybe going to cause some issues on some sites when
> I proposed that solution, but I don't know how to fix this issue without
> doing that.
> 
> The only sane thing I thing we can do is to go back to the spin buttons that
> we had some years ago:
> 
> Now we have this:
> https://trac.webkit.org/export/217674/webkit/trunk/LayoutTests/platform/gtk/
> fast/forms/number/number-appearance-spinbutton-layer-expected.png
> 
> Before we had this:
> https://trac.webkit.org/export/148821/webkit/trunk/LayoutTests/platform/gtk/
> fast/forms/number/number-appearance-spinbutton-layer-expected.png
> 
> 
> Web designers are going to keep testing their sites only with
> chrome/firefox/safari, and all of this browsers have small spinbuttons likes
> the ones we had before. Either we go back to small spinbuttons or we will
> breaking some sites like this one.

Would it be an acceptable tradeoff to use the current (new, bigger buttons)
version when the size of the field is wider than a certain threshold, and
use the other version (old, narrow buttons) for the narrow inputs? I am aware
that doing it that way makes us maintain two different ways for rendering the
inputs, but at least it wouldn't break any website, while still trying to
provide the version with the easier-to-hit button when possible.
Comment 4 Michael Catanzaro 2017-08-06 15:41:26 PDT
(In reply to Adrian Perez from comment #3)
> Would it be an acceptable tradeoff to use the current (new, bigger buttons)
> version when the size of the field is wider than a certain threshold, and
> use the other version (old, narrow buttons) for the narrow inputs? I am aware
> that doing it that way makes us maintain two different ways for rendering the
> inputs, but at least it wouldn't break any website, while still trying to
> provide the version with the easier-to-hit button when possible.

That sounds like an OK option. Clearly, using extra space is not OK.

Alternatively, could we scale down the nice GTK+ spinbuttons to fit in the available space?
Comment 5 Michael Catanzaro 2018-06-15 07:31:07 PDT
*** Bug 183030 has been marked as a duplicate of this bug. ***
Comment 6 Carlos Bentzen 2018-06-26 16:37:01 PDT
(In reply to Michael Catanzaro from comment #4)
> (In reply to Adrian Perez from comment #3)
> > Would it be an acceptable tradeoff to use the current (new, bigger buttons)
> > version when the size of the field is wider than a certain threshold, and
> > use the other version (old, narrow buttons) for the narrow inputs? I am aware
> > that doing it that way makes us maintain two different ways for rendering the
> > inputs, but at least it wouldn't break any website, while still trying to
> > provide the version with the easier-to-hit button when possible.
> 
> That sounds like an OK option. Clearly, using extra space is not OK.

I'll try to go for this option.

> 
> Alternatively, could we scale down the nice GTK+ spinbuttons to fit in the
> available space?

I think the horizontal square buttons make it difficult to fit nicely in tiny spacing...

Also a lot of refactoring would be need to this based on the way it's currently being drawn. The most simple way would be Adrian's suggestions I think.
Comment 7 Carlos Bentzen 2018-06-26 16:39:22 PDT
> The most simple way would be Adrian's suggestions I
> think.

simplest*
Comment 8 Carlos Bentzen 2018-07-31 16:49:48 PDT
Created attachment 346225 [details]
Spin buttons

I went through a more conservative approach and ported the old GTK spin button to the RenderTheme{Widget,Gadget} code and made it the only option. The result is as attached here.

WDYT?

If you think we should proceed to returning to this design, I will update test expectations were needed to submit the patch for review. Below I'll send the WIP patch for the moment.
Comment 9 Carlos Bentzen 2018-07-31 16:52:13 PDT
Created attachment 346226 [details]
WIP
Comment 10 Michael Catanzaro 2018-08-04 11:57:55 PDT
Carlos Garcia, what do you think?

It looks like the arrow buttons are too small when the entry is larger. And even at the smallest size, they appear to be misaligned horizontally, right? I would expect the arrows to be aligned with the edge of the entry.
Comment 11 Carlos Garcia Campos 2018-08-06 00:54:54 PDT
The point of our theming code is to be consistent with the rest of the platform (being GNOME the platform in this case). So, I don't think we should render the spin buttons differently, but I agree they don't work in case of small fields that are simply not allowed in the GTK+ world. So, I think we should either skip the theming when the size of the control is smaller than the minimum GTK+ spin button size for the current theme, or render a different style only when the control size is too small. It could happen that a website with spin buttons of different sizes look inconsistent, though, but I guess that's unlikely.
Comment 12 Adrian Perez 2018-08-06 05:06:25 PDT
(In reply to Carlos Garcia Campos from comment #11)
> The point of our theming code is to be consistent with the rest of the
> platform (being GNOME the platform in this case). So, I don't think we
> should render the spin buttons differently, but I agree they don't work in
> case of small fields that are simply not allowed in the GTK+ world. So, I
> think we should either skip the theming when the size of the control is
> smaller than the minimum GTK+ spin button size for the current theme, or
> render a different style only when the control size is too small. It could
> happen that a website with spin buttons of different sizes look
> inconsistent, though, but I guess that's unlikely.

I agree with Carlos here. Either there is space for the buttons and they
get painted in the same way as GTK+ does, and skip them if the entry is
too small. I would rather have the spin button entry degrade to a normal
entry than having a custom widget that does not correspond to anything
available in GTK+.
Comment 13 Michael Catanzaro 2020-01-20 13:48:33 PST
I hit this today trying to configure a meeting reminder on Google Calendar.

And then again after downloading Ubuntu to test something. This example doesn't require any login: https://ubuntu.com/download/desktop/thank-you
Comment 14 Michael Catanzaro 2020-03-31 09:54:12 PDT
This is fixed in trunk. The new spin buttons seem way too small tbh, but that's the opposite of the problem reported here.
Comment 15 Carlos Alberto Lopez Perez 2020-03-31 10:32:30 PDT
(In reply to Michael Catanzaro from comment #14)
> This is fixed in trunk. The new spin buttons seem way too small tbh, but
> that's the opposite of the problem reported here.

It was 257299 right?
Comment 16 Carlos Alberto Lopez Perez 2020-03-31 10:33:05 PDT
(In reply to Carlos Alberto Lopez Perez from comment #15)
> (In reply to Michael Catanzaro from comment #14)
> > This is fixed in trunk. The new spin buttons seem way too small tbh, but
> > that's the opposite of the problem reported here.
> 
> It was 257299 right?

r257299


r257299
Comment 17 Michael Catanzaro 2020-03-31 11:20:49 PDT
Yes.