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

Autocomplete: Assign ARIA attributes as combobox, with TinyMCE node mirroring #2789

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 25, 2017

Related: #2771

This pull request seeks to assign attributes of the Autocomplete cloned input as a combobox, bound as "owning" the Popover which it controls. To better represent the relation between the Editable and Popover search listing, props applied to the Editable have been improved using reference from the combobox role specification.

However, this highlighted an issue: Applying arbitrary props to the Editable contenteditable node is not currently possible. In a few cases, we have hard-coded support for a few whitelisted props (style, className, label). Instead, the changes here refactor Editable and TinyMCE to support arbitrary prop assignment, using a "mirror" node which is leverages React reconciliation in the parent Editable component. Since we disable reconciliation for the TinyMCE component, this mirror node serves to copy attributes when changed using MutationObserver. This is made more challenging by the fact that Editable accepts many props, most of which are handled by the root Editable component and should not be assigned to the contenteditable node. While I otherwise discourage propTypes assignments, the usage here helps simplify picking props to assign to the mirroring node.

These changes are extracted from commits originally present in #2771, in order to simplify review there to the minimal set of changes to fix the regression introduced to autocomplete behavior after #2323.

Testing instructions:

Verify that there are no regressions in:

  • Attribute assignment to TinyMCE (e.g. try applying background color to Paragraph block, which assigns itself as a combination of class and style attribute)
  • Placeholder behavior for Editable

Verify that correct ARIA expanded / ownership / active descendant attributes are assigned to both the mirror node (aria-hidden) and the contenteditable while using the default block slash inserter.

@aduth aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Sep 25, 2017
@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #2789 into master will increase coverage by 0.11%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2789      +/-   ##
==========================================
+ Coverage   33.95%   34.06%   +0.11%     
==========================================
  Files         191      191              
  Lines        5690     5703      +13     
  Branches      997      998       +1     
==========================================
+ Hits         1932     1943      +11     
- Misses       3180     3183       +3     
+ Partials      578      577       -1
Impacted Files Coverage Δ
blocks/editable/tinymce.js 0% <0%> (ø) ⬆️
components/autocomplete/index.js 89.47% <100%> (+0.28%) ⬆️
blocks/editable/index.js 13.43% <81.81%> (+2.92%) ⬆️

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 b7f61d2...081fa8b. Read the comment docs.

@aduth aduth force-pushed the update/autocomplete-combobox-role branch from 4ecc6a2 to 081fa8b Compare October 4, 2017 17:49
@BoardJames
Copy link

I'm not sure that I agree that putting aria attributes on the wrong element is a good idea even if you also put aria-hidden; for example quoting from https://www.w3.org/TR/wai-aria/states_and_properties#aria-owns :

Authors MUST ensure that an element's ID is not specified in more than one other element's aria-owns attribute at any time.

@aduth
Copy link
Member Author

aduth commented Oct 24, 2017

I'm not sure that I agree that putting aria attributes on the wrong element is a good idea even if you also put aria-hidden; for example quoting from https://www.w3.org/TR/wai-aria/states_and_properties#aria-owns :

Based on the conversation in #3106 I'd agree with your assessment. Maybe instead of a mirror node that the TinyMCE attaches an observer to, the parent Editable calls on the TinyMCE node directly with attributes from incoming prop changes. This is similar to your aria prop handling† in #3106, except for any prop not already handled by Editable (the editorProps variable in these changes). It's a bit ugly since we're effectively recreating React's native DOM reconciliation by hand. It would be nice if React exposed internal diff / patch utilities (example).

† I think ideally someone who renders an Editable could pass these props as the attributes they would expect to be applied, aria-label etc. instead of a custom grouping object, particularly if we're seeking to apply a broad range of attributes.

@aduth
Copy link
Member Author

aduth commented Nov 9, 2017

Superseded by #3106, #3343

@aduth aduth closed this Nov 9, 2017
@aduth aduth deleted the update/autocomplete-combobox-role branch November 9, 2017 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Feature] UI Components Impacts or related to the UI component system [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.

2 participants