Mike Conley (:mconley) (:⚙️) Reporter
	Description • 8 years ago

STR:

1) Visit https://bug1242450.bmoattachments.org/attachment.cgi?id=8711646
2) "Test 3" is an item in the dropdown that has hidden set to true
3) From the dropdown, select "Test 4"

ER:
"Test 4" should be the item that shows up in the closed <select>

AR:
The hidden "Test 3" is selected.

We should fix this pronto.

	Cervantes Yu [:cyu] [:cervantes]
	Comment 1 • 8 years ago

I think this fixes the bug. Also tested with optgroup with hidden options and hidden optgroup.

George, are currently on this bug? If not I'd like to propose this fix. Thanks.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 2 • 8 years ago

I think I'd prefer this method where we explicitly send the child process' index and use that. The current method of trying to iterate the index properly on the parent side when we build the option list is a bit fragile for my liking.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 3 • 8 years ago

Comment on attachment 8732271 [details] [diff] [review]
0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch

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

Yes, I think this should work.

I think we should land this, but also get some regression tests written. It might be worth checking with felipe / mrbkap to see if there are any <select> web platform tests around. If so, we should make sure they're working for the e10s case. And if there are, but they're not testing this case, we should add this case.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 4 • 8 years ago

Needinfoing Felipe and Blake as per comment 3.

	Pulsebot
	Comment 5 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f66ffb0d7984

	Carsten Book [:Tomcat]
	Comment 6 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f66ffb0d7984

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 7 • 8 years ago

[Tracking Requested - why for this release]:

This is somewhat busted behaviour in how e10s displays and treats <select> dropdowns. The end-result is that the user can get into a situation where they select from the dropdown, and what ends up getting selected in the form is not what they chose in the popup, but is instead an option that was originally hidden.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 8 • 8 years ago

Comment on attachment 8732271 [details] [diff] [review]
0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1242450
[User impact if declined]: any select dropdowns with hidden option elements won't work properly in e10s. functional issue.
[Describe test coverage new/current, TreeHerder]: no test coverage right now for this specific issue. relevant parties have been needinfod to see if we have existing tests or if we need to make a new one.
[Risks and why]: shouldn't be any risk, it's quite a simple change.
[String/UUID change made/needed]: none

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 9 • 8 years ago

Comment on attachment 8732271 [details] [diff] [review]
0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch

Approval Request Comment
See above comment.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

Hi Mike, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 11 • 8 years ago

Tracked since this is a pretty core issue. We really should add an automated test for this so we can catch any future regressions in this area.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 12 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #10)
> Hi Mike, could you please verify this issue is fixed as expected on the
> latest Nightly build? Thanks!

Yes, this is fixed.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 13 • 8 years ago

Comment on attachment 8732271 [details] [diff] [review]
0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch

Fix was verified, Aurora47+

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/21febfd7caca

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 15 • 8 years ago

I don't think we need to uplift this to beta since e10s will not be enabled on 46 after this week. 
Mike if there is a good reason to have this on beta please let me know! Thanks.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 16 • 8 years ago

Comment on attachment 8732271 [details] [diff] [review]
0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch

Too late for beta, heading into beta 7 now.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 17 • 8 years ago

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> I don't think we need to uplift this to beta since e10s will not be enabled
> on 46 after this week. 
> Mike if there is a good reason to have this on beta please let me know!
> Thanks.

The only users this will affect on beta are the ones in our experimental groups. I guess it's not a big deal to not uplift this - I don't imagine it'd be hit too too often.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 18 • 8 years ago

There are <select> web platform tests at tests/html/semantics/forms/the-select-element (as well as others) but they don't seem to cover this case. We could either add one or just add mochitests for this case.

	:Felipe Gomes (needinfo for replies!)
	Comment 19 • 8 years ago

Hey mconley, please re-read comment 3 and comment 18 where a testcase was talked about. If it's still missing, do you think this pending testcase could be written as part of the work in bug 1300784? Or at least get the contributor to try to discover if it already exists or not

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 20 • 8 years ago

(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> Hey mconley, please re-read comment 3 and comment 18 where a testcase was
> talked about. If it's still missing, do you think this pending testcase
> could be written as part of the work in bug 1300784? Or at least get the
> contributor to try to discover if it already exists or not

I think it's probably too late in the semester to try to include that as part of the project. We should probably just file a new bug for the test and mentor somebody. Filed bug 1321079.