fix($theme-default): Make navbar dropdown links accessible #1837

edisdev · 2019-09-05T20:14:09Z

Summary

What kind of change does this PR introduce? (check at least one)

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

Hi !
I've edited the accessibility of items on the navbar. (for this issue #1818 )

…uepress into accessibility-dropdown

missmatsuko

Thanks for getting into this! I added a couple notes re: accessibility.
Maybe adding the skip nav link should be a different issue/PR.
It's sort of related, but can be separate.

missmatsuko · 2019-09-07T19:35:49Z

packages/@vuepress/theme-default/__tests__/components/__snapshots__/DropdownLink.spec.js.snap

  <ul class="nav-dropdown" style="display: none;" name="dropdown">
    <li class="dropdown-item">
-      <!----> <a class="nav-link">Guide</a></li>
+      <!----> <a class="nav-link" tabindex="8">Guide</a></li>


We should try to avoid the use of positive values for tabindex if possible:
https://webaim.org/techniques/keyboard/tabindex

missmatsuko · 2019-09-07T19:38:38Z

packages/@vuepress/theme-default/components/DropdownLink.vue

@@ -5,7 +5,9 @@
  >
    <a


The NavLink element should be only when it acts as a link (e.g. has a valid href value). If it's acting as a trigger to open a dropdown navigation, it should be a . This should probably remove the need to set an explicit tabindex ( without href is not focusable, but is focusable, at least on Chrome).

missmatsuko · 2019-09-07T19:43:14Z

packages/@vuepress/theme-default/components/DropdownLink.vue

@@ -149,7 +160,8 @@ export default {
 @media (min-width: $MQMobile)
  .dropdown-wrapper
    height 1.8rem
-    &:hover .nav-dropdown
+    &:hover .nav-dropdown,
+    &:focus-within .nav-dropdown


Opening the submenu on focus-within works ok, but it could be annoying if there's a navbar with many long submenus (i.e. you'd have to tab through every single item to get past the navbar). Adding a skip navigation link could help with that (and probably should also be added regardless), but a keyboard user would still have to tab through every submenu item on the way to the one they actually want.

The Adobe Mega Menu is a good example of the expected keyboard interactions and aria markup:

thx for your explanation @missmatsuko

@missmatsuko Thank you for comment and review :) You are right, Maybe it can make to focus submenus when key press the enter. Or, can be added focusable property to config file. and i will make tabindex="0" for focusable items . What do you think about this ?

I think there doesn't need to be an tabindex attribute at all. Can the link element be changed to a if it has children submenu items, and an anchor if it has an href?

I made testing when I remove tabindex, element can not focusable in safari and firefox. @missmatsuko

missmatsuko · 2019-09-09T13:11:18Z

Is it because the element is an <a> with no href attribute set? The element should be focusable if this is changed to a <button> element. A button is more appropriate here anyways because it isn't a link to a page - it should be used to open/close the submenu.

…

On Mon., Sep. 9, 2019, 12:04 a.m. Hatice Edis, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/theme-default/components/DropdownLink.vue <#1837 (comment)>: > @@ -149,7 +160,8 @@ export default { @media (min-width: $MQMobile) .dropdown-wrapper height 1.8rem - &:hover .nav-dropdown + &:hover .nav-dropdown, + &:focus-within .nav-dropdown I made testing when I remove tabindex, element can not focusable in safari and firefox. @missmatsuko <https://github.com/missmatsuko> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1837?email_source=notifications&email_token=AEIOIJSSLLG7P7FZAO4ZEUTQIXYRVA5CNFSM4IUCARBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEAYCGI#discussion_r322089822>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEIOIJVWHRJKZEBD7RQ3GYDQIXYRVANCNFSM4IUCARBA> .

edisdev · 2019-09-09T13:34:17Z

Unluckily, element has href property but it can not focusable. But as you say , can change dropdown open/close element with button. @missmatsuko

(Edit : This problem is due to the firefox version, likely. Thank you for support :) I will remove tabindex and make dropdown toggle element with button. should those with false properties still be given -1 tabindex to add a focusable property?)

Is it because the element is an with no href attribute set? The element should be focusable if this is changed to a element. A button is more appropriate here anyways because it isn't a link to a page - it should be used to open/close the submenu.
…
On Mon., Sep. 9, 2019, 12:04 a.m. Hatice Edis, @.> wrote: @.* commented on this pull request. ------------------------------ In @.***/theme-default/components/DropdownLink.vue <#1837 (comment)>: > @@ -149,7 +160,8 @@ export default { @media (min-width: $MQMobile) .dropdown-wrapper height 1.8rem - &:hover .nav-dropdown + &:hover .nav-dropdown, + &:focus-within .nav-dropdown I made testing when I remove tabindex, element can not focusable in safari and firefox. @missmatsuko https://github.com/missmatsuko — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1837?email_source=notifications&email_token=AEIOIJSSLLG7P7FZAO4ZEUTQIXYRVA5CNFSM4IUCARBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEAYCGI#discussion_r322089822>, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIOIJVWHRJKZEBD7RQ3GYDQIXYRVANCNFSM4IUCARBA .

edisdev · 2019-09-09T18:46:16Z

What do you think about this ? @f3ltron

Unluckily, element has href property but it can not focusable. But as you say , can change dropdown open/close element with button. @missmatsuko

(Edit : This problem is due to the firefox version, likely. Thank you for support :) I will remove tabindex and make dropdown toggle element with button. should those with false properties still be given -1 tabindex to add a focusable property?)

Is it because the element is an with no href attribute set? The element should be focusable if this is changed to a element. A button is more appropriate here anyways because it isn't a link to a page - it should be used to open/close the submenu.
…
On Mon., Sep. 9, 2019, 12:04 a.m. Hatice Edis, @.> wrote: _@**._* commented on this pull request. ------------------------------ In _@_.*/theme-default/components/DropdownLink.vue <#1837 (comment)>: > @@ -149,7 +160,8 @@ export default { @media (min-width: $MQMobile) .dropdown-wrapper height 1.8rem - &:hover .nav-dropdown + &:hover .nav-dropdown, + &:focus-within .nav-dropdown I made testing when I remove tabindex, element can not focusable in safari and firefox. @missmatsuko https://github.com/missmatsuko — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1837?email_source=notifications&email_token=AEIOIJSSLLG7P7FZAO4ZEUTQIXYRVA5CNFSM4IUCARBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEAYCGI#discussion_r322089822>, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIOIJVWHRJKZEBD7RQ3GYDQIXYRVANCNFSM4IUCARBA .

missmatsuko · 2019-09-09T20:02:16Z

The anchor with javascript in the href should be a <button> element because it doesn't act as a link, it opens a submenu.
Link Behavior

Also sorry for breaking out of the thread, I didn't know replying by email would do that. 😬

edisdev · 2019-09-09T21:11:54Z

No problem :) @missmatsuko As i said, I will turn the anchor a button element. I think, focusable property can be optional. As you said, if submenus has so many element, It may not be comfortable and useful.

edisdev · 2019-09-10T23:02:31Z

Hi! @f3ltron @kefranabg . I did an new improvement, according last those described. I think pull request is ready of completely.

flozero · 2019-09-10T23:11:13Z

@missmatsuko are you ok with changes ? As i am not so good with accessibility. I will try but i have no real idea if it's good or not

missmatsuko · 2019-09-11T02:16:01Z

I'm confused why notFocusable is a config setting. Nav items should just not be focusable if it's hidden (i.e. in a closed submenu).

edisdev · 2019-09-13T07:58:58Z

@missmatsuko Thank you so much for your help 🤗😊🙏

edisdev · 2019-09-13T08:19:31Z

Pr is approved :) @kefranabg

Looks good!

kefranabg · 2019-09-13T08:24:42Z

I'll do a review ASAP. Thanks for your work 💪

kefranabg

Awesome work @edisdev 💪 I just made a few comments. Let me know if you have questions or want some help.

kefranabg · 2019-09-14T20:31:20Z

packages/@vuepress/theme-default/components/NavLink.vue

