- Notifications
You must be signed in to change notification settings - Fork27.4k
fix(input): update $viewValue when cleared#14772
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Cool! I believe writing a test is difficult because we can't click the clear button? |
2445f95 to1da5563CompareYeah - fussing around with this a bit to get the right condition currently, but I think the right fix might be attainable here. |
This is a little tough - so falsy model changes will convert to the empty string, so this potentially breaks some behavior. Any ideas on good workarounds for that situation? |
6273af0 tod997158Comparesrc/ng/directive/input.js Outdated
| var timeout; | ||
| var timeout, oldVal; | ||
| var viewValueUpdated = true, msieInput = msie >= 10 && msie <= 11; | ||
| if (msieInput && inputType === 'text') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why only text inputs? Won't types such as number have the same issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like you're correct - I assumed it would have the spinner, but apparently not.
| @@ -1152,10 +1158,18 @@ function baseInputType(scope, element, attr, ctrl, $sniffer, $browser) { | |||
| } | |||
| }; | |||
| function ieListener(ev) { | |||
| var val = element.val(); | |||
| if (val === oldVal && !viewValueUpdated) return; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it possible that the validity changed but the value did not (similar to thevalue === '' && ctrl.$$hasNativeValidators check inlistener)? Although I'm not seeing how that would have been handled previously either (in the!hasEvent('input') case)...
src/ng/directive/input.js Outdated
| var timeout; | ||
| var timeout, oldVal; | ||
| var viewValueUpdated = false, msieInput = msie >= 10 && msie <= 11; | ||
| if (msieInput && attr.type in IE_INPUTS_WITH_CLEARING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we even care about the type for setting the initial oldVal though? I'd think we'd always want to setup theoldVal ifmsieInput is true...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldn't that trigger a potentially unnecessary DOM access though? That is likely more expensive than this check I think. I originally had it just accessing the value, I'd be fine changing it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we keep it I think it should be part of themsieInput condition, not this setup condition.
But I'd thinkelement.val() is cheap enough it doesn't matter.
There are some complicated scenarios I'm having trouble figuring out a good solution to. Here is an incomplete table:
|
- Fix when user clicks clear button in an input element in IE, $viewValue not being correctly updated
Wouldn't it be easier to just add an extra listener on IEs: element.on('??mousedown/mouseup/click??',function(event){deferListener(event,this,this.value);}); |
@wesleycho Do you still want to work on this? |
Don't have the bandwidth anymore :( . |
blemaire commentedJun 8, 2018
What's the status on this issue (not doubt i'm not the only one encountering this) |
It has been abandoned (afaict) 😁 |
This shouldfix#11193.