1297909 - [e10s] "display: none" on a <select>'s first option hides the full list of options

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Karl Dubost💡 :karlcow]

Attachment: option-display-none-rendering.jpg

Origin: Web Compatibility issue for croma.com
described at https://webcompat.com/issues/2985

Steps to Reproduce
1) Navigate to: http://www.croma.com/store?location=default
2) click drop down boxes under Store locator

Expected Behavior:
Drop down box should work

Actual Behavior:
Cannot select stores, only keyboard arrows work once drop down box is selected


Bare Test Case
http://codepen.io/webcompat/pen/KrjmAN

Explanation: With Firefox, when setting display: none on the first option of a list of option hides the rest of the list.

Adding rendering for Gecko, WebKit and Blink. The rendering is different in WebKit and Blink, but it is still usable. The rendering in Gecko is not usable.

Comment 1

[Daniel Holbert [:dholbert]]

I think this might be e10s-specific -- at least, for me on Linux Nightly, I can only reproduce this bug when e10s is enabled.
(Note that e10s is enabled by default in Nightly, and it's enabled on Firefox 48 release for some fraction of users.)

Karl, can you confirm that this is e10s-only for you, too?

Comment 2

[Karl Dubost💡 :karlcow]

Daniel, yes confirmed only in e10s.
Non-e10s windows show the behavior of Blink.

Comment 3

[David Durst [:ddurst]]

Neil, you just looked at a bunch of <select> stuff -- mind looking at this? It would be good to uplift this as far as we can if it's safe.

Comment 4

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
bad e10s regression related to select drop downs. somewhat corner case though, doesn't impact all content.

Comment 5

[Neil Deakin]

Attachment: Handle options being hidden better

Several fixes here:
1. The height computation was only using the first option. Instead, use the first visible option. The strange edgecase of all options being hidden just displays a narrow popup similar to non-e10s.
2. Explicitly check for display: none, rather than just setting the display property without validation.
3. Hide all children of a hidden optgroup.

One minor issue is that when the first item is hidden, the selection is still on that first item when the popup is open. Browsers handle this inconsistently but after some testing I think the new e10s behaviour with this patch is no worse than non-e10s (and I think may be better as non-e10s let's you keyboard-cycle through the hidden items.)

Comment 6

[Gerry Chang [:gchang]]

Track 49+/50+/51+ as this is a e10s related issue for drop down box in UI.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8784916 [details] [diff] [review]
Handle options being hidden better

Review of attachment 8784916 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Neil!

::: browser/base/content/test/general/browser_selectpopup.js
@@ +519,5 @@
> +  // The label contains the substring 'Visible' for items that are visible.
> +  // Otherwise, it is expected to be display: none.
> +  is(selectPopup.parentNode.itemCount, 9, "Correct number of items");
> +  let child = selectPopup.firstChild;
> +  let idx = 1;

Alternatively, you could use Array.from(selectPopup.children), and use a vanilla for-loop instead of managing your own index. I don't think it really matters, however.

Comment 8

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23c5f9c6f6ea9555ec02dd1aa141c59e661357cc
Bug 1297909, improve handling when option elements are hidden inside a select, r=mconley

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c5f9c6f6ea
improve handling when option elements are hidden inside a select, r=mconley

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23c5f9c6f6ea

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I feel pretty confident about this for uplift Enn. How do you feel about it?

Comment 12

[Neil Deakin]

Should be ok.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8784916 [details] [diff] [review]
Handle options being hidden better

Approval Request Comment
[Feature/regressing bug #]:

e10s

[User impact if declined]:

Users who visit a site that has a <select> dropdown where the first item has been styled to be invisible will not be able to see any of the rest of the entries - an empty popup will be displayed.

[Describe test coverage new/current, TreeHerder]:

An automated test has been written to test this, and it has baked in Nightly for a few days now.

[Risks and why]: 

Low risk.

[String/UUID change made/needed]:

None.

Comment 14

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

Comment on attachment 8784916 [details] [diff] [review]
Handle options being hidden better

Common select menu configuration, I'd like this to work with e10s for 49. 
Adds a new test. Let's do it!

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

There were some minor conflicts in the test file for both aurora and beta, but those were easy to fix up. It looks like SelectParentHelper.jsm has changed drastically between aurora and beta, and I couldn't easily resolve those conflicts. Could we get a rebased patch for beta if this is important to have there?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68613c98775b

Comment 17

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

Mike maybe you can rebase for beta.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I cannot reproduce this bug in beta - the height calculation that this patch fixes isn't being done in the code on beta, so I think 49 is unaffected.

Hoping SV can confirm - Ioana, do you have some time to double check for me?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

More of a question for Andre than Ioana.

Comment 20

[Daniel Holbert [:dholbert]]

I can confirm that, using latest linux beta (49.0b8) with a fresh profile[1], I cannot reproduce this bug.  (and I was previously able to reproduce in affected nightlies, up in comment 1, so I believe I would be able to reproduce on this machine if 49 betas were affected)

I think that answers mconley's request & confirms that Beta 49 is unaffected. (I guess that also means this bug was a recent regression, for e10s users, caused by some change that landed after the 49-on-nightly timeframe & which wasn't uplifted back to 49?)

[1] (with e10s [randomly?] default-enabled according to " Multiprocess Windows 1/1 (Enabled by default)" in about:support)

Comment 21

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

Oh, good. Thanks y'all. I'm glad we don't have to land this for 49 this late in the cycle!

Comment 22

[Neil Deakin]

The code that caused this doesn't exist until 50 (bug 11281560).

Comment 23

[Karl Dubost💡 :karlcow]

Probably Bug 1128156 for Comment #22 above

Comment 24

[Carmen]

I managed to reproduce the issue using the STR provided in the description, on the affected Nightly 51.0a1 (2016-08-24) build, using Windows 10 Pro x64.

The fix is now verified on Firefox Beta 50.0b4 across platforms: Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Sierra 10.12.1

Comment 25

[Carmen]

The fix was also verified on latest Aurora 51.0a2 (2016-10-05) across platforms: Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Sierra 10.12.1.

Comment 26

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you, Carmen!