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

Inserter: Fix default block slash inserter #2771

Merged
merged 3 commits into from
Sep 25, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Sep 22, 2017

Regression introduced in #2323
Related: #2630

This pull request seeks to resolve an issue where the slash inserter introduced in #2630 stopped working after #2323. The issue is that because focus is now moved into a Popover when it transitions from a closed to open state, and because the Autocomplete field closes the popover when it loses focus, the popover is shown but immediately dismissed.

The changes here resolve this issue by adding a new focusOnOpen (default: true) prop to the Popover component to suppress the default focus behavior in cases such as Autocomplete where the focus behavior is not desirable, since it manages keyboard interactivity of the popover controls via the Editable input.

(Note: The following changes were extracted as a separate pull request to simplify review of the minimal set of changes necessary to resolve the regressed behavior. See #2789)

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.

Testing instructions:

Repeat testing instructions from #2630

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] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Status] In Progress Tracking issues with work in progress labels Sep 22, 2017
@aduth aduth force-pushed the fix/slash-inserter-popover-focus branch from 631f1b4 to ea7dec2 Compare September 25, 2017 12:40
@aduth aduth removed the [Status] In Progress Tracking issues with work in progress label Sep 25, 2017
@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #2771 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2771      +/-   ##
==========================================
+ Coverage   33.72%   33.77%   +0.04%     
==========================================
  Files         189      189              
  Lines        5636     5640       +4     
  Branches      980      982       +2     
==========================================
+ Hits         1901     1905       +4     
  Misses       3163     3163              
  Partials      572      572
Impacted Files Coverage Δ
components/tooltip/index.js 83.87% <ø> (ø) ⬆️
components/autocomplete/index.js 89.18% <100%> (+0.14%) ⬆️
components/popover/index.js 88.04% <100%> (+0.4%) ⬆️

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 75a5c16...6e686e2. Read the comment docs.

Since autocomplete manages keyboard behavior of autocomplete to control Popover content, redundant (and causes Popover to immediately close by blur of input)
@aduth aduth merged commit 68a93d4 into master Sep 25, 2017
@aduth aduth deleted the fix/slash-inserter-popover-focus branch September 25, 2017 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [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.

1 participant