Skip to content

A11y accessible modals #290

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 27 commits into from
Nov 6, 2019

Conversation

microbit-mark
Copy link
Contributor

microbit-mark and others added 8 commits October 30, 2019 15:34
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
@microbit-carlos
Copy link
Collaborator

Clicking inside the modals causes this "selected" effect:

image

image

Can this be avoided?

@microbit-carlos
Copy link
Collaborator

Fixed the issue with the table cells being selectable in this PR: microbit-mark#1

@microbit-mark
Copy link
Contributor Author

We can use blur on that element. Have done so inline 02b67c6, but not sure if that's the best approach.

@microbit-carlos
Copy link
Collaborator

Might be better to just remove the focus CSS?
Btw, this branch has a couple of conflicts with master, better resolve them now while we are testing, so that we don't have to retest later.

@microbit-mark
Copy link
Contributor Author

Have moved this to css and fixed the conflict

@microbit-carlos
Copy link
Collaborator

Snippets are not working on Safari, were they working before?

image
image

@microbit-carlos
Copy link
Collaborator

To avoid having something always focused when using the mouse, could we do this?

  • On modal open not focus any of the elements
  • When the tab key is pressed and about to an element outside the modal, focus the first element inside of the modal
  • Still rotate through the elements in the modal until closed

Alternatively, if we focus the element that we just edited to remove the focus CSS (01eb6dc) that would still have an element inside the modal focused and let users tab through the modal.

2019-10-31 13 09 49

@microbit-mark
Copy link
Contributor Author

microbit-mark commented Nov 1, 2019

Snippets are not working on Safari, were they working before?

I can't recreate in Safari 13.0.2. But Safari 12 exhibits the issue.

To avoid having something always focused when using the mouse...

I think good practice would be to pass the focus to the modal and trap it and show element focus if the user has entered the modal via keyboard. If they have used the mouse, don't show focus.

I have added a commit that focusses the div rather than the first selectable element

@microbit-mark
Copy link
Contributor Author

@microbit-carlos syntax error I think 8367008 only Safari prior to 13 complained about it. :)

I've tried this in browserstack, can you test locally when you have a moment @microbit-carlos

@microbit-carlos
Copy link
Collaborator

It works! Looks like i accidentally deleted one bracket more than necessary, thanks Mark!

@microbit-mark
Copy link
Contributor Author

Seem to have introduced another issue somewhere with the files help, now i've made it a button element

@microbit-carlos
Copy link
Collaborator

@microbit-mark is this PR ready or are there still things to iron out?

@microbit-mark
Copy link
Contributor Author

Ready :)

editor.html Outdated
</div>
<div id="addFile">
<div id="addFileHeader">
<h2 class="modal-title"><i class="fa fa-download"></i> <strong>{{ files-title }}</strong></h2>
</div>
<div id="addFileHelp"><a class="action" id="files-expand-help"><i class="fa fa-info-circle" aria-hidden="true"> <span>{{ help-button }}</span></i></a></div>
<div id="addFileHelp">
<button aria-labelledby="add-files" aria-expanded="false" type="button" class="snippet-button" id="expandHelpPara"><i class="fa fa-info-circle"><span>{{ help-button }}</span></i></button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think font awesome expects no content inside the <i ...></i> element. Should the text be moved after the closing </i>?
Also, it looks like <span> is not required?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not be updated as well?

