If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 44

Status

()

Core
Widget: Gtk
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: acomminos)

Tracking

({regression})

Trunk
mozilla44
Unspecified
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(8 attachments, 4 obsolete attachments)

284 bytes, text/html
Details
11.89 KB, image/png
Details
4.47 KB, image/png
Details
991 bytes, image/png
Details
1.17 KB, patch
karlt
: review+
Details | Diff | Splinter Review
3.27 KB, image/png
Details
813 bytes, patch
acomminos
: review+
Details | Diff | Splinter Review
1.75 KB, patch
karlt
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Created attachment 8638610 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8638611 [details]
screenshot of testcase
(Reporter)

Comment 2

2 years ago
Created attachment 8638612 [details]
zoomed-in screenshot of testcase
(Reporter)

Comment 3

2 years ago
I'm using Nightly 42.0a1 (2015-07-24) with Ubuntu 15.04 & gnome-shell.
Blocks: 1193807
Keywords: regression
OS: Unspecified → Linux
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.
Created attachment 8647700 [details] [diff] [review]
Use theme style padding for entry elements on GTK3.
Attachment #8647700 - Flags: review?(karlt)
Created attachment 8647702 [details] [diff] [review]
Correct reftests to consider native input widget padding on GTK3.

Update fuzz and change box sizing to include padding.
Attachment #8647702 - Flags: review?(karlt)
Created attachment 8647704 [details] [diff] [review]
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.
Attachment #8647704 - Flags: review?(karlt)
Comment hidden (spam)
Try push with changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcba96dc775
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.
Attachment #8647700 - Flags: review?(karlt) → review+
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
Attachment #8647700 - Flags: review+ → review-
Comment hidden (typo)
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.
Attachment #8647702 - Flags: review-
Attachment #8647702 - Flags: review+
Attachment #8647702 - Flags: feedback?(dholbert)
Attachment #8647704 - Flags: review?(karlt) → review+
(Reporter)

Comment 14

2 years ago
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.
Attachment #8647702 - Flags: feedback?(dholbert) → feedback-
Assignee: nobody → acomminos
Status: NEW → ASSIGNED
(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.
(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
(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.
(Reporter)

Comment 18

2 years ago
Created attachment 8666883 [details]
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.
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.
(In reply to Karl Tomlinson (ni?:karlt) from comment #19)
> The testcase in comment is interesting.

comment 18, I mean.
Created attachment 8671566 [details] [diff] [review]
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.
Attachment #8671566 - Flags: review?(karlt)
Attachment #8647700 - Attachment is obsolete: true
Created attachment 8671736 [details]
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.
Attachment #8671566 - Flags: review?(karlt) → review+
Created attachment 8672334 [details] [diff] [review]
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.
Attachment #8672334 - Flags: review+
Attachment #8647704 - Attachment is obsolete: true
Created attachment 8672335 [details] [diff] [review]
Fuzz reftest to accomodate entry padding changes on GTK3.
Attachment #8672335 - Flags: review?(karlt)
Attachment #8647702 - Attachment is obsolete: true
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c69a66ea94f7
Attachment #8672335 - Flags: review?(karlt) → review+

Comment 26

2 years ago
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
https://hg.mozilla.org/mozilla-central/rev/6c23b6538fda
https://hg.mozilla.org/mozilla-central/rev/f65351467258
https://hg.mozilla.org/mozilla-central/rev/c64252c6132b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1214957
Although this change should be good for NS_THEME_TEXTFIELD, I'm not comfortable uplifting this to 43 because of bug 1214957.
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 31

2 years ago
Created attachment 8688131 [details]
GtkEntry comparsion on Fedora

There's the actual screenshot from Fedora.

Comment 32

2 years ago
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.

Updated

2 years ago
Attachment #8688131 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.