Fix43788 removed aria haspopup on the control element#11053
Conversation
Annett7811
left a comment
There was a problem hiding this comment.
Hello!
From an accessibility perspective, this looks good. Thank you very much for your work.
Best regards,
Annett
oliversamoila
left a comment
There was a problem hiding this comment.
Hello together,
from an UX and UI perspective, the result looks good. Thank you very much for your contribution, @satyammangroliya.
I'm forwarding this to @thibsy for code review. He may contact you with change requests or incorporate the changes directly (That includes cherry picking too.)
Kind regards,
@oliversamoila (as UI coordinator)
thibsy
left a comment
There was a problem hiding this comment.
Thx a lot for your contribution to the UI framework!
I believe the actual HTML changes are OK – good catch that you adjusted the old and new components here. However, I think the unit tests probably need to be adjusted to these changes as well, otherwise our pipelines fail. For some reason I cannot re-run the pipelines and also not see the logs (due to age of this PR). Could you perform these locally and check?
Kind regards,
@thibsy (as UI coordinator)
|
Hello @satyammangroliya, Many thanks and best regards, |
|
Dear @thibsy , |
|
Hello @thibsy , Thank you for the hint. I checked the affected tests locally and found several failing expectations related to the removed aria-haspopup attribute. I updated the corresponding tests and verified the changes locally. The pipeline is now passing. Kind regards, |
aria-haspopup on the control element “Sort test results” is unfavorable
it is noted for the area of the button for sorting test results that the aria-haspopup attribute on the control element is unfavorable, since according to the specification it may only be used for certain types (or roles) of inserted elements (menu, listbox, tree, grid or dialog). It should therefore be removed, as users may otherwise expect a different type of operation.
https://mantis.ilias.de/view.php?id=43788