1187385 - [GTK3] Default-styled <input> has text / cursor overlapping its border, with GTK3

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. View attached testcase.

ACTUAL RESULTS:
The first <input> element demonstrates that the cursor overlaps the <input> border, and barely protrudes outside of its rounded corners.

The second <input> element demonstrates the same for "j" characters.

EXPECTED RESULTS:
<inputs> should be large enough to contain their text & cursor, by default.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of testcase

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: zoomed-in screenshot of testcase

Comment 3

[Daniel Holbert [:dholbert]]

I'm using Nightly 42.0a1 (2015-07-24) with Ubuntu 15.04 & gnome-shell.

Comment 4

[Andrew Comminos [:acomminos]]

The correct way to fix this would be to incorporate the style padding of GtkEntry elements as defined by the theme; it's a bit excessive in some cases though.

IIRC some reftests will have to be worked around if this approach is taken, not too bad though.

Comment 5

[Andrew Comminos [:acomminos]]

Attachment: Use theme style padding for entry elements on GTK3.

Comment 6

[Andrew Comminos [:acomminos]]

Attachment: Correct reftests to consider native input widget padding on GTK3.

Update fuzz and change box sizing to include padding.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Use -moz-appearance: none to ignore widget style padding in mochitests unrelated to widget rendering.

Set -moz-appearance: none for mochitests that depend on a 5px border padding for textfields.

Comment 8

[Andrew Comminos [:acomminos]]

Comment on attachment 8647700 [details] [diff] [review]
Use theme style padding for entry elements on GTK3.

># HG changeset patch
># User Andrew Comminos <[e-mail]>
>
>Bug 1187385 - Use theme style padding for entry elements on GTK3. r=karlt
>
>diff --git a/widget/gtk/gtk3drawing.c b/widget/gtk/gtk3drawing.c
>index 2bdaa6a..17b9dd0 100644
>--- a/widget/gtk/gtk3drawing.c
>+++ b/widget/gtk/gtk3drawing.c
>@@ -2665,16 +2665,17 @@ moz_gtk_get_widget_border(GtkThemeWidgetType widget, gint* left, gint* top,
> 
>             moz_gtk_add_style_border(style, left, top, right, bottom);
>             return MOZ_GTK_SUCCESS;
>         }
>     case MOZ_GTK_ENTRY:
>         {
>             ensure_entry_widget();
>             style = gtk_widget_get_style_context(gEntryWidget);
>+            moz_gtk_add_style_padding(style, left, top, right, bottom);
>             moz_gtk_add_style_border(style, left, top, right, bottom);
> 
>             return MOZ_GTK_SUCCESS;
>         }
>     case MOZ_GTK_TREEVIEW:
>         {
>             ensure_scrolled_window_widget();
>             style = gtk_widget_get_style_context(gScrolledWindowWidget);

Comment 9

[Andrew Comminos [:acomminos]]

Try push with changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcba96dc775

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8647700 [details] [diff] [review]
Use theme style padding for entry elements on GTK3.

Thanks.  I suspect other widgets are missing padding also.

We should probably make the default moz_gtk_get_widget_border implementation after the switch statement use moz_gtk_add_style_padding() and then the MOZ_GTK_ENTRY special case would not be required.
But that can be a project for another day.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8647700 [details] [diff] [review]
Use theme style padding for entry elements on GTK3.

Actually, sorry.

Unless we have reason to believe that the text dimensions differ between Gecko and GTK, then we now have more padding in Gecko because both document / user agent CSS [1] and GTK padding is included.

I guess that means the default moz_gtk_get_widget_border implementation shouldn't use moz_gtk_add_style_padding().

If GTK padding is going to be used, then GetWidgetPadding() should return true.
That would mean that documents can't override the padding.
If it is a problem then I guess moz_gtk_get_widget_border can include the GTK-style padding but subtract 1 for the padding added by forms.css.

I guess that means the default moz_gtk_get_widget_border implementation shouldn't use moz_gtk_add_style_padding().

[1] http://hg.mozilla.org/mozilla-central/annotate/4e883591bb5d/layout/style/forms.css#l59

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8647702 [details] [diff] [review]
Correct reftests to consider native input widget padding on GTK3.

># HG changeset patch
># User Andrew Comminos <[e-mail]>
>
>Bug 1187385 - Correct reftests to consider native input widget padding on GTK3. r=karlt
>
>diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list
>index 9cb775e..5b68d63 100644
>--- a/layout/reftests/bugs/reftest.list
>+++ b/layout/reftests/bugs/reftest.list
>@@ -1477,17 +1477,17 @@ skip-if((B2G&&browserIsRemote)||Mulet) == 537507-2.html 537507-2-ref.html # bug
> == 539880-1-dynamic.html 539880-1-ref.html
> skip-if(B2G||Mulet) fuzzy-if(Android&&AndroidVersion>=15,12,1000) == 539949-1.html#test2 539949-1-ref.html#test2 # Initial mulet triage: parity with B2G/B2G Desktop
> == 541382-1.html 541382-1-ref.html
> random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)||!haveTestPlugin) == 541406-1.html 541406-1-ref.html
> needs-focus != 542116-1.html 542116-1-ref.html
> needs-focus != 542116-2.html 542116-2-ref.html
> needs-focus != 542116-3.html 542116-3-ref.html
> == 542317-1.html 542317-1-ref.html
>-fuzzy-if(Android,17,2) fuzzy-if(xulRuntime.widgetToolkit=="gtk3",1,230) skip-if((B2G&&browserIsRemote)||Mulet) == 542605-hidden-unscrollable.xul 542605-hidden-unscrollable-ref.xul # bug 974780 # Initial mulet triage: parity with B2G/B2G Desktop
>+fuzzy-if(Android,17,2) fuzzy-if(xulRuntime.widgetToolkit=="gtk3",1,240) skip-if((B2G&&browserIsRemote)||Mulet) == 542605-hidden-unscrollable.xul 542605-hidden-unscrollable-ref.xul # bug 974780 # Initial mulet triage: parity with B2G/B2G Desktop
> == 542620-1.html 542620-1-ref.html
> == 545049-1.html 545049-1-ref.html
> == 546033-1.html 546033-1-ref.html
> random-if(!haveTestPlugin) == 546071-1.html 546071-1-ref.html
> == 549184-1.html 549184-1-ref.html
> == 550325-1.html 550325-1-ref.html
> == 550325-2.html 550325-1-ref.html
> == 550325-3.html 550325-1-ref.html
>diff --git a/layout/reftests/floats/float-outside-block-push-ref.html b/layout/reftests/floats/float-outside-block-push-ref.html
>index 6403e1b..70360c5 100644
>--- a/layout/reftests/floats/float-outside-block-push-ref.html
>+++ b/layout/reftests/floats/float-outside-block-push-ref.html
>@@ -2,17 +2,17 @@
> <html lang="en">
> <head>
> <title>Testcase, bug 472252</title>
> 
> <style type="text/css">
> #wrap {width:600px; border:1px solid;position:relative}
> .a {background:lime; color:#fff; width:80%;}
> .b {position:absolute; right:0; width:18%; background: cyan; color: #000; height:10em;}
>-textarea {width: 100%; height:10em;}
>+textarea {width: 100%; height:10em; box-sizing: border-box;}
> </style> 
> 
> <title>test</title>
> </head>
> <body>
> <div id="wrap">
> 	<div class="b"></div>
> 	<div class="a"><textarea></textarea></div>
>diff --git a/layout/reftests/floats/float-outside-block-push.html b/layout/reftests/floats/float-outside-block-push.html
>index 006e841..3a8cba9 100644
>--- a/layout/reftests/floats/float-outside-block-push.html
>+++ b/layout/reftests/floats/float-outside-block-push.html
>@@ -6,17 +6,17 @@
>     <link rel="author" title="Mozilla Corporation" href="http://mozilla.com/" />
>     <link rel="help" href="http://www.w3.org/TR/CSS21/visuren.html#floats"/>
>     <meta name="flags" content="" />
> 
> <style type="text/css">
> #wrap {width:600px; border:1px solid;}
> .a {background:lime; color:#fff; width:80%;}
> .b {float:right; width:18%; background: cyan; color: #000; height:10em;}
>-textarea {width: 100%; height:10em;}
>+textarea {width: 100%; height:10em; box-sizing: border-box;}
> </style> 
> 
> <title>test</title>
> </head>
> <body>
> <div id="wrap">
> 	<div class="b"></div>
> 	<div class="a"><textarea></textarea></div>

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8647702 [details] [diff] [review]
Correct reftests to consider native input widget padding on GTK3.

Sorry, I'm going to revoke this too because bug 472252 was about the textarea being larger than 100% width.

Daniel, due you know what is going on in this test, or can you redirect to dbaron if not, please?

Andrew, I'd just mark this one as failing because it doesn't explain what it is trying to do and having extra widget border is not a bug in the code, so I guess it is a problem with the test.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8647702 [details] [diff] [review]
Correct reftests to consider native input widget padding on GTK3.

(In reply to Karl Tomlinson (ni?:karlt) from comment #13)
> Daniel, due you know what is going on in this test, or can you redirect to
> dbaron if not, please?

I think it's mainly testing whether the float wraps or not.  And:
 (1) Good news: the way it's failing with the attached patch isn't wrapping-related -- it's just stacking-order difference between the reference case and the testcase, due to the use of "position:absolute" in the reference case to hack up a position.
 (2) Bad news: if you add "box-sizing:border-box", I *think* it may prevent the test from actually being able to test the wrapping thing that it's intending to test. (i.e. it might not cause the bug that it's trying to test for anymore, in a buggy build.) (though I'm not entirely sure)

My suggestion would be to simply change the reference case to fix its stacking order, without adjusting the testcase at all, so that when there's overlap (due to a wider-than-expected widget as is the case here), the reference case's stacking order matches the testcase.

One way to do this is to give .a "transform:translate(0);z-index:1", which will bring it to the front, which is where it is in the testcase. (and which is where it would be in the reference case if not for the 'position:absolute' hack)

Alternately, you could give the test a fails-if() annotation for linux, and file a followup & tag dbaron on the followup. He's busy traveling for a few weeks, though, and I'm not sure he'd remember the test well enough to have a better solution than the above. So in the interests of keeping the test actually running, I'd lean towards fixing the reference case's stacking order as described above.

Comment 15

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> Comment on attachment 8647700 [details] [diff] [review]
> Use theme style padding for entry elements on GTK3.
> 
> Actually, sorry.
> 
> Unless we have reason to believe that the text dimensions differ between
> Gecko and GTK, then we now have more padding in Gecko because both document
> / user agent CSS [1] and GTK padding is included.
> 
> I guess that means the default moz_gtk_get_widget_border implementation
> shouldn't use moz_gtk_add_style_padding().
> 
> If GTK padding is going to be used, then GetWidgetPadding() should return
> true.
> That would mean that documents can't override the padding.
> If it is a problem then I guess moz_gtk_get_widget_border can include the
> GTK-style padding but subtract 1 for the padding added by forms.css.
> 
> I guess that means the default moz_gtk_get_widget_border implementation
> shouldn't use moz_gtk_add_style_padding().
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/4e883591bb5d/layout/style/
> forms.css#l59

I'm not sure we should move the padding calculations to GetWidgetPadding; author-specified padding via CSS is handled on all other platforms. Making widget padding non-overridable causes many tests to fail (https://treeherder.mozilla.org/#/jobs?repo=try&revision=301eb298da2e) as well.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #15)
> author-specified padding via CSS is handled on all other platforms. Making
> widget padding non-overridable causes many tests to fail
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=301eb298da2e) as
> well.

Thanks for looking at that.  Would it make sense to subtract 1 to compensate for HTML padding?

It seems weird that textarea (with -moz-appearance: textfield-multiline) has different padding to input
http://hg.mozilla.org/mozilla-central/annotate/4e883591bb5d/layout/style/forms.css#l104
I don't know why that would be but it may be easiest to adjust the compensation according to -moz-appearance.

I don't know how well that works with scrollbar positions, but it may be better not to subtract anything for textfield-multiline.

XUL themes may benefit from some adjustment, but that can be considered separately.
http://hg.mozilla.org/mozilla-central/annotate/d590b9601ba8/toolkit/themes/linux/global/textbox.css#l15

Comment 17

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #16)
> (In reply to Andrew Comminos [:acomminos] from comment #15)
> > author-specified padding via CSS is handled on all other platforms. Making
> > widget padding non-overridable causes many tests to fail
> > (https://treeherder.mozilla.org/#/jobs?repo=try&revision=301eb298da2e) as
> > well.
> 
> Thanks for looking at that.  Would it make sense to subtract 1 to compensate
> for HTML padding?

I think that this is an okay solution as long as we handle the case where the author has specified 0 padding, in which case we should make sure we clamp to GTK's padding.

Letting the author's padding override the theme's padding doesn't always make sense, considering that it may not provide proper spacing for custom theme engine drawn insets (like what we're seeing now with Ambiance and Unico), or even insets provided by a custom background-image, etc. So we still probably want to be adding GTK's padding to the widget's border. Double padding is the safest as far as usability goes, but it can be ugly.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: screenshot of <textarea> with no padding

More severe testcase (just discovered in the wild):
  data:text/html,<textarea style="padding:0" autofocus>

Screenshot attached -- in Firefox nightly (with gtk3), the cursor is entirely outside of the textarea's interior -- it's superimposed on the border, and clearly sticking out of the upper-left where the border curves around.  (I'm seeing this on Ubuntu 15.04 & 15.10, with gnome-shell installed.)

For comparison -- in Firefox release (v41), without gtk3, the cursor is clearly inside of the textarea on this testcase.

Comment 19

[Karl Tomlinson (:karlt)]

The testcase in comment is interesting.  Apparently Ambience is drawing its border in the padding rect.  Compare
data:text/html,<textarea style="padding:0;-moz-appearance:none" autofocus>
where the caret is adjacent to the border.

(In reply to Andrew Comminos [:acomminos] from comment #17)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #16)
> > Thanks for looking at that.  Would it make sense to subtract 1 to compensate
> > for HTML padding?
> 
> I think that this is an okay solution as long as we handle the case where
> the author has specified 0 padding, in which case we should make sure we
> clamp to GTK's padding.
> 
> Letting the author's padding override the theme's padding doesn't always
> make sense, considering that it may not provide proper spacing for custom
> theme engine drawn insets (like what we're seeing now with Ambiance and
> Unico), or even insets provided by a custom background-image, etc. So we
> still probably want to be adding GTK's padding to the widget's border.
> Double padding is the safest as far as usability goes, but it can be ugly.

Detecting zero padding sounds fine, if that's feasible, but I don't know how simple that is, and I suspect it is not necessary.  I expect most themes are going to have 1 pixel of space between the visible border and the text, in which case subtracting 1 pixel will at worst put the text adjacent to the border, which is what an author using padding:0 would expect.

Ambience has plenty of padding so there should still be plenty after subtracting one pixel.

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #19)
> The testcase in comment is interesting.

comment 18, I mean.

Comment 21

[Andrew Comminos [:acomminos]]

Attachment: Use GTK style padding for entry widgets.

This accounts for the 1px padding set on input elements in forms.css.

We shouldn't have to worry about the case where GTK themes have 0px of padding I don't think.

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: textarea-extra-padding.png

Including the extra padding for NS_THEME_TEXTFIELD_MULTILINE also is adding space to textareas that shouldn't be there.

That may be due to using GtkEntry instead of GtkTextView, so I think that it's ok to consider that a different bug.

Comment 23

[Andrew Comminos [:acomminos]]

Attachment: Use -moz-appearance: none to ignore widget style padding in test_caretPositionFromPoint. Carry r=karlt

Looks like by subtracting the padding added by forms.css less tests fail. It may be worth looking into these.

Comment 24

[Andrew Comminos [:acomminos]]

Attachment: Fuzz reftest to accomodate entry padding changes on GTK3.

Comment 25

[Andrew Comminos [:acomminos]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c69a66ea94f7

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c23b6538fda
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65351467258
https://hg.mozilla.org/integration/mozilla-inbound/rev/c64252c6132b

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c23b6538fda
https://hg.mozilla.org/mozilla-central/rev/f65351467258
https://hg.mozilla.org/mozilla-central/rev/c64252c6132b

Comment 28

[Karl Tomlinson (:karlt)]

Although this change should be good for NS_THEME_TEXTFIELD, I'm not comfortable uplifting this to 43 because of bug 1214957.

Comment 29

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

IMHO there's something fundamentally wrong in this code. Even with this patch mozilla renders GtkEntry 2pix higher than Gtk on Fedora & default theme.

I did some debugging and here on Fedora/Adwaita theme Gtk renders GtkEntry as 28 pix high (15pix textarea size + 13 pixels borders). Mozilla renders it 30 pix high (13 pixels border returned from Gtk and ??).

I expected that Gtk somehow cheats with border/padding (
https://bugzilla.redhat.com/show_bug.cgi?id=1205651#c3) but I was wrong here - Gtk follows the border/padding. 

Maybe it's related to the textarea size in Firefox as suggested at https://bugzilla.mozilla.org/show_bug.cgi?id=1198613#c10 ? But Gtk renders it exactly as it states - I just measured it pixel by pixel. It's really 15 pix text size (font) and 13 pix borders.

Comment 30

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

(In reply to Martin Stránský from comment #29)
> Maybe it's related to the textarea size in Firefox as suggested at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1198613#c10 ? But Gtk renders
> it exactly as it states - I just measured it pixel by pixel. It's really 15
> pix text size (font) and 13 pix borders.

The measurement was made with the same font size on Firefox and gtk3-widget-factory (which is a sample gtk3 app) when Firefox also renders the actual chars 15 pix large. Maybe there's some extra border/padding somewhere in the styles?

Comment 31

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

Attachment: GtkEntry comparsion on Fedora

There's the actual screenshot from Fedora.

Comment 32

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

Err, shame on me, I used wrong and old tree for the testing. After update it really match the FF dialogs really match the gtk3-widget-factory. Sorry for the noise.