refactor: do not disable buttons when moving to overflow #9027
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
web-padawan
force-pushed
the
refactor/menu-bar-buttons-disabled
branch
from
737cd02 to
4ea0861
Compare
web-padawan
commented
May 1, 2025
| @@ -160,22 +143,6 @@ describe('overflow', () => { | |||
| await nextResize(menu); | |||
|
|
|||
| expect(buttons[0].getAttribute('tabindex')).to.equal('0'); | |||
| expect(buttons[1].getAttribute('tabindex')).to.equal('-1'); | |||
There was a problem hiding this comment.
This check is not so relevant for the test, the actual thing is to ensure tabindex is set to 0 on the first visible button (which had tabindex="-1" before resizing above).
web-padawan
commented
May 1, 2025
| expect(buttons[1].getAttribute('tabindex')).to.equal('-1'); | ||
| }); | ||
|
|
||
| it('should set tabindex -1 on the overflow menu in tab navigation', async () => { |
There was a problem hiding this comment.
This is a copy-paste of the test above. With tabNavigation the logic for setting tabindex="0" on the buttons that remain visible is not needed since all enabled buttons have tabindex="0" by default. Removed the test.
web-padawan
force-pushed
the
refactor/menu-bar-buttons-disabled
branch
2 times, most recently
from
cce9d1c to
2590bd6
Compare
web-padawan
force-pushed
the
refactor/menu-bar-buttons-disabled
branch
from
2590bd6 to
c22ba0c
Compare
|
vursen
approved these changes
May 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Depends on #8871
The original implementation sets
button.disabled = truewhen hiding buttons and moving items to the overflow menu - see vaadin/vaadin-menu-bar#16 (comment). It was needed because back then the menu-bar used its own version of_getAvailableIndex()that checked fordisabledandhiddenattributes but not forvisibility.Disabling is no longer needed since both Tab and arrow keys are now handled by
KeyboardDirectionMixinwhich filters out hidden buttons usingisElementHidden()and that helper checks forvisibilityproperty internally.Updated to not modify
disabledon buttons manually so that it can be handled consistently via the Lit template, and modified the observer to call__renderButtons()ondisabledproperty change, the same as as on_themechange.Also adjusted some tests to not check for
tabindex="-1"set on the buttons that are moved to the overflow menu.This was a side-effect of setting
disabled, in fact we don't need to not modifytabindexfor those buttons.Type of change