Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select multiple - does not scroll to selected item(items) #13222

Closed
kre0n opened this issue Jul 17, 2018 · 12 comments · Fixed by #13270
Closed

Select multiple - does not scroll to selected item(items) #13222

kre0n opened this issue Jul 17, 2018 · 12 comments · Fixed by #13270

Comments

@kre0n
Copy link

kre0n commented Jul 17, 2018

bug
In react@15 we could set "value" or "defaultValue", and selected element scrolled into view.
https://codesandbox.io/s/6vx637r10n

But in react@16 this does not work.
https://codesandbox.io/s/7jqqz3zmo1

@nhunzaker
Copy link
Contributor

Intriguing! I wonder if there was a change in how the selected item was assigned in React 16.0.

@drexpp
Copy link

drexpp commented Jul 17, 2018

I tried to take a look at these files:

-https://github.com/facebook/react/blob/v15.6.2/src/renderers/dom/client/wrappers/ReactDOMSelect.js

-https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberSelect.js

Sadly I wasn't able to find that much difference. (Sorry for my lack of help but I am interested in seeing how this will be resolved or if I was looking at the wrong place).

@nhunzaker
Copy link
Contributor

No worries! Those are the first places I'd look too. One difference I see is that React 16.x assigns defaultSelected after selected:

https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMFiberSelect.js#L95

I wonder if this is throwing off React. I'm not able to test at the moment myself, but I wonder if you'd see a scroll change if you set a breakpoint and stepped through this loop.

@drexpp
Copy link

drexpp commented Jul 18, 2018

Hello Nathan, I have been exploring this issue for some hours, I installed react 15.6.2 and 16.3.2, I was able to follow the program execution with some breakpoints, it seems like the function:

updateOptions

will set one of its option attributes with "select" to true (good) also selectedIndex is set to 4 (also good), but from here I am not able to see why the element is not printed the way it should, because its parameters seem to be okay. Also I saw no difference between assigning defaultSelected before selected.

@maxLatoche
Copy link

When I ran this through a debugger selected and defaultSelected (depending on which you use) are both correctly set as true attributes on the appropriate <option> DOM element.

In the 16.3.2 example it does still select the value passed, it just doesn't scroll to it. Would there be somewhere else to look in the renderer where that value attribute is assessed/checked and the behavior is changing there? I poked around but there is a lot of code to parse through.

@irbab00n
Copy link

irbab00n commented Jul 20, 2018

Not sure how much this helps, but I did a small test using the link @kre0n supplied to find the build of React where the issue first started occurring. To test, I uninstalled and reinstalled each version of React using the interface on codesandbox.

From what I observed, everything from and after the release of 16.0.0 caused this issue to occur. Knowing that 15.6.2 was working, I decided to look into all of the pre-release builds leading from 16.0.0-alpha on up until the issue first started occurring.

I found that the select stopped scrolling to the 'value' supplied to the select element in build 16.0.0-alpha.3.

I did not test if it was fixed in a later build, but I figured first point of failure could help in some way.

@segoddnja
Copy link

Good afternoon. I think I identified what causes such undesired behavior, but, I need some more time to investigate a proper fix. So, if nobody do not mind, I would like to take this issue and do my first commitment into project.

@segoddnja
Copy link

Update. It appears that issue is a bit more complicated than I have thought. I am sure that this behavior introduced with react fiber architecture. I'll keep trying to fix it though I don't know how much time can it take. As I did not get into the implementation details before, it can take some time

@nhunzaker
Copy link
Contributor

Sorry. I've been doing a lot of travel :) Sounds like this is a really tough issue.

@segoddnja I'll assign this to you, but no pressure to burn a lot of time on this on your own. Let's try to figure out a hypothesis and maybe we can work together to resolve it.

@nhunzaker
Copy link
Contributor

I'm curious if something is happening to the select ahead of time that could cause scrolling to stop. I wonder if there's an order of property/attribute assignments that could have changed which would lead to scrolling stopping.

@nhunzaker
Copy link
Contributor

Here's an interesting phenomena, let's say the following steps happen:

https://codepen.io/nhunzaker/pen/vaZxvE

  1. Given 6 options
  2. You select the first option
  3. You select the 6th option
  4. You deselect the first option

Scrolling is still focused on the first option, even though the 6th option is the only one selected.

It appears that, Chrome anyway, only scrolls to the first selected value when assigned, not _unassigned.

Maybe this is related.

@segoddnja
Copy link

@nhunzaker Yep, thanks, you think in the right direction. Browser scrolls to the first option which got selected property true. Any further changes doesn't cause scroll.

So, what I have discovered now. React creates bare selest node and appends options to it. As bare select has multipe=false, first appended option automatically gets selected=true. So further switching to multiple mode and updating options selection doesn't cause it to scroll.

I think it is necessary to set multiple attribute value before appending options.

segoddnja added a commit to segoddnja/react that referenced this issue Jul 25, 2018
segoddnja added a commit to segoddnja/react that referenced this issue Jul 27, 2018
segoddnja added a commit to segoddnja/react that referenced this issue Aug 1, 2018
gaearon pushed a commit that referenced this issue Aug 3, 2018
…tate before appending options (#13270)

* Make sure that `select` has `multiple` attribute set to appropriate state before appending options
fixes #13222

* Add dom test fixture to test long multiple select scrolling to the first selected option
fixes #13222

* typo fix

* update comment
remove redundant conversion to bool type

* change a way of assigning property to domElement

* Remove unused ref on select fixture form
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants