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.

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

Docs: update keyboard shortcuts section of FAQ #9388

Merged
merged 1 commit into from
Oct 12, 2018
Merged

Docs: update keyboard shortcuts section of FAQ #9388

merged 1 commit into from
Oct 12, 2018

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Aug 28, 2018

Description

This PR updates the keyboard shortcuts section of the FAQ to show the Windows/Linux keyboard shortcuts in addition to the macOS shortcuts.

Detailed changes

  • The keyboard shortcuts are now displayed in tables using proper semantic HTML markup.
  • The Windows/Linux keyboard shortcuts are now listed, instead of just the macOS shortcuts.
  • Multiple-key shortcuts now use a wrapping <kbd> for the entire shortcut and nested <kbd>s for each individual key. This is to be be more semantically correct according to the HTML 5.2 specification.
  • Changed all instances of to ⌘Command for consistency. Modifier keys in macOS keyboard shortcuts now all include the corresponding symbols used in macOS.
  • Modifier keys in Windows/Linux keyboard shortcuts include the technically correct symbols according to ISO/IEC 9995-7 and ISO 7000, which means using "⎇" for Alt, "⌫" for Backspace, "⎈" for Ctrl, "⎋" for Esc, and "⇧" for Shift.
  • macOS keyboard shortcuts now show a "+" between ⌘Commands and the following key. The lack of spacing only makes sense when it is just icons being shown for all modifier keys, and before there was an inconsistent use of "+"s between some keys and not others due to the combined use of keys with just labels and keys with just icons.

@ZebulanStanphill ZebulanStanphill changed the title Docs: Update keyboard shortcuts section of FAQ Docs: update keyboard shortcuts section of FAQ Aug 28, 2018
@gziolo gziolo assigned talldan and unassigned talldan Aug 28, 2018
@gziolo gziolo requested a review from talldan August 28, 2018 07:20
@gziolo gziolo added the [Type] Developer Documentation Documentation for developers label Aug 28, 2018
@ZebulanStanphill
Copy link
Member Author

Squashed and rebased.

@talldan
Copy link
Contributor

talldan commented Oct 10, 2018

Hey @ZebulanStanphill - Thank you for working on this! Looks like there are conflicts again 😱

Would be good to see how this looks in the handbook before merging (@gziolo is there a way to do this?) I'm concerned that I couldn't see an example of a table in the gutenberg handbook, and I'm unclear on whether a markdown or html table would be preferable in terms of how it'll end up looking.

When viewing in github's markdown viewer, there are some things that could be tidied up:

  • Although the wrapping <kbd> elements are the right thing to do semantically, visually they don't look great on github. Looks like in the handbook there's no styling at all for them, which is unfortunate. I think that'd require a meta trac ticket if we wanted to improve them.
  • The icons on windows/linux are a bit confusing since you don't tend to see them on keyboards or in user interfaces. I think it'd be good to be consistent with how we show shortcuts in Gutenberg - so for windows / linux that'd be CtrlAltShift, on Mac ^ is good. On the Mac we also don't show a + between keys.
  • The tables should ideally all have the same column widths and overall table widths. Maybe moving everything into a single table would be tidier. The section titles could be a single th using colspan to span the columns.
  • I don't like the centered bold descriptions. It adds a lot of noise and is harder to read. I think it's fine to make them <td> elements so that they're left aligned and not bold.

@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

Would be good to see how this looks in the handbook before merging (@gziolo is there a way to do this?) I'm concerned that I couldn't see an example of a table in the gutenberg handbook, and I'm unclear on whether a markdown or html table would be preferable in terms of how it'll end up looking.

I don't think there is a way to do it because we don't have any tables exposed in the handbook. I would say, give it a go and we can always iterate.

I don't like the centered bold descriptions. It adds a lot of noise and is harder to read. I think it's fine to make them elements so that they're left aligned and not bold.

I second this.

@ZebulanStanphill
Copy link
Member Author

ZebulanStanphill commented Oct 10, 2018

@talldan @gziolo

Although the wrapping <kbd> elements are the right thing to do semantically, visually they don't look great on github. Looks like in the handbook there's no styling at all for them, which is unfortunate. I think that'd require a meta trac ticket if we wanted to improve them.

So I should remove the wrapping <kbd> elements, then?

The icons on windows/linux are a bit confusing since you don't tend to see them on keyboards or in user interfaces. I think it'd be good to be consistent with how we show shortcuts in Gutenberg - so for windows / linux that'd be CtrlAltShift, on Mac ⌘⌥^⇧ is good. On the Mac we also don't show a + between keys.

I thought that including the names of each key would be better for accessibility. Is that not the case?

The tables should ideally all have the same column widths and overall table widths. Maybe moving everything into a single table would be tidier. The section titles could be a single th using colspan to span the columns.

I will do this.

I don't like the centered bold descriptions. It adds a lot of noise and is harder to read. I think it's fine to make them elements so that they're left aligned and not bold.

But wouldn't this be worse for accessibility?

@talldan
Copy link
Contributor

talldan commented Oct 11, 2018

So I should remove the wrapping elements, then?

I think that's one of those things where we might have to break a rule to make it look better.

I thought that including the names of each key would be better for accessibility. Is that not the case?

There was some discussion about accessibility of the symbols here:
#9362

To summarise, voiceover reads the symbols correctly, adding the name causes them to be read twice, which is confusing. It is a shame the legibility of the shortcuts in github isn't great (the font size is so small!), which is a separate a11y issue, but at least we can make them look nice in the handbook at some point.

But wouldn't this be worse for accessibility?

Centered and bolded text is very hard to read for many, and I don't think screen readers will announce a th in a row so no difference there.

@ZebulanStanphill
Copy link
Member Author

Addressed feedback. I think this issue should get a "Needs Accessibility Feedback" label, just to be sure, though.

@talldan talldan added the Needs Accessibility Feedback Need input from accessibility label Oct 11, 2018
@talldan
Copy link
Contributor

talldan commented Oct 11, 2018

👍 Thanks for making those changes. I've added the label as requested.

One thing - I think we should still use Escape or Esc on MacOS, instead of the symbol, as the symbol is not widely known or used.

@ZebulanStanphill
Copy link
Member Author

I think we should still use Escape or Esc on MacOS, instead of the symbol, as the symbol is not widely known or used.

Done! 👍

</thead>
<tbody>
<tr>
<th colspan="3">Editor shortcuts</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is the best way to use a table header. Screen readers do announce the table headers (th) when navigating through the table cells. While the first 3 headers in the <thead> section give a proper information, I'm not sure what happens when then there's another header with colspan="3".

I'd suggest to separate sections with headings and use separate tables.

Testing with just Safari and VoiceOver isn't enough, this should be tested also on Windows (at least) with JAWS and NVDA.

Rather than abstract semantics, I'd suggest to keep things as simple as possible, ensuring information is announced correctly an navigable with ease. Also, redundant information and "noise" for screen readers should be avoided. For example. all the "plus" get announced and they're a bit annoying, not strictly relevant.

I'd also suggest to have a look at how webaim.org made similar tables, e.g.:
https://webaim.org/resources/shortcuts/jaws
https://webaim.org/resources/shortcuts/nvda

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also suggest to have a look at how webaim.org made similar tables, e.g.:
https://webaim.org/resources/shortcuts/jaws
https://webaim.org/resources/shortcuts/nvda

I find the tables in those links really hard to parse with the lack of consistent indentation, line breaks mid-way through shortcuts and highly variable column widths. I'm very surprised if that's the gold standard for accessibility.

Copy link
Member

Choose a reason for hiding this comment

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

screen shot 2018-10-12 at 09 44 17

I observed that keycap class shares styles with kbd. I bet you can simplify those tables and move on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd been keen to just get this merged now, separate tables or not. Will probably need iteration in a second PR or trac ticket once we know how it looks in the handbook.

Copy link
Member

@gziolo gziolo Oct 12, 2018

Choose a reason for hiding this comment

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

For example. all the "plus" get announced and they're a bit annoying, not strictly relevant.

Let's remove it as suggested.

I bet you can simplify those tables and move on.

I meant, that it's okay to have 3 tables with different width if that helps to make them more a11y friendly.

Copy link
Contributor

@talldan talldan Oct 12, 2018

Choose a reason for hiding this comment

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

Let's remove it as suggested.

They're that way in the Gutenberg front-end, though. If we remove them here, lets remove them in Gutenberg as well. I'd prefer it if we kept things consistent so that if we apply an improvement in one place we apply it in others as well. Maybe we can get this merged with the 'plus' symbols in place and then create an issue where we can discuss removing them across the board?

Copy link
Member

Choose a reason for hiding this comment

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

Good call @talldan, ideally we could copy and paste HTML from the modal dialog to the handbook to make the process easier.

@afercia
Copy link
Contributor

afercia commented Oct 12, 2018

I find the tables in those links really hard to parse with the lack of consistent indentation, line breaks mid-way through shortcuts and highly variable column widths. I'm very surprised if that's the gold standard for accessibility.

I'm an accessibility specialist, when I refer to examples I look at their markup, semantics, and the way content is perceived and announced by assistive technology. The presentation layer is secondary to me, and you can style it in the way you prefer 🙂

@talldan
Copy link
Contributor

talldan commented Oct 12, 2018

@ZebulanStanphill Sorry for the u-turn, it looks like the preference is for separate tables. I think that's the only action left. Once this is merged lets look at what can be done to make the the tables look a bit nicer 😄

@ZebulanStanphill
Copy link
Member Author

Separated the tables. 🙂

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks! Let's get this merged.

@talldan talldan added this to the 4.1 milestone Oct 12, 2018
@talldan talldan merged commit c22ec87 into WordPress:master Oct 12, 2018
@ZebulanStanphill ZebulanStanphill deleted the patch-1 branch October 13, 2018 04:38
@talldan talldan removed the Needs Accessibility Feedback Need input from accessibility label Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants