Remove tabindex of -1 on upload image input in the write a post editor

[Manaswini1832]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

In the "Write a post" editor, the "Upload Image" button is skipped while tabbing i.e. while navigating with a keyboard, making it inaccessible to keyboard users. This happens due to the tabindex of -1 set on the "Upload Image" input element. Removing it enables tabbing, thus solving the issue.

Related Tickets & Documents

Resolves #10521 (second part of the issue)

This article talks more about how a tabindex of -1 on HTML elements that need to be interacted with, makes them inaccessible to keyboard users

Added tests?

• yes
• no, because they aren't needed
• no, because I need help

Added to documentation?

• docs.forem.com
• readme
• no documentation needed

[CLAassistant]

All committers have signed the CLA.

[ludwiczakpawel]

Just to be clear, what happens currently (in the prod) is:

• Focus on title.
• [TAB]
• Focus on tags.
• [TAB]
• Focus on body.

What instead is being proposed here in this PR is:

• Focus on title.
• [TAB]
• Focus on tags.
• [TAB]
• Focus on image uploader.
• [TAB]
• Focus on body.

^ I'm not sure if this is an improvement - it's very unlikely someone will ever want to upload an image BEFORE writing a content.
Another thing is that we may soon add more controls to the toolbar (so not only image uploader but also "bold", "italic", "list" etc..) which means that user would have to tab several time to get to the body field.

I'm not saying the image uploader should NOT be accessible with keyboard - it's definitely accessibility issue. I'm saying it should be accessible in slightly different way... Maybe (just maybe):

• Focus on title.
• [TAB]
• Focus on tags.
• [TAB]
• Focus on body.
• [TAB]
• Focus on image uploader.

?

[Manaswini1832]

Yes @ludwiczakpawel that sounds fair. Since giving a negative tab index to any element on the page will skip tabbing to it, adding a positive tab index seems to be the only solution that I can think of right now. So if

1. A positive tab index (of let's say 1) is added to the title, tags and the content and
2. The negative tab index on the image uploader is removed,

then the order of tabbing that you specified will work. Should I go ahead with that then?

[ludwiczakpawel]

If it gonna work like that, then I think so.. I'm not an expert with figuring out tabindexes etc :D so i'd have to play with it first.

[Manaswini1832]

I've done some research and I've come to realize that using positive tab indexes might not be a very good idea.

• This article gives a good explanation why

Also, it can make our lives much harder in the future when more elements will have to be introduced on the page and then we'll have to manually check and change the tab indexes of almost every element on the page.

On experimenting a little bit with the Dev tools on the editor view, I came upon this solution, which is to introduce a "Skip to content" link above the Image uploader.

This GIF shows the tabbing order if the user doesn't want to skip to the main content

Now, this GIF shows the tabbing order if the user WANTS TO SKIP to the main content

OR

If the "Skip to content" button needs to be avoided, then we could settle for the previous solution too, where I removed the tab index of -1 from the image uploader. In this case, even though the user reaches the image uploader before writing any content, they can tab ahead if they wish to not upload an image and then tab back when they want to upload something.

The following GIF shows this solution in action

For this solution though, the pull request I made earlier has a small mistake that I'll correct if this is the solution that we will be going for.

[Link2Twenty]

Personally, I think things should be tabbed through in the order they appear on the screen. People that use a mouse won't notice and people that need to use their keyboard are aware that they will need to tab over things to get to what they want.

I also think it's worth thinking about keyboard shortcuts

We could do something quite simple like this then a user could press ALT + I, or something similar to add an image.

[Manaswini1832]

Yeah the third GIF that I attached does this. Tabbing follows the order of the elements on the page. If a user doesn't want to upload an image before writing, they can simply tab ahead.
I'm not sure how to implement the keyboard shortcuts though.

[nickytonline]

Thanks so much for this fix @Manaswini1832. I've tested it out and it works great. One thing we should address within this PR is to also give it some focusable styling. I knew the button would work because of the PR description, but visually there is no indicator so show it has focus. Most likely the hover state would be good for this. cc: @ludwiczakpawel

[Manaswini1832]

Thanks for the review! I missed removing the negative tab index on the outer button of the image uploader, which I'll correct and create a proper pull request here again. For the hover state, we could possibly go with an outline on the button.

[nickytonline]

I'll defer to @ludwiczakpawel for the focus styling. Outline, the default focus styling is a good option, but let's wait to hear back from Pawel. Thanks for being patient.

[ludwiczakpawel]

Outline, the default focus styling is a good option

+1

[Manaswini1832]

I've figured out how to tab to the uploader by altering the tab index but I'm not able to figure out how to apply this hover style to that uploader button. I'm unable to get to the class that has to be changed to apply that hover effect. Could you all help please?

OR

Should I create an issue for this asking for improvements in the focus styles and leave this pull request to include only the tab index changes?

[ludwiczakpawel]

@Manaswini1832 I can help you with with styling but I've just pulled your branch locally and there's something off: I need to tab twice through "Upload image" button now..

UPDATE: As mentioned above: there's something off with jumping between fields since I had to tab through the "Upload image" button twice. It looks like all you have to do is bring back first of the negative indexes (so you gonna have that negative tabindex on <Button /> element but not on the input inside.: https://github.com/forem/forem/pull/10543/files#diff-3ee8f3fc4573e7aa12fab2d9f4b5f7fbL138

As for styling, in _buttons.scss file, replace line 71 with:

&:focus:enabled,
&:focus-within {

^ this should do the trick.

[Link2Twenty]

Is there are reason we have button wrapping an input?

[ludwiczakpawel]

@Link2Twenty Nope. Likely a mixture of legacy code with I'll get back to this later.

[Manaswini1832]

UPDATE: As mentioned above: there's something off with jumping between fields since I had to tab through the "Upload image" button twice. It looks like all you have to do is bring back first of the negative indexes (so you gonna have that negative tabindex on <Button /> element

Yep got it! I'll do that right away

[Manaswini1832]

I've done this but unexpectedly the build failed. Wonder why that happens. Any clues?

[Link2Twenty]

There error is

Failure/Error: expect(page).to have_text(new_comment_text)

Which is this code though I don't think your changes should have caused this.

[Manaswini1832]

Yeah. I just changed the buttons.scss and the ImageUploader.jsx files

[ludwiczakpawel]

I'm restarting the build for you, lets see if that helps 🤷‍♂️

[Manaswini1832]

That helped! Thank you so much 😊

[nickytonline]

Looks good to me @Manaswini1832. I tested it out in Safari, Firefox, Edge and Chrome. Thanks so much for the PR! 👏🏻

[Manaswini1832]

Thanks a lot for helping me out with this PR :D