refactor: add methods to make setting invalid overridable

[vursen]

Description

• Added a protected method _shouldSetInvalid(invalid) that determines whether the invalid property should be updated to the given invalid state (by default: always true).
• Added a protected method _setInvalid(invalid) that updates the invalid property to the given invalid state if it passes _shouldSetInvalid. This method is supposed to be used by the web components wherever they need to update invalid internally instead of manipulating the property directly.

The main purpose of _shouldSetInvalid is to make it possible later for Flow to prevent the web components from committing client-side validation success: the final decision on whether a component is valid will be made by the server.

Part of #4081

Type of change

• Internal

Checklist

• I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

[DiegoCardoso]

The main purpose of _shouldCommitValidationResult is to make it possible for Flow to later prevent setting invalid to true on the client-side: only the server will decide when the validation is considered successful.

If I understood it correctly, it should be the other way around. The Flow counterpart should allow the web component to set invalid to true, but disallow it to set it back to false. (ping @yuriy-fix)

[vursen]

If I understood it correctly, it should be the other way around. The Flow counterpart should allow the web component to set invalid to true, but disallow it to set it back to false.

Ah, you are right 👍 . It is just a typo by the way. I corrected the description.

[web-padawan]

Would it make sense to simplify method names e.g. _shouldSetInvalid() and _setInvalid() to better describe what they do?

[vursen]

Would it make sense to simplify method names e.g. _shouldSetInvalid() and _setInvalid() to better describe what they do?

My concern at the beginning was that this naming might be confusing especially when you override shouldSetInvalid and then anyway set invalid on your own, but now when I think about that again, I see that the alternative naming brings more overhead than solves anything.

So I agree to use more straightforward naming. 👍 This way it will be in line with setFocused() and shouldSetFocus().

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[DiegoCardoso]

I have noticed that ErrorController implements setInvalid, but didn't check if it might affect the new behavior. Could you double check it?

[web-padawan]

I have noticed that ErrorController implements setInvalid, but didn't check if it might affect the new behavior

The new method is actually in the ValidateMixin and it has nothing to do with controllers. Unlike mixins that wrap each other (and can override methods), controllers are separate objects that do not have this caveat.

[DiegoCardoso]

The new method is actually in the ValidateMixin and it has nothing to do with controllers. Unlike mixins that wrap each other (and can override methods), controllers are separate objects that do not have this caveat.

Ah, right. I wasn't fully aware of the difference between mixins and controllers. Got it now, thanks.