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($vdom): Don't replace input for text-like type change, fix #6313 #6344

Merged
merged 6 commits into from
Sep 1, 2017

Conversation

nickmessing
Copy link
Member

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
fix #6313

@@ -48,14 +48,12 @@ function sameVnode (a, b) {
)
}

// Some browsers do not support dynamically changing type for <input>
// so they need to be treated as different nodes
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not relevant since it affects browsers that Vue doesn't support.

@@ -27,6 +25,8 @@ import {
isPrimitive
} from '../util/index'

const isTextInputType = makeMap(TEXT_INPUT_TYPES)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could move function isTextInputType to src/platforms/web/util/element.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

@javoski, done, any more feedback?

}).$mount()
const node = vm.$el.children[0]
vm.$el.children[0].value = 'test'
vm.ok = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be vm.show? Same goes at 155.

Copy link
Member Author

Choose a reason for hiding this comment

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

😆, lol, that is a dumb mistake :D

waitForUpdate(() => {
expect(vm.$el.children[0]).toBe(node)
expect(vm.$el.children[0].value).toBe('test')
vm.show = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also assert the input type attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsonet, done, any more feedback?

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Aug 14, 2017

I would suggest to skip warning in v-model if two changed types are all text-like. We can support full dynamic expression instead of only ternary expression.

We can apply the logic in dom-props.js. (Also need to update mustUseProp to add input type combination)

@yyx990803
Copy link
Member

@nickmessing I removed the ternary support parts to make this PR more focused. I have some ideas on how to fully support dynamic types on v-model but that will be part of 2.5.

@yyx990803 yyx990803 merged commit f76d16e into vuejs:dev Sep 1, 2017
@nickmessing nickmessing deleted the text-like-input-vdom branch September 1, 2017 23:20
FranklinTesla pushed a commit to FranklinTesla/vue that referenced this pull request Sep 5, 2017
ztlevi pushed a commit to ztlevi/vue that referenced this pull request Feb 14, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conditional change of <input> type attribute not working.
7 participants