-    isTel
+    isTel,
+    keypressTab () {
+      this.$emit('keypressTab')


Could you rename this event keypress-tab. Other events names in the vuepress code uses - syntax.

kefranabg · 2019-09-14T20:35:43Z

packages/@vuepress/theme-default/components/DropdownLink.vue

+  },
+
+  computed: {
+    AllItemsCount () {


should be named with a lower-case a

kefranabg · 2019-09-14T20:43:32Z

packages/@vuepress/theme-default/components/DropdownLink.vue

@@ -65,12 +70,34 @@ export default {
  props: {
    item: {
      required: true
+    },
+    dropdownName: {


Why using a props for the dropdown name? Why don't we just keep it as a local variable in the component? Btw I think we should consider a way to make this label customizable. The user might want to display different labels depending on the current selected language. This should be a default theme local. Could you add this feature and update the documentation?

kefranabg · 2019-09-14T21:01:12Z

packages/@vuepress/theme-default/components/DropdownLink.vue

            </li>
          </ul>

          <NavLink
            v-else
+            @keypressTab="focusoutDropdown(index + 1)"


I don't think that focusoutDropdown is a correct name for what the function is doing, right?

kefranabg · 2019-09-14T21:05:48Z

packages/@vuepress/theme-default/components/DropdownLink.vue

            >
-              <NavLink :item="childSubItem"/>
+              <NavLink
+                @keypressTab="focusoutDropdown((index + 2) * (subIndex + 1))"


I don't understand the + 2? Could you explain? Why not just + 1?

I'm sorry, There was a bug here. I resolved this. 👍

kefranabg · 2019-09-14T21:42:34Z

I also found a bug:

The dropdown will immediately close when the last child element of it is focused

…item index

edisdev · 2019-09-15T08:46:59Z

Hi @kefranabg :) Thank you so much review, i resolved your comments :) And also i fixed your said bug. 👍

kefranabg · 2019-09-15T09:41:03Z

packages/@vuepress/theme-default/components/NavLinks.vue

@@ -54,6 +54,7 @@ export default {
        const themeLocales = this.$site.themeConfig.locales || {}
        const languageDropdown = {
          text: this.$themeLocaleConfig.selectText || 'Languages',
+          ariaLabel: this.$themeLocaleConfig.ariaLabel || 'Languages',


The default value should be Select language

kefranabg · 2019-09-15T09:41:16Z

packages/docs/docs/.vuepress/config.js

@@ -39,6 +39,7 @@ module.exports = ctx => ({
      '/': {
        label: 'English',
        selectText: 'Languages',
+        ariaLabel: "Languages",


The value should be Select language

kefranabg · 2019-09-15T12:50:37Z

packages/@vuepress/theme-default/components/DropdownLink.vue

  methods: {
    toggle () {
      this.open = !this.open
+    },
+
+    getSubItemIndex (index, subIndex) {


This part is kind of hard to read/understand, I'll try to find something easier for this part

I too will try to find to a more easy solution.

kefranabg · 2019-09-15T12:52:06Z

We should also add a example for aria-label in this documentation page

edisdev · 2019-09-15T13:07:53Z

@kefranabg i will resolve immediately your comments. 👍

edisdev · 2019-09-15T14:08:35Z

@kefranabg I added a example for aria-label in documentation page . But, i could not write description for Chinese example, because i don't know Chinese. :(

kefranabg · 2019-09-15T14:21:30Z

packages/docs/docs/theme/default-theme-config.md

@@ -72,6 +72,25 @@ module.exports = {
 }
 ```

+You can change `aria-label` of dropdown button to with a custom aria-label. Default value is text property. if the dropdown menu consists of language items., default value is 'Select language'


I don't think we need an entire section for aria-label 😉 What I meant was just adding ariaLabel: 'Language Menu' in existing examples in this page. And I think you can do the same for Chineese. Sorry I wasn't clear about this 😛

Okey, i wil add that in existing examples :)

kefranabg · 2019-09-15T14:22:28Z

@edisdev You'll find my proposition in the last commit to simplify the close dropdown algorithm. What do you think?

edisdev · 2019-09-15T14:32:40Z

I think code is more readable and clear now :) Thank you :)

@edisdev You'll find my proposition in the last commit to simplify the close dropdown algorithm. What do you think?

kefranabg

Thanks again!

edisdev · 2019-09-15T14:45:28Z

Thank you so much too :) @kefranabg

vue-bot · 2019-09-15T14:52:00Z

Thanks again! 💚

edisdev added 2 commits September 5, 2019 23:04

vuejs#1818 | Make Accessibility Elements in navbar

311f11f

fix: Change to dropdownlink test

44462b6

edisdev changed the title ~~#1818 | Make Accessibility Elements in navbar~~ Refactoring: #1818 | Make Accessibility Elements in navbar Sep 5, 2019

edisdev changed the title ~~Refactoring: #1818 | Make Accessibility Elements in navbar~~ ref: #1818 | Make Accessibility Elements in navbar Sep 5, 2019

edisdev changed the title ~~ref: #1818 | Make Accessibility Elements in navbar~~ refactor: Make Accessibility Elements in navbar Sep 5, 2019

edisdev added 2 commits September 6, 2019 00:30

fix: change to testing for dropdownlink component

3593b4b

Merge branch 'accessibility-dropdown' of https://github.com/edisdev/v…

e996d18

…uepress into accessibility-dropdown

edisdev mentioned this pull request Sep 7, 2019

Accessibility: Navbar dropdown can't be used with just a keyboard #1818

Closed

1 task

edisdev added 2 commits September 7, 2019 21:26

feat: addauto opened & focus dropdown menu when keypress tab

510be5b

fix: update test for dropdownLink

380911d

missmatsuko suggested changes Sep 7, 2019

View reviewed changes

flozero approved these changes Sep 8, 2019

View reviewed changes

flozero self-requested a review September 8, 2019 04:41

flozero added need feedback Awaiting author response type: enhancement Request to enhance an existing feature version: 1.x Relates to version 1 of VuePress labels Sep 8, 2019

flozero assigned edisdev Sep 8, 2019

flozero added the status: community assigned Community assigned label Sep 8, 2019

kefranabg removed the need feedback Awaiting author response label Sep 10, 2019

edisdev added 3 commits September 11, 2019 01:17

edit tabindex to navlink & dropdownlink for not focusable

fd30a60

Update docs for notFocusable props

8bd733f

Update dropdownlink test

1b38f36

flozero assigned kefranabg Sep 13, 2019

kefranabg requested changes Sep 14, 2019

View reviewed changes

edisdev added 2 commits September 15, 2019 10:48

Rename navlink & dropdown action / computed and bugfix for find last …

0a27789

…item index

Make ariaLabel config for dropdown

96b1287

edisdev added 2 commits September 15, 2019 12:02

fix typo

c67010e

fix typo dropdown computed

e84b6c6

kefranabg requested changes Sep 15, 2019

View reviewed changes

edisdev added 2 commits September 15, 2019 16:20

edit language dropdown aria-label default value

6da2e5f

update docs for ariaLabel property

842157d

feat: Try to improve code readability

371af79

kefranabg reviewed Sep 15, 2019

View reviewed changes

add aria-label property in docs page (theme)

0ef66cc

kefranabg approved these changes Sep 15, 2019

View reviewed changes

kefranabg changed the title ~~refactor: Make Accessibility Elements in navbar~~ fix($theme-default): Make navbar dropdown links accessible Sep 15, 2019

kefranabg merged commit a8ce645 into vuejs:master Sep 15, 2019

haoranpb mentioned this pull request Sep 25, 2019

fix($theme-default): make dropdown-title's UI consistent with nav-link #1890

Merged

18 tasks

snyk-bot mentioned this pull request Apr 6, 2020

[Snyk] Upgrade vuepress from 1.1.0 to 1.3.1 vchain-us/docs#11

Closed

snyk-bot mentioned this pull request May 1, 2020

[Snyk] Upgrade vuepress from 1.0.2 to 1.4.0 adamlaska/osmos-cosmos-sdk#5

Open

fix($theme-default): Make navbar dropdown links accessible #1837

fix($theme-default): Make navbar dropdown links accessible #1837

Conversation

edisdev commented Sep 5, 2019

missmatsuko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edisdev Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

missmatsuko commented Sep 9, 2019 via email

edisdev commented Sep 9, 2019 • edited Loading

edisdev commented Sep 9, 2019

missmatsuko commented Sep 9, 2019 • edited Loading

edisdev commented Sep 9, 2019

edisdev commented Sep 10, 2019

flozero commented Sep 10, 2019

missmatsuko commented Sep 11, 2019

edisdev commented Sep 13, 2019

edisdev commented Sep 13, 2019 • edited Loading

kefranabg commented Sep 13, 2019

kefranabg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kefranabg commented Sep 14, 2019

edisdev commented Sep 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kefranabg commented Sep 15, 2019

edisdev commented Sep 15, 2019

edisdev commented Sep 15, 2019

kefranabg Sep 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kefranabg commented Sep 15, 2019

edisdev commented Sep 15, 2019

kefranabg left a comment

Choose a reason for hiding this comment

edisdev commented Sep 15, 2019 • edited Loading

vue-bot commented Sep 15, 2019

edisdev Sep 8, 2019 •

edited

Loading

edisdev commented Sep 9, 2019 •

edited

Loading

missmatsuko commented Sep 9, 2019 •

edited

Loading

edisdev commented Sep 13, 2019 •

edited

Loading

kefranabg left a comment •

edited

Loading

kefranabg Sep 15, 2019 •

edited

Loading

edisdev commented Sep 15, 2019 •

edited

Loading