Skip to content
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

menu, menu-item: improve JAWS readability #1578

Merged
merged 4 commits into from
Aug 13, 2015
Merged

menu, menu-item: improve JAWS readability #1578

merged 4 commits into from
Aug 13, 2015

Conversation

sipayRT
Copy link
Contributor

@sipayRT sipayRT commented Jul 10, 2015

Closes #1508 #1458

@sipayRT sipayRT added the ready label Jul 10, 2015
@tadatuta tadatuta mentioned this pull request Jul 13, 2015
Closed
20 tasks
@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 14, 2015

@aristov need your review

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 20, 2015

@aristov ping. Need your review here

@aristov
Copy link
Contributor

aristov commented Jul 20, 2015

2nd item of #1508 description is not satisfied.
When virtual cursor comes to __group-title DOM-node, JAWS reads "[title] group box", but shouldn't. Use presentation role to avoid it.

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 21, 2015

@aristov I tried to prevent it by presentation role, but it not works for me (I added this attribute - you can check it). To fix it we can add 'aria-hidden' attribute for group-title element.

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 22, 2015

@aristov tests are failed because of role="presentation". I'll fix it after your review

@aristov
Copy link
Contributor

aristov commented Jul 23, 2015

To fix it we can add 'aria-hidden' attribute for group-title element.

Ok, use aria-hidden

@aristov
Copy link
Contributor

aristov commented Jul 23, 2015

2nd item of #1508 description is not satisfied.

Don't forget this. Try to use aria-activedescendant and aria-label attributes to get the described behaviour.

aristov
aristov reviewed Jul 23, 2015
View reviewed changes
common.blocks/menu-item/menu-item.bemhtml
'menuitem',
attrs = {
role : role,
'aria-disabled' : mods.disabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit aria-disabled="false"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if block will be without disabled mod 'aria-disabled' attribute will not be added. So, this attribute will be added just in case when disabled mod will be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you talking about _disabled_false case — we don't need to support it. See documentation for details

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 24, 2015

Don't forget this. Try to use aria-activedescendant and aria-label attributes to get the described behaviour.

@aristov but when menu is focused no one menu-item has hovered state and we don't know which item should be said by JAWS. We can set hovered modifier for first item on menu focus, but I think it's not good idea.

@aristov
Copy link
Contributor

aristov commented Jul 25, 2015

@sipayRT no _hovered. We should use the first menu-item_checked for that purpose.

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 27, 2015

@aristov set id to first menu item on menu hover. it looks better now

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 30, 2015

@aristov this branch was rebased and updated. Need your review

@aristov
Copy link
Contributor

aristov commented Aug 3, 2015

JAWS doesn't speak content of the first menu-item of menu-focused (2nd item of #1508). Use aria-label to fix it.

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 6, 2015

@aristov I just checked it one more time - on TAB event (when menu is focused) JAWS says "menu, item one not checked" and then "to move ... bla bla bla". Maybe you should clean your caches

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 11, 2015

Remove aria-checked attribute if menu hasn't any mode modifier

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 11, 2015

Clear uniqId on blur

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 11, 2015

Try to generate id for each menu-item

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 11, 2015

@aristov check now, plz

aristov
aristov reviewed Aug 12, 2015
View reviewed changes
common.blocks/menu/__group/menu__group.bemhtml
@@ -6,7 +6,7 @@ block('menu').elem('group')(
}),
content()(function() {
return [
{ elem : 'group-title', content : this.ctx.title },
{ elem : 'group-title', attrs : { 'aria-hidden' : true }, content : this.ctx.title },
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's wrap this long line.

@aristov
Copy link
Contributor

aristov commented Aug 12, 2015

I've checked the second point of #1508:

  • on menu without _mode it works properly
  • on menu_mode_* JAWS reads the first menu-item wrongly

I have no idea why it works so and how to fix it. Besides, @sipayRT can not reproduce this. We assume that this is a JAWS internal bug.

I think we should leave the solution, based on the permanent ids on menu-items. It looks more proper, than move id from item to another.

Now all points of issue are satisfied, but there are some notes in the code changes. Fix it and ping me again, please.

@sipayRT
Copy link
Contributor Author

sipayRT commented Aug 13, 2015

@aristov All your notes were fixed.

aristov pushed a commit that referenced this pull request Aug 13, 2015
Merge pull request #1578 from /issues/1508@v2
094b290 
menu, menu-item: improve JAWS readability
@aristov aristov merged commit 094b290 into issues/1206-common@v2 Aug 13, 2015
@aristov aristov removed the ready label Aug 13, 2015
@aristov aristov deleted the issues/1508@v2 branch August 13, 2015 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sipayRT @aristov @narqo