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

Block Toolbar: Move the block toolbar to the editor's header #2998

Merged
merged 5 commits into from Oct 27, 2017

Conversation

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 12, 2017

Description

refs #2983

This PR moves the block's toolbar to the editor's header.

screen shot 2017-10-12 at 09 16 10

Checklist

  • Move the toolbar.
  • Always show the toolbar when the block is selected.
  • Unify header buttons style
  • Find a better place for the permalink
  • Address the classic block's toolbar (needs design probably)
  • Mobile
  • Accessibility (part of this will be addressed by #2960
@youknowriad youknowriad self-assigned this Oct 12, 2017
@codecov
Copy link

@codecov codecov bot commented Oct 12, 2017

Codecov Report

Merging #2998 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2998      +/-   ##
==========================================
- Coverage   31.51%   31.49%   -0.02%     
==========================================
  Files         221      221              
  Lines        6305     6309       +4     
  Branches     1121     1122       +1     
==========================================
  Hits         1987     1987              
- Misses       3629     3632       +3     
- Partials      689      690       +1
Impacted Files Coverage Δ
editor/modes/visual-editor/block.js 0% <ø> (ø) ⬆️
editor/block-toolbar/index.js 0% <0%> (ø) ⬆️
editor/block-switcher/index.js 0% <0%> (ø) ⬆️
editor/header/index.js 0% <0%> (ø) ⬆️
components/navigable-menu/index.js 25.92% <0%> (-2.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24f6cb7...591e55e. Read the comment docs.

@youknowriad youknowriad changed the title Block Toolbar: Move the block toolbar to the editor's headeer Block Toolbar: Move the block toolbar to the editor's header Oct 12, 2017
@afercia
Copy link
Contributor

@afercia afercia commented Oct 12, 2017

Copying and pasting the accessibility items from #2960

1
Screen readers, especially the Windows ones (e.g. NVDA, JAWS) work in two different modes: "browse mode" and "forms mode". In "browse mode" they intercept key presses for their own goals and don't pass them to the browser. As a result, browsers are unaware of JS events on non-interactive elements (in our case the BlockToolbar). Only when screen readers switch to "forms mode", e.g. when the users lands on a form field, they pass key presses to the browser. They do the same on non-interactive elements only if they have some special ARIA roles. Only in this case JS events work on non-interactive elements.
In a few words: arrows navigation doesn't work with NVDA and JAWS (and seems it's buggy with Safari/VoiceOver too) because the browser doesn't know a keyboard event happened on the toolbar. To fix this, the toolbar needs an ARIA role="toolbar". Given the current markup, this would be a bit tricky, see below.

2
Toolbars need to have a "flat" structure, just a div with controls inside. Not a list.
I understand this is a bit tricky to address because the toolbars are actually composed by multiple toolbars, each one using a list. I've mentioned this issue a while ago in a conversation with @aduth . This should be addressed, and maybe #2148 would be the proper place.
Update: the toolbars won't be lists any longer if #3001 gets merged. However, there should be just one wrapper with role="toolbar" that contains all the controls. To my understanding, this would be easier with #3001 because the wrapper would contain just divs (and the controls). Will need testing with Windows screen readers.

3
To meet the ARIA spec and the ARIA Authoring Practices, navigation through the toolbar controls should happen with arrow keys only (no Tab). See https://www.w3.org/TR/wai-aria-practices/#toolbar

4
The toolbar should be labeled with an aria-label attribute. Worth noting the role="toolbar" already makes screen readers announce "tool bar" so the aria-label should say just something like "Formatting".

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 13, 2017

Thank you for working on this. 🏅

I think this is a great start. I think unifying the button style should be done separately, I have on my todo to look at focus styles again, given all that's changed and what we've learned. I have some ideas here, but I think this is probably fine for now.

The permalink should probably live in the Status menu of the sidebar, and if/when we merge the publish dropdown, there as well.

Can we keep the Classic blocks docked toolbar? It works pretty well there, and it is a rather unique block compared to all the others.

I would love to jump in and fix mobile in this one — I hope to add a breakpoint that makes the toolbar stacked below the editor bar — but perhaps it's best I jump in once some of afercia's notes are addressed?

@youknowriad youknowriad force-pushed the update/fixed-toolbar branch 3 times, most recently from 59b57ca to 0c121d4 Oct 13, 2017
@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 13, 2017

Rebase this to fix the accessibility issues:

  • The toolbar has arrow navigation
  • The toolbar has in/out shortcuts
  • The toolbar has the toolbar role
  • The toolbar is a div and no uls anymore
  • The toolbar has a label (probably not a great one but "Formatting" is not generic enough)

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 13, 2017

I will take a look at the responsiveness/mobile on monday. Thank you Riad!

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 16, 2017

I'm looking at some polish here but for now I'm seeing this JS warning when setting focus on a text block:

screen shot 2017-10-16 at 11 21 49

@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 16, 2017

@jasmussen yep, this should be fixed in master (just rebase)

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 16, 2017

I pushed a tweak so that the toolbar stacks on the mobile breakpoint.

But it's not perfect. Long term we should work to lower the breakpoint that causes these to stack, right now they stack already at 960.

Short term we should think about doing some polish to how the "mobile controls" work. Right now the "mobile controls" are basically the mover, and the now-in-dropdown Settings, Delete and HTML mode. These are now in 2 levels of drawers that all look weird.

I see two ways we can fix this:

  1. Drop the current implementation we have for handling the mover and trash on mobile, and instead move these to the ellipsis dropdown. Then we can put the ellipsis dropdown in the quick toolbar on mobile.

  2. Start thinking about #2898, and how that could potentially solve both the issue of the mover/trash/settings, as well as any item overflow in the quick toolbar.

screen shot 2017-10-16 at 12 02 13

screen shot 2017-10-16 at 12 02 19

screen shot 2017-10-16 at 12 02 28

screen shot 2017-10-16 at 12 02 33

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 16, 2017

There's also an issue with the fact that the toolbar disappears entirely when nothing is selected. This means the stacked toolbar overlaps the scrollbar. Perhaps worth thinking about showing a grayed text block toolbar when nothing is selected, or something in that vein.

@karmatosed
Copy link
Member

@karmatosed karmatosed commented Oct 17, 2017

A minor issue with Hello Dolly and smaller screen sizes:

2017-10-17 at 12 56

@youknowriad youknowriad force-pushed the update/fixed-toolbar branch 2 times, most recently from 2328f9e to f7cd821 Oct 24, 2017
@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 24, 2017

Rebased this and fixed some issues with the mobile toolbar. Do you think we could merge this soon in the weekend to discover any drawbacks before the next release?

@androb androb added this to the Beta 1.6 milestone Oct 25, 2017
@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 26, 2017

I think it would be good to get this in soon, yes. Nice fixes.

I wish the stacked toolbar didn't cover the scrollable area, but instead pushed it down:

screen shot 2017-10-26 at 09 34 45

However I feel like when I pushed fixes to this branch, I tried addressing that and running into issues. Perhaps that if you have nothing selected, the stacked toolbar disappears entirely:

screen shot 2017-10-26 at 09 38 20

Perhaps we should embrace the toolbar and show it there, even when empty:

screen shot 2017-10-26 at 09 39 13

This would allow us to change the scrollable areas top coordinate so it's never covered. I'm also confident that this toolbar area — when no block is selected — we'll soon find a use for. Whether through extensions or permalinks or something else.

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 26, 2017

I also got this JS error while messing around, not sure if it's this branch or also master:

screen shot 2017-10-26 at 09 36 56

Try these steps to reproduce:

  • Type in a paragraph
  • In a new paragraph paste a bunch of text

Or this one:

  • Type in a bunch of paragraphs
  • Change one of them to a quote
  • Delete the quote

@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 26, 2017

I pushed a fix for the JS error 5d16fa3

@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 26, 2017

The toolbar is always visible on mobile now.

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 26, 2017

Nice. Couple of issues though:

screen shot 2017-10-26 at 10 32 30

It covers the sidebar

We'll also want to push down this region so the scrollbar starts after the docked toolbar:

screen shot 2017-10-26 at 10 32 57

Thanks for working on this.

@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 26, 2017

Not easy to get right but I think it's ok now? bb70961

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 26, 2017

This seems great to me now. And I do agree, would be good to get this in soon so we can both test it, and polish it before the next version. CC: @mtias

@mtias
Copy link
Contributor

@mtias mtias commented Oct 26, 2017

@youknowriad is this rebased with master? I'm not seeing the keyboard improvements.

@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 26, 2017

@mtias not the last master, rebasing.

Edit: It is now

@youknowriad youknowriad force-pushed the update/fixed-toolbar branch from a0fdfc7 to 591e55e Oct 27, 2017
@youknowriad youknowriad merged commit 12d459a into master Oct 27, 2017
3 checks passed
@youknowriad youknowriad deleted the update/fixed-toolbar branch Oct 27, 2017
@youknowriad
Copy link
Contributor Author

@youknowriad youknowriad commented Oct 27, 2017

Merged to give us time to fix any missed bug before the release

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 27, 2017

🔥 🔥 🔥 🔥 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants