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.

Sign up for GitHub

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

Jump to bottom

Fix regression in option selected #6874

Closed
wants to merge 4 commits into from
Closed

Fix regression in option selected #6874

wants to merge 4 commits into from

Conversation

RaiVaibhav
Copy link
Contributor

This adds the selected attr to the dom element and later using the hasAttribute it checks if any of the option have selected attribute then don't set selectedIndex to -1, this way it avoids setting the selectedIndex on mount

Fixes: #6873

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@RaiVaibhav
Fix: regression in option selected
ace6bc0 
This add the selected attr to the dom element
and later using the hasAttribute it check if any
of the option have selected atrribute then don't set
selectedIndex to -1, this way it avoid setting the
selectedIndex on mount

Fixes: sveltejs#6873
@RaiVaibhav RaiVaibhav force-pushed the fix/regression-in-selected-attr branch from b620b46 to ace6bc0 Compare October 22, 2021 20:02
@RaiVaibhav RaiVaibhav changed the title WIP: Fix regression in option selected Fix regression in option selected Oct 22, 2021
@RaiVaibhav
Copy link
Contributor Author

The other way around to solve the issue was to add a check before the select.selectedIndex = -1 where it checks if the value is null or undefined but I wasn't confident about going in that direction because of the value comparison.

This PR is open for review, tests will be updated and added after the initial review

dummdidumm
dummdidumm reviewed Oct 23, 2021
View reviewed changes
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling this! Could you add tests that would fail without this fix?

src/runtime/internal/dom.ts Outdated
@@ -534,16 +534,17 @@ export function set_style(node, key, value, important) {
}

export function select_option(select, value) {
let count_selected_attr = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is used as a boolean, so I suggest to make it a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Will revert 👍 after digging a little I figured there will be a bug with this logic, can you tell me what will the value of val in this case https://svelte.dev/repl/d8faa8db7ce84059bb633dd969b429eb?version=3.44.0

@RaiVaibhav
Fix: behavior when value is null and add test
369468a 
when value is null in that selectedIndex set same
as selected but in that that it should be -1.
RaiVaibhav
RaiVaibhav commented Oct 23, 2021
View reviewed changes
src/runtime/internal/dom.ts Outdated
@@ -534,17 +534,15 @@ export function set_style(node, key, value, important) {
}

export function select_option(select, value) {
let count_selected_attr = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part of the code reverted because it will create a bug when value is null or something similar like ""

RaiVaibhav
RaiVaibhav commented Oct 23, 2021
View reviewed changes
src/runtime/internal/dom.ts
@@ -556,6 +554,14 @@ export function select_options(select, value) {

export function select_value(select) {
const selected_option = select.querySelector(':checked') || select.options[0];
if (select.selectedIndex === -1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select_value get's called when value is void 0 but before this get's called select_option from dom will get's called which will set the select index to -1, and we need the -1 value to make this #6126 work.

RaiVaibhav
RaiVaibhav commented Oct 23, 2021
View reviewed changes
src/compiler/compile/render_dom/wrappers/Element/index.ts
block.chunks.mount.push(b`
(${data}.multiple ? @select_options : @select_option)(${this.var}, ${data}.value);
('value' in ${data} && !${data}.multiple && @select_option(${this.var}, ${data}.value));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reason - selection option gets called even though the rest is something like {...{}} which will again create a bug

@RaiVaibhav RaiVaibhav force-pushed the fix/regression-in-selected-attr branch from 71bd316 to 97344f4 Compare October 24, 2021 08:07
@RaiVaibhav
Copy link
Contributor Author

Hey @dummdidumm do let me know if this PR needs any update

@RaiVaibhav RaiVaibhav mentioned this pull request Nov 23, 2021
Closed
@dummdidumm
Copy link
Member

The bug was fixed by #8371

@dummdidumm dummdidumm closed this Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dummdidumm dummdidumm dummdidumm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

select with spread doesn't work normally
2 participants
@RaiVaibhav @dummdidumm