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

VERIFIED FIXED in Firefox 46

Status

()

Core
Widget: Gtk
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: dholbert, Assigned: karlt)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox45 unaffected, firefox46+ verified, firefox47+ verified)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Description

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

Comment 1

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

Looks like a regression from bug 1198613.
Blocks: 1198613
(Reporter)

Updated

2 years ago
Summary: Dropdown menu at wellsfargo.com is unreadable (too far down & clipped) → Dropdown menu at wellsfargo.com is unreadable (text is pushed too far down & clipped)
(Reporter)

Comment 2

2 years ago
Created attachment 8695150 [details]
screenshot from before regression (no bug)
(Reporter)

Comment 3

2 years ago
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.
Blocks: 627699
(Reporter)

Comment 4

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

Comment 5

2 years ago
I guess <select/> should center the text vertically like we do for <input/>.
Component: Widget: Gtk → Layout: Form Controls
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.
tracking-firefox45: --- → ?
Flags: needinfo?(jwatt)
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?
Flags: needinfo?(karlt)
(Assignee)

Comment 8

2 years ago
(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.
Flags: needinfo?(karlt)
(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?
(Assignee)

Comment 10

2 years ago
(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.
Regression and direct impact on websites, tracking!
status-firefox46: --- → affected
tracking-firefox45: ? → +
(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.
Flags: needinfo?(jwatt)
OS: Unspecified → Linux
(Reporter)

Comment 13

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

2 years ago
GTK3 is disabled starting in 45.0b4. Please update tracking flags.
tracking-firefox45: + → ?
(Reporter)

Updated

2 years ago
status-firefox45: affected → unaffected
tracking-firefox45: ? → ---
tracking-firefox46: --- → ?
(Reporter)

Updated

2 years ago
Summary: Dropdown menu at wellsfargo.com is unreadable (text is pushed too far down & clipped) → [GTK3] Dropdown menu at wellsfargo.com is unreadable (text is pushed too far down & clipped)
(Reporter)

Comment 15

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

Updated

2 years ago
Blocks: 1193807
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?
tracking-firefox46: ? → +
Flags: needinfo?(karlt)
Also tracking for 47, regression
status-firefox47: --- → affected
tracking-firefox47: --- → +
Whiteboard: [gfx-noted]
(Assignee)

Comment 18

2 years ago
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.
Assignee: nobody → karlt
Flags: needinfo?(karlt)
(Assignee)

Comment 19

2 years ago
Created attachment 8726034 [details]
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/
Attachment #8726034 - Flags: review?(andrew)
(Assignee)

Comment 20

2 years ago
Created attachment 8726035 [details]
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 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!
Attachment #8726034 - Flags: review?(andrew) → review+

Comment 22

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9251fa3a32
(Assignee)

Comment 23

2 years ago
(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.
Component: Layout: Form Controls → Widget: Gtk

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b9251fa3a32
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 25

2 years ago
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.
Attachment #8726034 - Flags: approval-mozilla-beta?
(Reporter)

Comment 26

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

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox47: fixed → verified
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.
Attachment #8726034 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 28

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/563d9b2dfe7e
status-firefox46: affected → fixed
Flags: qe-verify+
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
Flags: needinfo?(karlt)
(Assignee)

Comment 30

a year ago
(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.
Flags: needinfo?(karlt)
(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).
status-firefox46: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.