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

Disable navigation block for text mode. #12185

Merged
merged 8 commits into from
Feb 5, 2019
Merged

Disable navigation block for text mode. #12185

merged 8 commits into from
Feb 5, 2019

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Nov 21, 2018

Description

When switching to code/ text mode navigation block should be disabled.
This adjustment is related to this issue: #12157

Fixes partially: #12157

How has this been tested?

It has been smoke-tested.

Screenshots

screen shot 2018-11-21 at 18 12 18

Types of changes

I've added a check which verifies editor mode.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @nicolad thank you for your contribution 👍 When I tried the keyboard shortcut ^⌥O it worked even if we are on the code editor. I think we also need to conditionally apply the shortcut e.g:

					{ hasBlocks && ! isTextModeEnabled && <KeyboardShortcuts
						bindGlobal
						shortcuts={ {
							[ rawShortcut.access( 'o' ) ]: onToggle,
						} }
					/> }

label={ __( 'Block Navigation' ) }
className="editor-block-navigation"
shortcut={ displayShortcut.access( 'o' ) }
aria-disabled={ ! hasBlocks || isTextModeEnabled }
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the disabled attribute instead of aria-disabled? I think it has the same effect for screen readers users and avoids the need for the onClick change done here as the onClick is not called if disabled is set to true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @jorgefilipecosta for tips.
Had adjusted details you've pointed.
And because this PR had been merged before:
https://github.com/WordPress/gutenberg/pull/12073/files
I did not touch any of those modifications regarding onClick. Only my changes had been discarded.

Copy link
Member

Choose a reason for hiding this comment

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

@jorgefilipecosta - in my testing there is a subtle difference between disabled and aria-disabled - in case of using disabled you never have to tab through the disabled element which seems to be reasonable but differs from other buttons like redo or undo. I have already landed this PR, but given that we need to do something similar for Document Outline I can open a follow-up and include that, too. What do you think?

@gziolo gziolo added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Code Editor Handling the code view of the editing experience labels Feb 5, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for fixing. We should land it much faster.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 5, 2019
@gziolo gziolo merged commit 307b237 into WordPress:master Feb 5, 2019
daniloercoli added a commit that referenced this pull request Feb 5, 2019
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Disable navigation block for text mode.

* Add additional rule for disabling navigation blocks.

* Disable click handler for required rules.

* Adjusted click handler for for text mode.

* Disabled keyboard shortcut in text mode.

* Deleted obsolete onClick check.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Disable navigation block for text mode.

* Add additional rule for disabling navigation blocks.

* Disable click handler for required rules.

* Adjusted click handler for for text mode.

* Disabled keyboard shortcut in text mode.

* Deleted obsolete onClick check.
@nicolad nicolad self-assigned this Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Code Editor Handling the code view of the editing experience [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants