refactor!: update checkbox to use slotted input #2539

vursen · 2021-09-15T08:48:27Z

Description

The PR re-implements <vaadin-checkbox> to make it use slotted input. Using slotted input improves the checkbox's accessibility as proven by the prototype.

Along the way, the PR introduces the label attribute for the checkbox. The attribute can be used as an alternative way to define a label for a checkbox besides using the default slot.

Re-implement <vaadin-checkbox> to use the slotted input.
Adapt the Lumo theme and corresponding visual tests to the slotted input.
Adapt the Material theme and corresponding visual tests to the slotted input.
Make unit tests of <vaadin-checkbox> pass.
Make unit tests of <vaadin-checkbox-group> pass.
Make unit tests of <vaadin-grid-pro> pass (see the comment).

Fixes #2212
Fixes #980

Type of change

Breaking change

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.

vursen · 2021-09-17T11:26:38Z

packages/component-base/src/active-mixin.js

-      this.addEventListener('blur', () => {
-        this._setActive(false);
-      });


note: Having this blur listener causes the checkbox to lose the active attribute in case the user switches the checkbox by clicking on the label (for some reason, there is fired the blur event on <vaadin-checkbox> (!!) while moving focus between the input and the label).

Didn't have enough time to investigate that all the way but I assume the blur listener can be removed altogether.
It originally came to the mixin from the button component. However, even for the button, it doesn't look relevant since we changed its implementation so that there are not any native buttons.

vursen · 2021-09-17T11:46:20Z

packages/checkbox/test/visual/lumo/checkbox.test.js

