Skip to content

feat(ui5-multicombobox): Implement latest accessibility spec #1564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
May 4, 2020
Merged

feat(ui5-multicombobox): Implement latest accessibility spec #1564

merged 4 commits into from
May 4, 2020

Conversation

ivoplashkov
Copy link
Member

@ivoplashkov ivoplashkov commented Apr 29, 2020 •
edited
Loading

FIXES:#1285

fifoosid
fifoosid suggested changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

One thing I noticed is that the aria-selected of the token is not set correctly(it is always false). I know it is not from this change, but I think it is related to the acc of the multicombobox.

@ivoplashkov
Copy link
Member Author

Looks good to me.

One thing I noticed is that the aria-selected of the token is not set correctly(it is always false). I know it is not from this change, but I think it is related to the acc of the multicombobox.

I will look into it, thanks.

fifoosid
fifoosid suggested changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the following bug should be fixed and it's good to go
ezgif com-video-to-gif (1)

elenastoyanovaa
elenastoyanovaa reviewed Apr 30, 2020
View reviewed changes
Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved from ACC side.

vladitasev
vladitasev requested changes May 4, 2020
View reviewed changes
packages/main/src/Token.js
@@ -152,6 +153,7 @@ class Token extends UI5Element {

if (isEnter(event) || isSpace(event)) {
this.fireEvent("select", {});
this.selected = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this set to false?

vladitasev
vladitasev requested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the bug, reported by Filip. The whole component jumps whenever you click on a token.

@ilhan007
Copy link
Member

ilhan007 commented May 4, 2020

I think the bug is reproducible in the current master, when you click a token
https://sap.github.io/ui5-webcomponents/master/playground/main/pages/MultiComboBox/

@vladitasev vladitasev merged commit b0917d5 into SAP:master May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elenastoyanovaa elenastoyanovaa elenastoyanovaa left review comments

@vladitasev vladitasev vladitasev approved these changes

@fifoosid fifoosid fifoosid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ivoplashkov @ilhan007 @elenastoyanovaa @vladitasev @fifoosid