1230065 - [GTK3] Dropdown menu at wellsfargo.com is unreadable (text is pushed too far down & clipped)

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

STR:
 1. Visit https://www.wellsfargo.com/
 2. Look at the dropdown menu (<select>) towards the upper-left, under "View Your Accounts".

ACTUAL RESULTS:
The text on the dropdown menu is unreadable; it's pushed down such that it can't be seen. See attached screenshot. Pretty much impossible to tell what's selected.

EXPECTED RESULTS:
Readable text on dropdown

Comment 1

[Daniel Holbert [:dholbert]]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=099f695d31326c39595264c34988a0f4b7cbc698&tochange=c321d84038519dcf1670d59fd2c5c00ad8a85a55

Looks like a regression from bug 1198613.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot from before regression (no bug)

Comment 3

[Daniel Holbert [:dholbert]]

wellsfargo gives this dropdown menu an explicit "height: 18px".  If I remove that, the widget gets substantially taller (28px) and the text fits just fine, with generous padding on the top & bottom.

This might be a Tech Evang issue (either to wellsfargo or to GTK about their default padding), though really it might also be a sign of more trouble to come from bug 1198613...  I'd wager we'll encounter more cases of this, where the now-larger desired height of a dropdown menu causes site breakage.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of data-url with 21px-tall <select>, in Firefox Nightly vs. release

Semi-testcase:
 data:text/html,<select style="height: 21px"><option>Hello

In Firefox release (42, with gtk2), I can see the text just fine -- it barely fits.

In current Firefox Nightly (45, with gtk3 and bonus padding from bug 1198613), I can only see the very top of the text.

Comment 5

[Karl Tomlinson (:karlt)]

I guess <select/> should center the text vertically like we do for <input/>.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Centering seems like the right thing, and seems like we should get that done sooner rather than later.

Any chance jwatt could look at this?


[Tracking Requested - why for this release]:
Fixing the styles of GTK selects (following the switch to GTK3) exposed this issue; seems like the right fix is to make the layout code center the text, but we should get that in the same release.

Comment 7

[Andrew Comminos [:acomminos]]

The issue here appears to be that the text is centered within the bounds of the widget's border, rather than the bounds of the entire widget. I don't really see this as a problem, however; many widgets' centers are defined relative to their borders (e.g. to horizontally center text in a combobox, it should account for the space taken up by the dropdown arrow). In addition, this doesn't just affect comboboxes, it affects any widget with text whose padding violates the sizing expectations of the developer writing the CSS styles.

I think there are three routes to go in fixing this;

1) Enforce a minimum widget size such that text doesn't get cut off.
2) Remove or limit GTK widget padding when there is insufficient space (bounds are smaller than natural size).
3) Ignore the issue, encouraging developers to respect the widget's natural sizing.

Thoughts, Karl?

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #7)
> The issue here appears to be that the text is centered within the bounds of
> the widget's border, rather than the bounds of the entire widget.

I don't think I follow.

Compare data:text/html,<select style="height: 15px"><option>Help
where text is border-top-aligned and
data:text/html,<input style="height: 15px" value="Help">
where text is vertically centered by line-height: -moz-block-height.

The bug exists in select elements even in versions without changes from bug
1198613.

> I don't
> really see this as a problem, however; many widgets' centers are defined
> relative to their borders (e.g. to horizontally center text in a combobox,
> it should account for the space taken up by the dropdown arrow).

Horizontal positioning in a combobox is usually left-aligned, and this bug is about vertical positioning.

> In
> addition, this doesn't just affect comboboxes, it affects any widget with
> text whose padding violates the sizing expectations of the developer writing
> the CSS styles.
> 
> I think there are three routes to go in fixing this;
> 
> 1) Enforce a minimum widget size such that text doesn't get cut off.
> 2) Remove or limit GTK widget padding when there is insufficient space
> (bounds are smaller than natural size).
> 3) Ignore the issue, encouraging developers to respect the widget's natural
> sizing.
> 
> Thoughts, Karl?

I think vertical centering is the appropriate solution for select elements, and I guess some other widgets too.  This is similar to option 2, but the cross-platform code that knows the height can do the positioning.

Comment 9

[Andrew Comminos [:acomminos]]

(In reply to Karl Tomlinson (back Dec 21 ni?:karlt) from comment #8)
> (In reply to Andrew Comminos [:acomminos] from comment #7)
> > The issue here appears to be that the text is centered within the bounds of
> > the widget's border, rather than the bounds of the entire widget.
> 
> I don't think I follow.

Sorry, what I intended to convey was that the current vertical centering implementation does not allow the contents of the <select> field to go into the widget border (instead, bounded by the content area). I agree though that it would be good to do this at the layout level, as you suggested.

The clipping of content within the border bounds as seen with
> data:text/html,<input style="height: 15px" value="Help">
will need to be addressed before this can work though, I think. Would we just subtract the necessary height to center from the border widths in layout, I suppose?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #9)
> (In reply to Karl Tomlinson (back Dec 21 ni?:karlt) from comment #8)
> > (In reply to Andrew Comminos [:acomminos] from comment #7)
> > > The issue here appears to be that the text is centered within the bounds of
> > > the widget's border, rather than the bounds of the entire widget.
> > 
> > I don't think I follow.
> 
> Sorry, what I intended to convey was that the current vertical centering
> implementation does not allow the contents of the <select> field to go into
> the widget border (instead, bounded by the content area).

Ah, thanks.  Comment 7 makes more sense to me now.  I assume you are saying
the centering happens within the content box (inside the border and padding)
rather than within the border box.

Perhaps though for elements using box-sizing: border-box [1]
the centering should be within the border box.  I'm not entirely sure.
Maybe it depends on whether the height is explicit or intrinsic.
[1] https://developer.mozilla.org/en/docs/Web/CSS/box-sizing

Perhaps elements that specify the height should not be natively styled?

> The clipping of content within the border bounds as seen with
> > data:text/html,<input style="height: 15px" value="Help">
> will need to be addressed before this can work though, I think.

Ah, yes.  That is an issue here.

For input, that may be related to using "overflow-clip-box: content-box;"
and the anonymous div using overflow-y:auto.
DOM inspector View menu lets you "Show Anonymous Content".  The built-in
Inspector can also do that by setting a pref in about:config.

select doesn't have an anonymous div but uses overflow-clip-box: padding-box.

> Would we
> just subtract the necessary height to center from the border widths in
> layout, I suppose?

Sounds reasonable.

Ideally I guess we should be distinguishing between border and padding in the
widget code.  Perhaps we can do that for selects?  They don't specify
padding, which was the problem with doing that for inputs.

The height of separators in moz_gtk_combo_box_paint() might need some
rethinking if the padding can be changed by content.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Regression and direct impact on websites, tracking!

Comment 12

[Jonathan Watt [:jwatt]]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #6)
> Centering seems like the right thing, and seems like we should get that done
> sooner rather than later.
> 
> Any chance jwatt could look at this?

Not likely in the next few weeks. :( I have a lot of other things to catch up or just now. Clearing needinfo for now to avoid any expectation that I'll be looking at this soon.

Comment 13

[Daniel Holbert [:dholbert]]

Another site with the same problem: http://www.safeco.com/ (insurance carrier)  The dropdown below "Get a quote" only shows the top half of the letters inside of it. Looks like the letters would fit if we centered them vertically, though.

Comment 14

[Jet Villegas (inactive)]

GTK3 is disabled starting in 45.0b4. Please update tracking flags.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug on Google login page, with language selector

FWIW: this affects the Google sign-in page, too ( https://accounts.google.com/ServiceLogin ). Screenshot attached, with a Nightly window on top of a Firefox 44 window, for comparison.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Karl, should this be a blocker for gtk3 moving to release?   Affecting a major banking site and google login sounds pretty major.  Can you help find someone to work on this?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Also tracking for 47, regression

Comment 18

[Karl Tomlinson (:karlt)]

Layout changes would be too risky to uplift, but I've been experimenting with some patches to widget code that would help.  It looks like we'll be able to improve the situation considerably.

Not sure whether this alone would block GTK3.  GTK3 may provide more improvements overall than regressions.  Although this regression makes the sites look awful, they are usable, and I suspect this bug has existed in some form since forever.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: MozReview Request: bug 1230065 consider arrow size in dropdown minimum widget size r?acomminos

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

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: screenshot of Google login with patch

The patch uses the minimum size approach.  Thanks for the suggestion, Andrew.

It is layout's responsibility to size appropriately for the text.  This widget code patch does not do that, but ensuring space for the dropdown button means that most of the text is visible on the Wells Fargo and Google sites.

Comment 21

[Andrew Comminos [:acomminos]]

Comment on attachment 8726034 [details]
MozReview Request: bug 1230065 consider arrow size in dropdown minimum widget size r?acomminos

https://reviewboard.mozilla.org/r/37773/#review34317

This seems like it'll work for most cases, although (similar to what you mentioned) this bug may still occur for larger font sizes where the text exceeds the height of the arrow widget.

Looks good though!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9251fa3a32

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to Andrew Comminos [:acomminos] from comment #21)
>  this bug may still occur for larger font sizes where the text
> exceeds the height of the arrow widget.

Yes.  Bug 328035 and bug 429802.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b9251fa3a32

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8726034 [details]
MozReview Request: bug 1230065 consider arrow size in dropdown minimum widget size r?acomminos

Approval Request Comment for 46
[Feature/regressing bug #]:
Changes from bug 1198613 make a long standing bug much more obvious.
[User impact if declined]: hard to read text in selects, sometimes requiring clicking on the select to see the current value.
[Describe test coverage new/current, TreeHerder]:
There are a number of existing tests for selects, but they do not cover this case.  There is no new test because the patch is more of a workaround than a fixing, merely mitigating the effect of the bug in comment situations.
[Risks and why]: The patch is reasonably simple, but there is risk of larger than expected select buttons.
[String/UUID change made/needed]: none.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: screenshot of wells fargo login patch, with patch (fixed)

I'll mark this as VERIFIED -- as confirmation, here's a screenshot of the wells fargo login page, with an after-the-fix Nightly (dated 2016-03-06).

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8726034 [details]
MozReview Request: bug 1230065 consider arrow size in dropdown minimum widget size r?acomminos

Select menu fix, recent regression, fix is verified. 
This should land in time for the beta 2 build.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/563d9b2dfe7e

Comment 29

[Bogdan Maris, Desktop Test Engineering]

Will this be fixed for google as well? It looks like after this patch the dropdown from google has moved a few pixels down. I compared 44.0 with 46 beta 8: http://i.imgur.com/DAUeBel.png?1

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> Will this be fixed for google as well? It looks like after this patch the
> dropdown from google has moved a few pixels down. I compared 44.0 with 46
> beta 8: http://i.imgur.com/DAUeBel.png?1

I'm not planning to make further changes to widget code to address that.
Bug 328035 and bug 429802 cover the changes required to layout code.

Comment 31

[Bogdan Maris, Desktop Test Engineering]

(In reply to Karl Tomlinson (ni?:karlt) from comment #30)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> > Will this be fixed for google as well? It looks like after this patch the
> > dropdown from google has moved a few pixels down. I compared 44.0 with 46
> > beta 8: http://i.imgur.com/DAUeBel.png?1
> 
> I'm not planning to make further changes to widget code to address that.
> Bug 328035 and bug 429802 cover the changes required to layout code.

Thanks Karl, I'll go ahead and mark this as verified based on my testing done on 46 beta 8 across platforms (Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).