Closed
Bug 1255841
Opened 8 years ago
Closed 8 years ago
Selecting an <option> after a hidden <option> will select the hidden <option>
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla48
People
(Reporter: mconley, Assigned: gw280)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.62 KB,
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gwright
Updated•8 years ago
|
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.
Flags: needinfo?(gwright)
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.
Flags: needinfo?(gwright)
Attachment #8732271 -
Flags: review?(mconley)
Updated•8 years ago
|
Attachment #8732098 -
Attachment is obsolete: true
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.
Attachment #8732271 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Needinfoing Felipe and Blake as per comment 3.
Flags: needinfo?(mrbkap)
Flags: needinfo?(felipc)
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f66ffb0d7984
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
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
Attachment #8732271 -
Flags: approval-mozilla-beta?
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.
Attachment #8732271 -
Flags: approval-mozilla-aurora?
Hi Mike, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(mconley)
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.
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mconley)
Comment on attachment 8732271 [details] [diff] [review] 0001-Bug-1255841-Explicitly-send-down-the-child-index-whe.patch Fix was verified, Aurora47+
Attachment #8732271 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/21febfd7caca
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.
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.
Attachment #8732271 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
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.
Flags: needinfo?(mrbkap)
Updated•8 years ago
|
Version: unspecified → 46 Branch
Comment 19•7 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
Flags: needinfo?(felipc) → needinfo?(mconley)
Reporter | ||
Comment 20•7 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.
Flags: needinfo?(mconley)
You need to log in
before you can comment on or make changes to this bug.
Description
•