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

Document outline and block navigation in text mode #12157

Closed
gziolo opened this issue Nov 21, 2018 · 14 comments · Fixed by #14081
Closed

Document outline and block navigation in text mode #12157

gziolo opened this issue Nov 21, 2018 · 14 comments · Fixed by #14081
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). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Milestone

Comments

@gziolo
Copy link
Member

gziolo commented Nov 21, 2018

Clicking on links in the document outline and block navigation can't really work in text mode. Should we disable those features or convert links to plain text?

screen shot 2018-11-21 at 09 28 20
screen shot 2018-11-21 at 09 28 29

/cc @jasmussen

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Code Editor Handling the code view of the editing experience Needs Design Feedback Needs general design feedback. labels Nov 21, 2018
@jasmussen
Copy link
Contributor

Can we just disable those button in the text editor?

@gziolo
Copy link
Member Author

gziolo commented Nov 21, 2018

Sure, @tofumatt or @afercia should they be marked as disabled or can be removed altogether when Code editor is active?

@gziolo gziolo added Needs Accessibility Feedback Need input from accessibility and removed Needs Design Feedback Needs general design feedback. labels Nov 21, 2018
@gziolo gziolo added this to the WordPress 5.0.x Follow Ups milestone Nov 21, 2018
@gziolo gziolo added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Nov 21, 2018
@afercia
Copy link
Contributor

afercia commented Nov 21, 2018

I'd vote for disabling them. Please consider there's some inconsistency in the way these controls get disabled, I've tried to improve consistency in #12073

@nicolad
Copy link
Member

nicolad commented Nov 21, 2018

Had disabled button, please confirm that it should not be deleted.
#12185

@jasmussen
Copy link
Contributor

Let's go with disabling them for now, then we can revisit if it becomes pressing. 👍 👍

@afercia
Copy link
Contributor

afercia commented Nov 22, 2018

I'd suggest to address this together with #12073, which takes a different approach to "disable" those buttons.

@afercia
Copy link
Contributor

afercia commented Nov 23, 2018

Discussed during today's accessibility bug scrub, and agreed to recommend the approach taken in #12073 to "disable" those buttons. Removing the Needs Accessibility Feedback label, as feedback has been given.

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed Needs Accessibility Feedback Need input from accessibility labels Nov 23, 2018
@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Dec 3, 2018
@gziolo
Copy link
Member Author

gziolo commented Feb 5, 2019

Block navigation was fixed in #12185.

@afercia
Copy link
Contributor

afercia commented Feb 24, 2019

@gziolo what's left here? Can this be closed?

@gziolo
Copy link
Member Author

gziolo commented Feb 25, 2019

Document navigation still needs to be adjusted.

There is also one thing that needs to be standardized:

in my testing (for block navigation) 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.

@afercia
Copy link
Contributor

afercia commented Feb 25, 2019

Hm will have a look.

aria-disabled doesn't do anything in terms of interaction and focus. It just communicates to assistive technologies a certain element is disabled. To avoid focus losses, some buttons just noop and use aria-disabled so they're still focusable but they don't do anything.

Focusability of disabled controls
https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_disabled_controls

@afercia
Copy link
Contributor

afercia commented Feb 25, 2019

Yep, the change introduced in #12185 is inconsistent with the behavior of the other buttons. It should use aria-disabled and noop, instead of the HTML attribute disabled. Will try a PR, looking also at Document navigation.

@afercia
Copy link
Contributor

afercia commented Feb 25, 2019

I see there's also a warning:

Warning: React does not recognize the `isSelected` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `isselected` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in button (created by ForwardRef(Button))
    in div (created by BlockNavigationList)
    in li (created by BlockNavigationList)
    ...

@afercia
Copy link
Contributor

afercia commented Feb 25, 2019

Turns out the isSelected prop is there because these where previously MenuItem components. Now, they're buttons and the prop should be removed.

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). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants