Skip to content

fix(ui5-input): Announce selected item #1578

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.

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

Merged
merged 13 commits into from
May 26, 2020

Conversation

ivoplashkov
Copy link
Member

FIXES:#1306

@@ -831,6 +833,14 @@ class Input extends UI5Element {
};
}

get itemSelectionAnnounce() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code logically belongs to InputSuggestions.js. The clue to this is that you don't actually use the state of Input.js, but refer to this.Suggestions for everything inside. Move it there, and in this method, if suggestions are imported by the user, just return a method call to suggestions

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.

Almost good to go, just remove onDefine

@@ -831,6 +831,10 @@ class Input extends UI5Element {
};
}

get itemSelectionAnnounce() {
return this.Suggestions.selectedItemIndex !== null ? this.Suggestions.itemSelectionAnnounce : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I think you have to check if this.Suggestions is present, because it is undefined if "showSuggestions" is false.

@ivoplashkov
Copy link
Member Author

With the current implementation, when we try to close the popover using "Escape" key, the "_showListAnnouncement" property is removed, which triggers the control's re-rendering and the "shouldOpenSuggestions" method returns "true" which leads to re-opening of the popover. Either we need to extend the "shouldOpenSuggestions" logic or think of a different approach.

@vladitasev
Copy link
Contributor

@ivoplashkov The state of a component should be completely deterministic. This means that whenever all values are the same (state is the same), the component should look exactly the same way too. If something differs, depending on open/closed popup, then there is "hidden" state that is not accounted for, therefore you get problems when you invalidate the component. Introduce new state, if necessary, so that each invalidation is non-breaking.

@elenastoyanovaa
Copy link
Contributor

Approved from ACC side.

@@ -40,6 +40,7 @@

{{#if showSuggestions}}
<span id="{{_id}}-suggestionsText" class="ui5-hidden-text">{{suggestionsText}}</span>
<span id="{{_id}}-selectionText" class="ui5-hidden-text" aria-live="polite" role="status"></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

bind a new private metadata state property here,inside the span, f.e. {{_announcementText}}
don't forget to declare it in metadata

@@ -798,6 +801,16 @@ class Input extends UI5Element {
};
}

_announceSelectedItem() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this function change the value of the new property. This will invalidate and set it. Don't manipulate DOM directly.

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.

I would still like to see my comments implemented, but I realize with the current implementation bigger changes must be done for this to happen. Let's merge this in the current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants