Closed Bug 1300405 Opened 8 years ago Closed 1 year ago

Inserting a selected option above the currently selected option leads to multiple selected options

Categories

(Core :: DOM: Forms, defect, P3)

Product:
Core
Component:
DOM: Forms
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
111 Branch
Tracking Flags:
Tracking Status
firefox111 --- verified
firefox112 --- verified

People

(Reporter: zbinlin, Assigned: ben.freist)

References

Details

(Keywords: good-first-bug, helpwanted, testcase, Whiteboard: [lang=c++], [wptsync upstream])

Attachments

(4 files)

Reporter's testcase
704 bytes, text/html
Details
Workaround
731 bytes, text/html
Details
Bug 1300405 - Change for loop in FindSelectedIndex to start at 0 and skip currently selected option, add new testcase. r?mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1300405 - Fix insertion of already selected options r=emilio
48 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160830141501

Steps to reproduce:

test case:

http://jsbin.com/cilotedoqe/1/edit?html,js,output

Click the `insert B` button to test.

User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0


Actual results:

The select element selected A option.


Expected results:

The select element select B option.
Tested on Mac OS X 10.10 with FF 48 release and FF Nightly 51.0a1(2016-09-06) and I can reproduce it.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Attached file Reporter's testcase — 
Actually, both A and B are selected, or at least it looks like that
in the dropdown menu.
Attached file Workaround — 
One workaround might be to select the inserted option from a setTimeout.
Severity: normal → minor
Keywords: helpwanted, testcase
Priority: -- → P4
Whiteboard: [good first bug][lang=c++]
Changing SelectedIndex works fine. Tested in Firefox 45.0.1

Code below:
<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>Testcase for bug 1300405</title>


<script>
function insertOption() {
	var select = document.querySelector("select");
	var refNode = document.querySelectorAll("option")[1];
	var opt = document.createElement("option");
	opt.selected = true;
	opt.value = 20;
	opt.textContent = "B";
	select.insertBefore(opt, refNode);
        select.selectedIndex=1;
}

window.insertOption = insertOption;
</script>


</head>
<body>
  <select>
    <option value="10">A</option>
    <option value="30">C</option>
    <option value="40" selected>D</option>
  </select>
  <button onclick="insertOption()">insert B</button>
</body>
</html>
Hello!

I want to work on this bug. I thought, that problems can be somewhere in HTMLSelectElement class, but according to GDB constructor of this class isn't call during loading of this page. Isn't this object a HTMLSelectElement?
Hey !

I would want to work on this as my first bug, if its still free.
Correct me if I am wrong, is the error in the fact that everytime we press insert B a new B option gets added and that it has to be added only once??
or is it that when I do the first 'insert B' operation both a and b are getting selected.
Keywords: good-first-bug
Whiteboard: [good first bug][lang=c++] → [lang=c++]

This try push test shows a first working solution for the issue mentioned above.

Further examination of the issue limits the scope of this to inserting selected options above the currently selected option. Inserting selected options below the currently selected options does not reproduce the issue.

Frederic provided the first code solution to this, thus assigning this issue to him.

Assignee: nobody → frederic.kirstein
Summary: When insert a selected option to select element, the select element will display(select) the first option, but not the selected option. → Inserting a selected option above the currently selected option leads to multiple selected options
Version: 50 Branch → Trunk

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: frederic.kirstein → nobody
Assignee: nobody → hexagonrecursion

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: hexagonrecursion → nobody

Hi! Is anyone working on this bug? I see that several patches have been submitted, I'd be happy to help get this resolved if it's available.

(In reply to Stephen Donahue from comment #13)

Hi! Is anyone working on this bug? I see that several patches have been submitted, I'd be happy to help get this resolved if it's available.

Confirmed that it's still present in 102.0a1

Severity: minor → S3
Component: Layout: Form Controls → DOM: Forms
Priority: P4 → P3

(In reply to Stephen Donahue from comment #13)

Hi! Is anyone working on this bug? I see that several patches have been submitted, I'd be happy to help get this resolved if it's available.

Feel free to take this bug :)

Flags: needinfo?(mail)

Hey, I see that this bug still exists. I would like to try fix this. I am however, unable to find the source code in C++. How do I access that? Please help me out :)

HTMLSelectElement.cpp is there still in the same location where it is in the patch in comment 10.

Hello, is this bug still available? If it is, I will look into it, and once I grasp what's involved, I'll ask it to be assigned to me.

This is still available, as far as I see.

I wanted to take a stab at this bug too. Would this fix be acceptable?

--- a/dom/html/HTMLSelectElement.cpp
+++ b/dom/html/HTMLSelectElement.cpp
@@ -796,7 +796,7 @@ bool HTMLSelectElement::SetOptionsSelect
         }
 
         // If the index is already selected, ignore it.
-        if (option && !option->Selected()) {
+        if (option) {
           // To notify the frame if anything gets changed. No need
           // to flush here, if there's no frame yet we don't need to
           // force it to be created just to notify it about a change

As I understand it the problem is that when SetOptionsSelectedByIndex runs, it correctly deselects option D but it skips propagating the selection status of B to the SelectElement. SetOptionsSelectedByIndex seems to assume that that option hasn't been selected outside yet.
To avoid an empty selection CheckSelectSomething just tries all options starting at A (and already succeeds there).
Would this have performance implications?

Hey,
is this bug still free? Can I send you an MR?

Flags: needinfo?(emilio)

Hey, sure, please do!

Flags: needinfo?(emilio)

Redirect a needinfo that is pending on an inactive user to the triage owner.
:peterv, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mail) → needinfo?(peterv)
Assignee: nobody → ben.freist
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9753327b9bf4
Fix insertion of already selected options r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38458 for changes under testing/web-platform/tests
Whiteboard: [lang=c++] → [lang=c++], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/9753327b9bf4

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox111: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Upstream PR merged by moz-wptsync-bot
QA Whiteboard: [qa-111b-p2]

Reproducible on a 2023-02-10 Nightly build on macOS 12.
Verified as fixed on Firefox 111.0b4(build ID: 20230221190142) and Nightly 112.0a1(build ID: 20230223094032) on macOS 12, Windows 11, Ubuntu 22.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-111b-p2]
status-firefox111: fixed → verified
status-firefox112: --- → verified
Flags: needinfo?(peterv)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: