fix: avoid using initial step in validation

[pekam]

setting the min or max property should not trigger step-based validation

fix #420

[Haprog]

@pekam I think proper fix for this would be to finally refactor things so that step property (of number field) has no default value. This is not the first time this has caused an issue. But I guess changing that would be a breaking change, so I'm fine with this workaround for now.

[tomivirkki]

If you set a custom step value with an attribute, the internal <input>'s step becomes "any"
<vaadin-number-field step="1.5"></vaadin-number-field>

[pekam]

If you set a custom step value with an attribute, the internal 's step becomes "any"

Fixed by also checking step === 1. The only problem is that it doesn't recognize if user sets step="1" as attribute. In this case the step won't be used in validation (same as with the default step).

I think this is the best we can do. WDYT?

[Haprog]

@pekam I think you could use getAttribute('step') to check if step has been set as an attribute to use it in validation (when attribute is set, even if it is "1").

Edit: actually maybe not since it has reflectToAttribute: true :( But maybe you could test if it's the first step change handler, if property hasn't been reflected to attribute yet (if it is done after that), then you could rely on checking the attribute in the first call to the handler.

[pekam]

@Haprog I've tested that. Unfortunately the step attribute has been updated on the first observer call, no matter if it was set as an attribute or property. So it's not possible to recognize if it was set originally as an attribute or property.

[pekam]

If we consider the expected behavior to be:
Step should be used in validation when user has set it as an attribute or property,
then this PR in its current form fixes most cases, although not covering everything.

But the problem is that it will break one case which was previously working as expected:

• Setting step="1" as an attribute or step=1 (as number) as property
• Also setting min and/or max

Expected: validated with step

Although this previously worked just because setting min or max triggers step validation, it's a case that would be a regression from this PR.

So I'm actually not sure if this can be merged. It is fixing more than it's breaking, but it is breaking something that previously worked.

[Haprog]

I guess we could maybe remove reflectToAttribute (from step) and set the attribute manually at the end of the property change listener. Or can you think of some problems with that?

[pekam]

That doesn't help in this case since it's initially defined as an attribute, which is reflected to property. It propagates this way even when reflectToAttribute=false.

I'm trying if I can use attributeChangedCallback.

[Haprog]

That doesn't help in this case since it's initially defined as an attribute, which is reflected to property. It propagates this way even when reflectToAttribute=false.

I don't think it matters in this case if it has been propagated from attribute to property already. If reflectToAttribute is false and in first step property change handler we check if the attribute is there we know that the developer explicitly set the attribute and validation should be used.

[Haprog]

Well I guess that would only solve the problem with setting it via attribute to "1" but it would still be an issue if the developer wanted to set it via the property to 1. Maybe that could be documented as a known issue (until we can refactor to get rid of the explicit default value for step) so developer could workaround it by setting it as attribute instead of property if they need it initially set to 1 and to have validation.

[pekam]

reflectToAttribute doesn't make any difference. Both the property and attribute are already updated when running the observer.

[pekam]

But it seems that we can check if the attribute was set in connectedCallback, before Polymer does its magic.

[Haprog]

reflectToAttribute should make the difference that it doesn't automatically reflect to attribute at all, right? As I see it that is the only thing needed to make it work as setting via attribute.

reflectToAttribute: false:

• first _stepChanged called where (!this.__stepChangedCalled && step === 1) equals true:
  • check getAttribute('step'), if attribute is found, use validation, user has explicitly set the attribute.
  • if no attribute is found at this point, just set input step to any like before
  • at the end setAttribute('step', step)

[pekam]

D'oh, you're right. Sorry about my brain fart. :D Of course it makes a difference in the case when setting only the property but not the attribute.

[Haprog]

So basically when reflectToAttribute is true, then in the step change handler the attribute is always set and we don't know if it was set by user or by polymer (from property), but when reflectToAttribute is false, in the first call to the step change we know that if the attribute is there, it was set by the user, and if it's not there it was not set by the user.

[Haprog]

The harder case is if the user tries to set step to 1 (either via attribute or property) after the component is ready/initialized already. (since then the default value 1 is already in both property and attribute and change events might not happen if it's being set to the same value it's at already (I don't remember if it's the same case with setting attribute?).

Not sure if it would help to replace step "Polymer property" with setter and getter methods. I guess property setter would be called even if you set the same value to it again?

[pekam]

I have tried overriding the set step() function but it doesn't get called. Polymer apparently overrides that. If the property is not declared in properties, then the setter is called. Trying to investigate it more.

That case at least has an easy workaround: setting the property as a string "1".

[Haprog]

Ok, I think this is probably as good as it can be fixed without breaking changes.

I think we should refactor the step property to not have a default value (but internally default to 1 for calculations if needed) and make a new major alpha version with that breaking change so this situation would improve and simplify the code when we can bump the new major version to stable and include in some later version of platform.

[pekam]

make a new major alpha version with that breaking change so this situation would improve and simplify the code when we can bump the new major version to stable and include in some later version of platform.

IMO it's not such a big deal that it would be worth to fix for the Polymer-based platform majors, which won't be long-term supported. But when rebuilding with Lit, this should be taken care of.

[Haprog]

I guess so, but I'm mainly thinking if it's not a big effort to change it might be easier to do now so we won't forget about it and implement the same logic with default value when porting to Lit. It would be less refactoring required when doing the Lit version if this part was done already.

[pekam]

Issue created #435. We can continue discussion on this elsewhere.

One more thing, @Haprog do you agree with my latest commit which I pushed after your approval? Read the full commit message for relevant info.

[Haprog]

I agree

[pekam]

Ok, thanks for the review! :)