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

fix(runtime-dom): enable set form attr to null on form-elements (#2840) #2849

Merged
merged 3 commits into from
Feb 3, 2021

Conversation

luwuer
Copy link
Contributor

@luwuer luwuer commented Dec 18, 2020

Close #2840

Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

Just a typo needing to be fixed, see comment.

packages/shared/src/domTagConfig.ts Outdated Show resolved Hide resolved
Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

Typo need to be fixed, see comment.

Edit: commited the fix myself.

Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

This should be good now!

@LinusBorg LinusBorg added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. ready to merge The PR is ready to be merged. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. and removed ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. labels Dec 19, 2020
@posva posva removed the ready to merge The PR is ready to be merged. label Dec 19, 2020
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
@posva posva added the ready to merge The PR is ready to be merged. label Dec 21, 2020
@LinusBorg
Copy link
Member

LinusBorg commented Dec 21, 2020

I thought about this a bit more, and I think this solution is good for this individual issue, but there are more readonly properties out there that could fail like this.

Example: #2766

I think the core reason is that the last line of shouldSetAsProp only checks for the existance of the key, but not wether it has a setter:

https://github.com/vuejs/vue-next/blob/92d7ad18296fe43a01d10f38e03b983188dfba5a/packages/runtime-dom/src/patchProp.ts#L112

Instead of adding new exceptions for individual properties like we do here, we could do something like this:

return key in el && !!Object.getOwnPropertyDescriptor(el.__proto__, key)?.set

But I'm not 100% sure if thats a) performance and b) safe for all elements. It works for not the button.form and textarea.type scenarios.

Is there another (better?) way to check if a prop is readonly?

@LinusBorg LinusBorg added need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. and removed ready to merge The PR is ready to be merged. labels Dec 21, 2020
@posva
Copy link
Member

posva commented Dec 21, 2020

We could still cache it for perf reasons but I wonder if there isn't an exception in a particular browser that makes the property readonly without setting the proper flag (you never know with JS!)

I don't find having a list of exception that bad unless it begins to get too big. It also allows for specific checks and better handling of edge cases

@LinusBorg
Copy link
Member

agreed. I sent a seperate PR for the textarea thing. This one here is again good to go.

@LinusBorg LinusBorg added ready to merge The PR is ready to be merged. and removed need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. labels Dec 26, 2020
@LinusBorg LinusBorg added this to Planned in Next Patch Feb 1, 2021
@LinusBorg LinusBorg moved this from Planned to Approved in Next Patch Feb 1, 2021
@LinusBorg LinusBorg merged commit f262438 into vuejs:master Feb 3, 2021
Next Patch automation moved this from Approved to Done Feb 3, 2021
@yyx990803 yyx990803 moved this from Done to Final (Reviewed by Evan) in Next Patch Feb 24, 2021
@yyx990803
Copy link
Member

Removed tag check in 180310c

The form tag list adds to the baseline size and isn't really worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to unset a 'form' prop on a button with null value.
5 participants