fix: make controls respond to the invalid event

[blm768]

Rather than emitting invalid events on blur, we should be listening for them.

Fixes #407

Proposed Changes

• FormElementMixin.checkHtml5Validity checks the validity property rather than calling checkValidity, so it doesn't emit invalid events on blur.
• Controls that use FormElementMixin respond to invalid events, which the browser fires when their containing form is submitted.

Open Questions

• Should we suppress the browser's native HTML5 validation pop-ups?
  When the form is submitted, most browsers will focus the first invalid field and display a pop-up with a validation message. This is standard behavior for plain HTML inputs, but for Oruga controls inside a field, the pop-up is redundant because we provide our own user-visible error message. Should we suppress the browser's native pop-up by default?

[netlify]

✅ Deploy Preview for oruga-documentation-preview ready!

Name	Link
🔨 Latest commit	032183b
🔍 Latest deploy log	https://app.netlify.com/sites/oruga-documentation-preview/deploys/63ae20836a21770008c2939c
😎 Deploy Preview	https://deploy-preview-420--oruga-documentation-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[jtommy]

Should we suppress the browser's native HTML5 validation pop-ups?
When the form is submitted, most browsers will focus the first invalid field and display a pop-up with a validation message. This is standard behavior for plain HTML inputs, but for Oruga controls inside a field, the pop-up is redundant because we provide our own user-visible error message. Should we suppress the browser's native pop-up by default

Yes we can avoid it

[jtommy]

@blm768 the PR looks ok but probably it's better release it in the next breaking version (0.6.0), what do you think?

[blm768]

Waiting until 0.6.0 should be OK; I've got a workaround that I can use for this in the meantime. That'd also give us more time to evaluate suppressing the browser's native pop-up messages. We'll probably only want to do that for controls with a parent field, and it might be worthwhile to make it configurable, although the existing useHtml5Validation flag might be good enough.

[astagi]

@blm768 in the meantime check some github comments about linting :) is it ready for review? I can mark it as breaking and remind to merge in the next version as @jtommy suggested! Let me know!

[codecov]

Codecov Report

Base: 63.43% // Head: 62.80% // Decreases project coverage by -0.63% ⚠️

Coverage data is based on head (032183b) compared to base (1f4cdf1).
Patch coverage: 9.52% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #420      +/-   ##
===========================================
- Coverage    63.43%   62.80%   -0.64%     
===========================================
  Files           79       79              
  Lines         5467     5527      +60     
  Branches      1513     1535      +22     
===========================================
+ Hits          3468     3471       +3     
- Misses        1897     1946      +49     
- Partials       102      110       +8     

Flag	Coverage Δ
oruga	65.92% <6.89%> (-0.39%)	⬇️
oruga-next	48.70% <11.76%> (-1.41%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...-next/src/components/autocomplete/Autocomplete.vue	52.05% <ø> (ø)
...oruga/src/components/autocomplete/Autocomplete.vue	75.00% <ø> (ø)
...ges/oruga/src/components/datepicker/Datepicker.vue	75.17% <ø> (ø)
...a/src/components/datetimepicker/Datetimepicker.vue	86.89% <ø> (ø)
packages/oruga/src/components/input/Input.vue	80.00% <ø> (ø)
...ges/oruga/src/components/inputitems/Inputitems.vue	35.96% <ø> (ø)
packages/oruga/src/components/select/Select.vue	82.35% <ø> (ø)
...ges/oruga/src/components/timepicker/Timepicker.vue	87.87% <ø> (ø)
packages/oruga-next/src/utils/FormElementMixin.ts	44.44% <6.45%> (-17.88%)	⬇️
packages/oruga/src/utils/FormElementMixin.js	54.73% <6.89%> (-21.39%)	⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[blm768]

The first commit in this branch is basically in a reviewable state, although I should probably add some tests. The second commit, which tries to get rid of the browser's native validation tooltip/pop-up, has an issue that will need some further discussion.

The basic problem, as it turns out, is that there's not a "good" (well-supported, standards-based, and with minimal code) way to scroll an element into view only if it's not already in the view. All of the "simple" options have problems:

• scrollIntoViewIfNeeded: fairly OK browser support (around 95%), but not standardized and unlikely to ever be standardized.
• scrollIntoView:
  • Always scrolls even if the element is already onscreen, which is kind of annoying (doesn't match the browser's default behavior for scrolling to a focused field)
  • Doesn't have broad support for the block/inline positioning options (not the end of the world, but another deviation from the behavior for scrolling to a focused field, which generally uses the nearest mode)
• Just focus the input rather than the parent field
  • Works, but if the input is off the bottom of its scroll container, the message will likely still be offscreen when scrolling finishes, which is at least annoying and may also be confusing depending on how invalid inputs get styled.

So far, the optimal solution (from a usability perspective) seems to be to use a third-party polyfill for a feature that may be standardized eventually. That doesn't seem like a great option in terms of keeping Oruga's dependencies and bundle size minimal.

At this point, it seems like we've got a handful of fairly reasonable options to choose from (possibly including a combination of options):

1. Use scrollIntoViewIfNeeded on browsers that support it. On others, just focus the input (which may leave the message cut off).
2. Don't try to suppress the browser's native pop-ups. They'll naturally scroll like the "bad case" for Option 1, but the pop-up means that the user should always see a message. (We could combine this with Option 1 and just leave the native pop-up alone in browsers that don't support scrollIntoViewIfNeeded.)
3. Do one of the above, but provide a configuration option that lets the user pass in their own handler function for reporting invalid events. This would be the most flexible and allow individual users to decide for themselves whether to pull in a dependency to get "ideal" scrolling behavior.

Does one of those options sound clearly better than the others?

[blm768]

Looks like this issue will make automated tests tricky.

[astagi]

@blm768 the work in this branch is going to finish, we're ready to review all the breaking changes :)

About your PR, lgtm and I also agree with you that Oruga should not come with extra dependencies (polyfills included). I don't know which is the best option, seems ok for me to use scrollIntoViewIfNeeded only on browsers that support it and just focus the input on others. The third option (handler function defined by users) looks also good, do you mean to make users able to define their own handler function alongside a standard behaviour (option 1 & 2)?

@jtommy any thoughts?

[blm768]

The third option (handler function defined by users) looks also good, do you mean to make users able to define their own handler function alongside a standard behaviour (option 1 & 2)?

Yep; that's what I had in mind. We'd probably use the standard behavior if the user doesn't provide a handler function and skip our built-in behavior entirely if the user does provide one.

[jtommy]

The third option (handler function defined by users) looks also good, do you mean to make users able to define their own handler function alongside a standard behaviour (option 1 & 2)?

Yep; that's what I had in mind. We'd probably use the standard behavior if the user doesn't provide a handler function and skip our built-in behavior entirely if the user does provide one.

I agree with you!