refactor: default tabindex property to undefined in TabIndexMixin #3627

vursen · 2022-03-31T16:57:45Z

Description

The PR refactors TabIndexMixin so that its tabindex property defaults to undefined rather than 0 which achieves fewer overrides in the components. There are only two cases at this moment when tabindex needs to be initialized as 0: it is vaadin-button and the legacy ShadowFocusMixin.

Type of change

Bugfix

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.

packages/grid/src/vaadin-grid.js

vursen · 2022-03-31T18:56:03Z

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

         * @protected
         */
        tabindex: {
          type: Number,
-          value: 0,


note: It would be better to have it null by default to entirely avoid undefined. However, that might be a breaking change for some components, so I decided not to change this behavior for now.

web-padawan

Overall looks good. Let's update JSDoc annotation for the mixin as it still says:

By default, the attribute is set to 0 that makes the element focusable.

vursen · 2022-04-01T06:37:37Z

Overall looks good. Let's update JSDoc annotation for the mixin as it still says:

@web-padawan Thanks, updated.

vursen · 2022-04-01T08:22:45Z

packages/field-base/src/delegate-focus-mixin.js

+           * @protected
+           * @override
+           */
+          _lastTabIndex: {


note: It turned out that DelegateFocusMixin requires _lastTabIndex to be 0 by default even though the tabindex attribute is undefined by default.

If I remove it, the tests start failing. Seems that the __forwardTabIndex method is tied on the 0 value. It looks suspicious and it might be that a bug is hiding here, but I decided not to consider it in the scope of this refactoring.

packages/field-base/src/delegate-focus-mixin.js

vursen · 2022-04-01T09:19:47Z

packages/grid/src/vaadin-grid.js

-    super._disabledChanged(disabled, oldDisabled);
-
-    if (oldDisabled) {
-      this.removeAttribute('tabindex');


note: The tabindex attribute is now removed by TabIndexMixin, which has become possible as a result of making _lastTabIndex default to undefined.

sissbruecker · 2022-04-01T09:49:38Z

packages/button/src/vaadin-button.js

+       * @protected
+       */
+      tabindex: {
+        value: 0


I think this will affect the vaadin-select refactoring, which introduces a button mixin: https://github.com/vaadin/web-components/pull/3612/files#diff-936c1ce6ea70b0b86633d2f358c5c3cfa916ecfe6ad5376b5244ddbb04002315R18

If we want to apply tabindex=0 to the vaadin-select-value-button as well (which we do currently), we have to move this to the new button mixin. It looks like the snapshot tests for select should cover this, but just wanted to mention it regardless.

It is a good reminder, thanks!

This reverts commit aab5175.

sonarqubecloud · 2022-04-06T12:37:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

)

) (#3641) Co-authored-by: Sergey Vinogradov <[email protected]>

) (#3640) Co-authored-by: Sergey Vinogradov <[email protected]>

vaadin-bot · 2022-04-14T09:17:51Z

This ticket/PR has been released with Vaadin 23.1.0.alpha2 and is also targeting the upcoming stable 23.1.0 version.

vursen requested review from web-padawan and sissbruecker March 31, 2022 18:12

vursen commented Mar 31, 2022

View reviewed changes

packages/grid/src/vaadin-grid.js Show resolved Hide resolved

vursen commented Mar 31, 2022

View reviewed changes

web-padawan reviewed Apr 1, 2022

View reviewed changes

vursen commented Apr 1, 2022

View reviewed changes

vursen changed the title ~~fix: restore tabindex attribute after re-enabling grid~~ refactor: default tabindex property to undefined in TabIndexMixin Apr 1, 2022

vursen changed the title ~~refactor: default tabindex property to undefined in TabIndexMixin~~ refactor: default tabindex property to undefined in TabIndexMixin Apr 1, 2022

web-padawan reviewed Apr 1, 2022

View reviewed changes

packages/field-base/src/delegate-focus-mixin.js Outdated Show resolved Hide resolved

vursen force-pushed the fix/grid/restoring-tabindex-after-switching-disabled branch from ee345e6 to 0e1e9b7 Compare April 1, 2022 09:13

vursen changed the title ~~refactor: default tabindex property to undefined in TabIndexMixin~~ refactor: default tabindex property to undefined in TabIndexMixin Apr 1, 2022

vursen commented Apr 1, 2022

View reviewed changes

sissbruecker reviewed Apr 1, 2022

View reviewed changes

sissbruecker approved these changes Apr 1, 2022

View reviewed changes

web-padawan approved these changes Apr 1, 2022

View reviewed changes

vursen force-pushed the fix/grid/restoring-tabindex-after-switching-disabled branch from 1900480 to c97ceae Compare April 4, 2022 07:49

vursen mentioned this pull request Apr 6, 2022

test: skip flaky grid scrolling mode test for WebKit #3637

Merged

7 tasks

vursen force-pushed the fix/grid/restoring-tabindex-after-switching-disabled branch 2 times, most recently from bfa0059 to 485bba1 Compare April 6, 2022 09:57

vursen added target/22.0 target/23.0 labels Apr 6, 2022

vursen added 8 commits April 6, 2022 15:33

test: add more tests

6e1731d

refactor: set tabindex property to undefined by default

9f2d2ec

fix: restore tabindex attribute after switching disabled

ffd82dd

remove unnecessary changes

fc06ef6

align JSDoc annontations

b618689

apply code review suggestions

1a6c5e5

Revert "fix: restore tabindex attribute after switching disabled"

906c68d

This reverts commit aab5175.

remove no longer necessary tests

decd3a6

vursen added 2 commits April 6, 2022 15:33

apply code review suggestions

7a0c4d7

apply code review suggestions

0addf5b

vursen force-pushed the fix/grid/restoring-tabindex-after-switching-disabled branch from 485bba1 to 0addf5b Compare April 6, 2022 12:33

vursen merged commit e156bfb into master Apr 6, 2022

vursen deleted the fix/grid/restoring-tabindex-after-switching-disabled branch April 6, 2022 12:52

vaadin-bot pushed a commit that referenced this pull request Apr 6, 2022

refactor: default tabindex property to undefined in TabIndexMixin (#3627

37bfc7b

)

vaadin-bot added the cherry-picked-23.0 label Apr 6, 2022

vaadin-bot pushed a commit that referenced this pull request Apr 6, 2022

refactor: default tabindex property to undefined in TabIndexMixin (#3627

8fe9453

)

vaadin-bot added the cherry-picked-22.0 label Apr 6, 2022

vursen added a commit that referenced this pull request Apr 6, 2022

refactor: default tabindex property to undefined in TabIndexMixin (#3627

2cddeed

) (#3641) Co-authored-by: Sergey Vinogradov <[email protected]>

web-padawan pushed a commit that referenced this pull request Apr 6, 2022

refactor: default tabindex property to undefined in TabIndexMixin (#3627

d29e5ad

) (#3640) Co-authored-by: Sergey Vinogradov <[email protected]>

vaadin-bot added prerelease version for Vaadin 23.1.0 Released with Vaadin 23.1.0.alpha2 labels Apr 14, 2022

web-padawan mentioned this pull request Dec 31, 2024

refactor: do not override tabindex property to set default value #8418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: default tabindex property to undefined in TabIndexMixin #3627

refactor: default tabindex property to undefined in TabIndexMixin #3627

vursen commented Mar 31, 2022 •

edited

Loading

vursen Mar 31, 2022 •

edited

Loading

web-padawan left a comment

vursen commented Apr 1, 2022 •

edited

Loading

vursen Apr 1, 2022 •

edited

Loading

vursen Apr 1, 2022 •

edited

Loading

sissbruecker Apr 1, 2022

vursen Apr 1, 2022 •

edited

Loading

sonarqubecloud bot commented Apr 6, 2022

vaadin-bot commented Apr 14, 2022

refactor: default tabindex property to undefined in TabIndexMixin #3627

refactor: default tabindex property to undefined in TabIndexMixin #3627

Conversation

vursen commented Mar 31, 2022 • edited Loading

Description

Type of change

Checklist

vursen Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

web-padawan left a comment

Choose a reason for hiding this comment

vursen commented Apr 1, 2022 • edited Loading

vursen Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

vursen Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

sissbruecker Apr 1, 2022

Choose a reason for hiding this comment

vursen Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 6, 2022

vaadin-bot commented Apr 14, 2022

vursen commented Mar 31, 2022 •

edited

Loading

vursen Mar 31, 2022 •

edited

Loading

vursen commented Apr 1, 2022 •

edited

Loading

vursen Apr 1, 2022 •

edited

Loading

vursen Apr 1, 2022 •

edited

Loading

vursen Apr 1, 2022 •

edited

Loading