outline: inherit;
}
.snippet-button:focus {
box-shadow: 0px 0px 5px 3px #FFCC33;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will need a PR in python-editor for the foundation colours, could you prepare that as well?

Copy link
Contributor Author

@microbit-mark microbit-mark Nov 5, 2019

Choose a reason for hiding this comment

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

I've pushed the branch up https://github.com/microbit-foundation/python-editor/tree/a11y-modal-focus, not sure specifically what changes to make

Copy link
Collaborator

Choose a reason for hiding this comment

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

If FFCC33 is the upstream yellow we will need a new branch from python-editor overwriting in style-foundation.css the new classes setting this colour to the foundation yellow.
This needs to be a new branch from the python-editor master, as it's in a different file not present here.

.save-button.py:focus {
box-shadow: 0px 0px 5px 3px #FFCC33;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The number of spaces for the CSS in this file is a bit of a mess, but I think right now there is more classes using 4 spaces than 2. Could we update all new classes use to use 4 spaces as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added further spacing to the css to tidy up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant the 4 spaces like this:

.save-button.py:focus {
  box-shadow: 0px 0px 5px 3px #FFCC33;
}

To:

.save-button.py:focus {
    box-shadow: 0px 0px 5px 3px #FFCC33;
}

@microbit-carlos
Copy link
Collaborator

microbit-carlos commented Nov 4, 2019

This is shaping up fantastically, great work Mark!
What browsers has this been tested in its latest iteration?
Taking in consideration how difficult it was to get it working in Chrome and Safari at the same time I think there is a high risk of having issues in other browsers. So based on previous experiences I think we should test it in:

  • Safari
  • IE10
  • IE11
  • Edge (pre-chrome)
  • Firefox Windows
  • Firefox mac
  • Chrome

The testing should include opening the modals, iterating through the elements, activating a few of them with the keyboard, click on a couple of elements to ensure the click handlers are still working, and test at least one of the snippets with the keyboard and the mouse.

@microbit-mark
Copy link
Contributor Author

microbit-mark commented Nov 5, 2019

Tests

  • opening the modals by tabbing to it and enter
  • iterating through the elements
  • activating a few of them with the keyboard
  • click on a couple of elements to ensure the click handlers are still working
  • test at least one of the snippets with the keyboard and the mouse.

✔️ Safari 13 Mac Mojave
✔️ Safari 12 Mac Mojave - Doesn't trap focus, can't focus on elements
✔️ ie10 Win 7
✔️ ie11 Win7
✔️ Edge 17 Win 10
✔️ FF 70 Win 10
✔️ FF 70 Mojave can't tab to open modals, all other checks pass
✔️ Chrome 78 Mojave
✔️ Chrome 78 Win 10

@microbit-carlos This works in all browsers, but for mac, full keyboard accessibility needs to be enabled and tabbing in safari

outline: inherit;
}
.snippet-button:focus {
box-shadow: 0px 0px 5px 3px #FFCC33;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If FFCC33 is the upstream yellow we will need a new branch from python-editor overwriting in style-foundation.css the new classes setting this colour to the foundation yellow.
This needs to be a new branch from the python-editor master, as it's in a different file not present here.

.save-button.py:focus {
box-shadow: 0px 0px 5px 3px #FFCC33;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I meant the 4 spaces like this:

.save-button.py:focus {
  box-shadow: 0px 0px 5px 3px #FFCC33;
}

To:

.save-button.py:focus {
    box-shadow: 0px 0px 5px 3px #FFCC33;
}

editor.html Outdated
</div>
<div id="addFile">
<div id="addFileHeader">
<h2 class="modal-title"><i class="fa fa-download"></i> <strong>{{ files-title }}</strong></h2>
</div>
<div id="addFileHelp"><a class="action" id="files-expand-help"><i class="fa fa-info-circle" aria-hidden="true"> <span>{{ help-button }}</span></i></a></div>
<div id="addFileHelp">
<button aria-labelledby="add-files" aria-expanded="false" type="button" class="snippet-button" id="expandHelpPara"><i class="fa fa-info-circle"><span>{{ help-button }}</span></i></button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not be updated as well?

Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Copy link
Collaborator

@microbit-carlos microbit-carlos left a comment

Choose a reason for hiding this comment

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

Great work here Mark!
All good and ready for merge :shipit:

@microbit-mark
Copy link
Contributor Author

@microbit-carlos microbit-carlos merged commit 0ce5e3f into bbcmicrobit:master Nov 6, 2019
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.

2 participants