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: make <select> <option value> behavior consistent #12316

Merged
merged 2 commits into from
Jul 8, 2024
Merged

fix: make <select> <option value> behavior consistent #12316

merged 2 commits into from
Jul 8, 2024
+94 −1

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jul 5, 2024 •
edited
Loading

Setting the value attribute of an <option> element to a falsy value should result in the empty string. This wasn't happening in all situations previously. Fixes #11616

While this fixes the behavior, I'm also somewhat questioning whether or not this should be the way it works. In other situations a falsy value means removing the attribute. In this case it's not. On the other hand for value on input elements it's the same behavior and it makes sense there, mostly because the value property is not reflected as an attribute - but for option elements it is. So there are arguments both in favor and against this behavior. Either way, it should be consistent, and this makes it consistent with the Svelte 4 behavior (and fixes the spreading inconsistency that was also present in Svelte 4)

Side note: During this I checked what the value attribute does for different elements. It's ... really different by element, to say the least:

  • progress: no attribute -> indeterminate state, progress bumps around
  • meter: no attribute -> value 0
  • option: no attribute -> uses text value
  • input: no attribute -> no default value
  • textare: no attribute -> no default value
  • button: no attribute -> empty string value

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 and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@dummdidumm
fix: make <select> <option value> behavior consistent
08b9599 
Setting the `value` attribute of an `<option>` element to a falsy value should result in the empty string. This wasn't happening in all situations previously.
Fixes #11616
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 5, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 7e55810

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

dummdidumm
dummdidumm commented Jul 5, 2024
View reviewed changes
packages/svelte/src/internal/client/dom/elements/attributes.js Outdated
Comment on lines 162 to 171
if (is_option_element && key === 'value') {
// Because of the "set falsy option values to the empty string" logic below, which can't
// differentiate between a missing value and an explicitly set value of null or undefined,
// we need to remove the attribute here and delete the key from the object.
element.removeAttribute(key);
delete current[key];
delete next[key];
} else {
next[key] = null;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a shame we need this additional code to be 100% correct. Since it's impossible to remove the value attribute in the case you set it through a regular (non-spreaded) attribute I'd be totally ok with removing this extra code and adding a comment to the other logic below about a super-edge-case-bug present and the deliberate tradeoff for less code

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to remove this logic and live with the super-edge-case (which is also described in the updated comment)

@dummdidumm dummdidumm merged commit 63f3ee4 into main Jul 8, 2024
7 of 9 checks passed
@dummdidumm dummdidumm deleted the consistent-option-value branch July 8, 2024 19:07
@github-actions github-actions bot mentioned this pull request Jul 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trueadm trueadm trueadm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@dummdidumm @trueadm