+    });
+
+    it('empty', async () => {
+      element.label = '';


note: The margin between the label and the input is defined separately for the RTL and LTL modes. The margin should disappear when there is no label. That being so, I thought it is reasonable to test the empty label case for both modes.

vursen · 2021-09-17T14:02:09Z

Some of the visual tests are failing now due to an issue that is not actually caused by this PR:

packages/custom-field/test/visual/lumo/custom-field.test.js:

 ❌ custom-field > alignment > label + helper text > label + helper text alignment
      Error: Visual diff failed. New screenshot is 0.01% different.
      See diff for details: /Users/vursen/dev/vaadin/web-components/packages/custom-field/test/visual/lumo/screenshots/custom-field/failed/alignment-label-helper-text-diff.png
        at async o.<anonymous> (packages/custom-field/test/visual/lumo/custom-field.test.js:185:9)

~~Failing tests are going to be fixed in #2532.~~ Fixed.

vursen · 2021-09-20T09:27:42Z

One of the <vaadin-grid-pro> unit tests is still failing because the click event fired on the checkbox gets prevented here:

 ❌ edit column editor type > checkbox > should update value from checkbox checked after edit mode exit
      AssertionError: expected 'true' to equal 'false'
      + expected - actual
      
      -true
      +false
      
      at r (node_modules/@esm-bundle/chai/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:9576:21)
      at node_modules/@esm-bundle/chai/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:251:8
      at p (node_modules/@esm-bundle/chai/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:1410:10)
      at c (node_modules/@esm-bundle/chai/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:7884:30)
      at handleDom (node_modules/@open-wc/semantic-dom-diff/chai-dom-diff.js:102:21)
      at f (node_modules/@esm-bundle/chai/node_modules/.pnpm/[email protected]/node_modules/chai/chai.js:9164:38)
      at packages/vaadin-grid-pro/test/edit-column-type.test.js:113:56

... and I'm currently searching for a way to fix it.

packages/checkbox-group/src/vaadin-checkbox-group.d.ts

packages/checkbox-group/test/checkbox-group.test.js

packages/checkbox/src/vaadin-checkbox.d.ts

packages/checkbox/test/checkbox.test.js

packages/vaadin-grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js

web-padawan

Thanks @vursen for your attention to detail 💯

sonarcloud · 2021-09-21T11:55:29Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
0.0% Duplication

vaadin-bot · 2021-10-12T05:14:17Z

This ticket/PR has been released with platform 22.0.0.alpha7 and is also targeting the upcoming stable 22.0.0 version.

vursen changed the title ~~feat!: re-implement checkbox with new mixins and slotted input (WIP)~~ feat!: re-implement checkbox with slotted input and new mixins (WIP) Sep 15, 2021

vursen changed the title ~~feat!: re-implement checkbox with slotted input and new mixins (WIP)~~ feat!: re-implement checkbox using slotted input and new mixins (WIP) Sep 15, 2021

web-padawan mentioned this pull request Sep 15, 2021

[checkbox] Should we introduce the label property? #980

Closed

vursen force-pushed the feat/checkbox branch from 2e76c04 to 576cfae Compare September 15, 2021 12:30

vursen changed the title ~~feat!: re-implement checkbox using slotted input and new mixins (WIP)~~ refactor!: update checkbox to use slotted input (WIP) Sep 16, 2021

vursen force-pushed the feat/checkbox branch 4 times, most recently from b8e1833 to d82402e Compare September 17, 2021 09:27

vursen commented Sep 17, 2021

View reviewed changes

vursen changed the title ~~refactor!: update checkbox to use slotted input (WIP)~~ refactor!: update checkbox to use slotted input Sep 17, 2021

vursen force-pushed the feat/checkbox branch from 7ede8a7 to 859cce8 Compare September 20, 2021 08:59

vursen marked this pull request as ready for review September 20, 2021 09:30

web-padawan requested changes Sep 20, 2021

View reviewed changes

vursen commented Sep 20, 2021

View reviewed changes

packages/vaadin-grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js Show resolved Hide resolved

web-padawan approved these changes Sep 20, 2021

View reviewed changes

vursen added 11 commits September 21, 2021 10:35

refactor!: use new mixins for checkbox

b1d3e4e

fix: update Lumo visual tests

816c2ef

fix: update Material visual tests

b4b66bb

refactor: switch checked state on change event

ea4fda2

feat: introduce _shouldSetActive method for ActiveMixin

df0a675

refactor: adapt checkbox to mixins' changes

0eb56c1

fix: update unit tests

127b5d0

fix: prevent input from losing focus while animation

75810d4

test: clean unit tests

447f406

fix: update type declarations

9f9a0da

chore: improve comment writing

e4db172

vursen added 12 commits September 21, 2021 10:35

fix: adjust CheckboxGroup to the new Checkbox

dd94a5a

chore: improve comment writing

634656f

chore: improve comment writing, clean package.json

14608aa

refactor: set default value in constructor

1b906f6

chore: add more code comments

914446e

fix: delegate checked property to input

600ebe3

chore: improve comment writing

743e716

fix: don't prevent default behaviour for the click event in grid pro

69f0985

fix: apply code review suggestions

79fc420

test: add tests on custom element definition

caf06b6

test: re-organize tests on active attribute

6af2f3b

test: improve test descriptions writing

f96ef7a

vursen force-pushed the feat/checkbox branch from 9ec9cb9 to f96ef7a Compare September 21, 2021 07:35

vursen added 2 commits September 21, 2021 14:11

fix: toggle checkbox in grid pro

9e50772

chore: add more code comments

470d836

web-padawan approved these changes Sep 21, 2021

View reviewed changes

vursen requested a review from yuriy-fix September 21, 2021 11:45

chore: improve comment writing

8b3859b

web-padawan removed the request for review from yuriy-fix September 21, 2021 12:02

web-padawan merged commit 8d21187 into master Sep 21, 2021

web-padawan deleted the feat/checkbox branch September 21, 2021 12:05

tomivirkki mentioned this pull request Sep 22, 2021

Toggling a checkbox in grid in interaction mode doesn't work #2610

Closed

This was referenced Sep 22, 2021

Avoid teleporting nodes in SlotTargetMixin #2609

Closed

Add missing integration tests for the Checkbox label vaadin/flow-components#2152

Closed

vursen mentioned this pull request Sep 30, 2021

[checkbox] Regression: the checkbox is not clickable over the whole area #2693

Closed

vaadin-bot added prerelease version for platform 22.0.0 Released with Platform 22.0.0.alpha7 labels Oct 12, 2021

vursen mentioned this pull request Oct 25, 2021

fix: don't mention no longer existing CSS parts in JSDoc #2918

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: update checkbox to use slotted input #2539

refactor!: update checkbox to use slotted input #2539

vursen commented Sep 15, 2021 •

edited

Loading

vursen Sep 17, 2021 •

edited

Loading

vursen Sep 17, 2021 •

edited

Loading

vursen commented Sep 17, 2021 •

edited

Loading

vursen commented Sep 20, 2021 •

edited

Loading

web-padawan left a comment

sonarcloud bot commented Sep 21, 2021

vaadin-bot commented Oct 12, 2021

refactor!: update checkbox to use slotted input #2539

refactor!: update checkbox to use slotted input #2539

Conversation

vursen commented Sep 15, 2021 • edited Loading

Description

Type of change

Checklist

vursen Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

vursen Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

vursen commented Sep 17, 2021 • edited Loading

vursen commented Sep 20, 2021 • edited Loading

web-padawan left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 21, 2021

vaadin-bot commented Oct 12, 2021

vursen commented Sep 15, 2021 •

edited

Loading

vursen Sep 17, 2021 •

edited

Loading

vursen Sep 17, 2021 •

edited

Loading

vursen commented Sep 17, 2021 •

edited

Loading

vursen commented Sep 20, 2021 •

edited

Loading