Bug 173572

Summary: [GTK] Spin buttons on input type number appear over the value itself for small widths
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebKitGTKAssignee: Carlos Alberto Lopez Perez <clopez>
Status: RESOLVED FIXED    
Severity: Normal CC: beau.adkins, bugs-noreply, buildbot, cgarcia, mcatanzaro, rniwa, simon.fraser, thorton, zalan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 174395, 175067    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
none
Patch cgarcia: review+

Description Carlos Alberto Lopez Perez 2017-06-19 17:36:50 PDT
On https://browserperfdash.igalia.com/ the input type number to select the number of rows or days (the first and the last inputs over the tables) don't show correctly on WebKitGTK+.

The GtkSpinButtons drawn to allow adjusting the values appear over the value of the input itself.


This is easily reproducible with something like:

What is your age? <input type="number" style="width: 50px;">


Not sure what should be the right fix here .... 

Maybe we should not draw the spin buttons when the size of the input element is too small to accommodate both the input values and the spin button itself ?
Comment 1 Carlos Alberto Lopez Perez 2017-06-19 17:39:24 PDT
(In reply to Carlos Alberto Lopez Perez from comment #0)
> 
> This is easily reproducible with something like:
> 
> What is your age? <input type="number" style="width: 50px;">
> 

Quick example: https://people.igalia.com/clopez/wkbug/173572/
Comment 2 Carlos Alberto Lopez Perez 2017-06-20 11:05:40 PDT
Created attachment 313415 [details]
Patch

initial patch: test EWS
Comment 3 Carlos Garcia Campos 2017-06-20 22:43:16 PDT
LGTM
Comment 4 Carlos Alberto Lopez Perez 2017-06-22 07:30:18 PDT
Created attachment 313616 [details]
Patch

add a test: test EWS again
Comment 5 Build Bot 2017-06-22 08:04:02 PDT
Comment on attachment 313616 [details]
Patch

Attachment 313616 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3978163

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 6 Build Bot 2017-06-22 08:04:04 PDT
Created attachment 313623 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-06-22 08:09:58 PDT
Comment on attachment 313616 [details]
Patch

Attachment 313616 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3978185

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 8 Build Bot 2017-06-22 08:09:59 PDT
Created attachment 313625 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-06-22 08:57:18 PDT
Comment on attachment 313616 [details]
Patch

Attachment 313616 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3978289

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 10 Build Bot 2017-06-22 08:57:19 PDT
Created attachment 313631 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 11 Build Bot 2017-06-22 08:58:22 PDT
Comment on attachment 313616 [details]
Patch

Attachment 313616 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3978274

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 12 Build Bot 2017-06-22 08:58:23 PDT
Created attachment 313632 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Michael Catanzaro 2017-06-23 16:48:51 PDT
Yeah, I'm not sure if it's caused by a change in Adwaita or a change in RenderThemeGtk, but it's a recent regression. We already have another bug report about this, but I have not managed to find it after a quick search.
Comment 14 Michael Catanzaro 2017-06-23 16:51:20 PDT
Comment on attachment 313616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313616&action=review

Nice layout test!

The 4 and 5 combo boxes don't look great, but it's surely a lot better than before.

> LayoutTests/fast/forms/number/number-minimum-size-spinbutton-not-clover.html:5
> +<p>Test that spin buttons don't clover the value of input type numbers.

clover -> cover
Comment 15 Carlos Alberto Lopez Perez 2017-06-28 11:36:36 PDT
Created attachment 314044 [details]
Patch

patch v2. No asking for review. Test EWS
Comment 16 Carlos Alberto Lopez Perez 2017-07-05 11:09:13 PDT
Created attachment 314623 [details]
Patch

patch v3. No asking for review. Test EWS
Comment 17 Build Bot 2017-07-05 11:57:37 PDT
Comment on attachment 314623 [details]
Patch

Attachment 314623 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4057691

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover-controled-style.html
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 18 Build Bot 2017-07-05 11:57:38 PDT
Created attachment 314630 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-07-05 12:15:22 PDT
Comment on attachment 314623 [details]
Patch

Attachment 314623 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4057679

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover-controled-style.html
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 20 Build Bot 2017-07-05 12:15:24 PDT
Created attachment 314635 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 21 Build Bot 2017-07-05 12:15:31 PDT
Comment on attachment 314623 [details]
Patch

Attachment 314623 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4057734

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover-controled-style.html
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 22 Build Bot 2017-07-05 12:15:33 PDT
Created attachment 314636 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 23 Build Bot 2017-07-05 13:14:12 PDT
Comment on attachment 314623 [details]
Patch

Attachment 314623 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4057989

New failing tests:
fast/forms/number/number-minimum-size-spinbutton-not-clover-controled-style.html
fast/forms/number/number-minimum-size-spinbutton-not-clover.html
Comment 24 Build Bot 2017-07-05 13:14:13 PDT
Created attachment 314646 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 25 Carlos Alberto Lopez Perez 2017-07-10 20:24:09 PDT
Created attachment 315068 [details]
Patch

Test EWS
Comment 26 Carlos Alberto Lopez Perez 2017-07-11 07:02:34 PDT
Created attachment 315106 [details]
Patch

patch: asking for review now
Comment 27 Carlos Garcia Campos 2017-07-11 08:05:00 PDT
Comment on attachment 315106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315106&action=review

> Source/WebCore/ChangeLog:10
> +        This ensures that we don't end clovering the input values with

I'm not an English expert at all, but I think here we should say "end up"

> Source/WebCore/rendering/RenderThemeGtk.cpp:939
> +    // Input field need a minimum dimensions to render the spinbuttons correctly (without clovering the values).

need a minimum dimensions -> need minimum dimensions

> Source/WebCore/rendering/RenderThemeGtk.cpp:957
> +    // renders on WebKitGTK+. To ensure that spin buttons don't end clovering the values of the input

Same comment here about "end up"
Comment 28 Carlos Alberto Lopez Perez 2017-07-11 08:20:21 PDT
(In reply to Carlos Garcia Campos from comment #27)
> Comment on attachment 315106 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315106&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        This ensures that we don't end clovering the input values with
> 
> I'm not an English expert at all, but I think here we should say "end up"
> 
> > Source/WebCore/rendering/RenderThemeGtk.cpp:939
> > +    // Input field need a minimum dimensions to render the spinbuttons correctly (without clovering the values).
> 
> need a minimum dimensions -> need minimum dimensions
> 
> > Source/WebCore/rendering/RenderThemeGtk.cpp:957
> > +    // renders on WebKitGTK+. To ensure that spin buttons don't end clovering the values of the input
> 
> Same comment here about "end up"


Right... 

I'm also not sure from where I got the word "clover / clovering" but I think it should be saying "cover / covering" instead. I will fix it.
Comment 29 Michael Catanzaro 2017-07-11 08:27:51 PDT
This is a very important fix. Thanks!

(In reply to Carlos Garcia Campos from comment #27)
> > Source/WebCore/ChangeLog:10
> > +        This ensures that we don't end clovering the input values with
> 
> I'm not an English expert at all, but I think here we should say "end up"

Yes

(In reply to Carlos Alberto Lopez Perez from comment #28) 
> I'm also not sure from where I got the word "clover / clovering" but I think
> it should be saying "cover / covering" instead. I will fix it.

Yes. See also: https://en.wikipedia.org/wiki/Clover :)
Comment 30 Carlos Alberto Lopez Perez 2017-07-11 08:34:50 PDT
Committed r219332: <http://trac.webkit.org/changeset/219332>
Comment 31 Beau Adkins 2018-10-08 13:27:21 PDT
This caused a display regression on http://www.paradiso-design.net/ntsc_pal_conversion_faq.html

This is strange because this site doesn't even have spinner buttons at all. Why is this code growing the text entry field if it doesn't have spin buttons?

Probably a better long term solution is to use a spin button style that more closely matches other browsers. The default GTK side-by-side buttons is just way too wide. Any chance this is in the works?
Comment 32 Michael Catanzaro 2018-10-09 12:13:24 PDT
We should try fixing this in bug #175067, following the solution proposed in comment 11 there.