Skip to content

Conversation

@ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Apr 1, 2020

This PR attempts to implement the Composite component from Reakit into the new AlignmentMatrixControl.

For the initial integration attempt, it seemed to work as expected in Storybook (yay).

However, there appears to be issues within Gutenberg.

(Not) Playing Nicely with Toolbar

When expanding the Position dropdown in the Toolbar, the AlignmentMatrixControl appears to have issues rendering (see below):

Screen Capture on 2020-04-01 at 16-07-02

It flashes and closes immediately. I'm not too sure why.

Seeing unstable_virtual: false seems to allow it to render okay. Although, the interactions and keyboard navigation seems to be off.

CC'ing @diegohaz for expertise 😊

Thoughts/feedback welcome! We can change anything in this branch / draft PR

@ItsJonQ ItsJonQ requested a review from diegohaz April 1, 2020 20:10
@ItsJonQ ItsJonQ self-assigned this Apr 1, 2020
@github-actions
Copy link

github-actions bot commented Apr 1, 2020

Size Change: +361 B (0%)

Total Size: 886 kB

Filename Size Change
build/a11y/index.js 1.02 kB +1 B
build/annotations/index.js 3.45 kB -1 B
build/api-fetch/index.js 3.8 kB +1 B
build/autop/index.js 2.58 kB -2 B (0%)
build/block-directory/index.js 6.02 kB -5 B (0%)
build/block-editor/index.js 102 kB +107 B (0%)
build/block-library/index.js 111 kB -1 B
build/components/index.js 197 kB +297 B (0%)
build/core-data/index.js 10.7 kB +2 B (0%)
build/data-controls/index.js 1.04 kB +2 B (0%)
build/data/index.js 8.24 kB +5 B (0%)
build/date/index.js 5.36 kB -5 B (0%)
build/dom/index.js 3.05 kB -1 B
build/edit-post/index.js 92.2 kB -21 B (0%)
build/edit-site/index.js 9.1 kB -7 B (0%)
build/edit-widgets/index.js 4.43 kB +2 B (0%)
build/editor/index.js 42.8 kB -10 B (0%)
build/element/index.js 4.44 kB -1 B
build/format-library/index.js 6.95 kB -4 B (0%)
build/i18n/index.js 3.57 kB -1 B
build/is-shallow-equal/index.js 713 B +3 B (0%)
build/notices/index.js 1.57 kB -1 B
build/nux/index.js 3 kB -2 B (0%)
build/plugins/index.js 2.54 kB +2 B (0%)
build/redux-routine/index.js 2.84 kB -1 B
build/server-side-render/index.js 2.54 kB +1 B
build/shortcode/index.js 1.7 kB +1 B
build/viewport/index.js 1.61 kB +1 B
build/warning/index.js 1.14 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/style-rtl.css 7.64 kB 0 B
build/block-library/style.css 7.65 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/edit-navigation/index.js 2.48 kB 0 B
build/edit-navigation/style-rtl.css 239 B 0 B
build/edit-navigation/style.css 241 B 0 B
build/edit-post/style-rtl.css 12 kB 0 B
build/edit-post/style.css 12 kB 0 B
build/edit-site/style-rtl.css 4.62 kB 0 B
build/edit-site/style.css 4.62 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.74 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/rich-text/index.js 14.5 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@diegohaz
Copy link
Member

diegohaz commented Apr 2, 2020

I cleaned up the code a little bit and fixed some issues I found, including the one you mentioned about it closing right away.

Apr-01-2020 22-26-14

Styles need some work and probably we will want to close the popover when the user clicks on an option?

I'm not sure what was causing the problem, but I think it has something to do with this queueBlurCheck on the with-focus-outside high order component:

queueBlurCheck( event ) {
// React does not allow using an event reference asynchronously
// due to recycling behavior, except when explicitly persisted.
event.persist();
// Skip blur check if clicking button. See `normalizeButtonFocus`.
if ( this.preventBlurCheck ) {
return;
}
this.blurCheckTimeout = setTimeout( () => {
// If document is not focused then focus should remain
// inside the wrapped component and therefore we cancel
// this blur event thereby leaving focus in place.
// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus.
if ( ! document.hasFocus() ) {
event.preventDefault();
return;
}
if ( 'function' === typeof this.node.handleFocusOutside ) {
this.node.handleFocusOutside( event );
}
}, 0 );
}

When unstable_virtual is activated, items receive focus and return it to the composite container in the same tick so, visually, it looks like the container never lost focus. This is so we can provide the same API for both aria-activedescendant and roving tabindex methods. That is, you can do <CompositeItem onFocus={...} /> regardless of the approach you're using.

This sequence of blurring the container, focusing the item, blurring the item and focusing the container might be conflicting with the queueBlurCheck method or some other thing in that module.

One option is to stopPropagation on the container's onBlur event when event.relatedTarget is some of the composite items:

<Composite
	onBlur={ ( event ) => {
		if (
			composite.items.some(
				( item ) => item.ref.current === event.relatedTarget
			)
		) {
			event.stopPropagation();
		}
	} }
/>

But, it's safer to just use roving tabindex instead as this is a valid approach for grids.

@mtias mtias force-pushed the try/cover-alignment-control branch from 98692da to b51794c Compare April 2, 2020 10:52
@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 2, 2020

@diegohaz Wowow! Thank you so much! Your explanation makes sense. Beautiful refactors :D.
I can fix up the styles, no worries!

Could not have done this without you 🤘

@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 2, 2020

@diegohaz Haiii!! I made some refactors and pushed it up!

One thing I'm noticing (that I'm not sure how to resolve, yet). I believe it touches upon the focus/blur logic you pointed out.

This has to do with keyboard navigation when AlignmentMatrixControl renders within the Toolbar.


If we use <Dropdown /> (like what you suggested)...

Keyboard navigation (up, down, left, right), works as expected.

But... Toolbar navigation is funky. When the AlignmentMatrix icon is focused, pressing DOWN on the keyboard doesn't open the dropdown. Shift and ESC keypresses are also finicky.


If we use <Toolbar /> (like what I had previously)...

Keyboard navigation (up, down, left, right), does not work as expected. The movements are weird...

However, Toolbar navigation works.


🤔

@diegohaz
Copy link
Member

diegohaz commented Apr 2, 2020

Collapsed Toolbar/ToolbarGroup uses DropdownMenu, which uses NavigableMenu, which implements its own keyboard navigation expecting a vertical one-dimensional list. So I guess using it is not an option?

I don't know if there's a Dropdown component that reproduces the same behaviors as DropdownMenu without NavigableMenu. I just took the block switcher dropdown as a reference. It implements the arrow down key here:

const openOnArrowDown = ( event ) => {
if ( ! isOpen && event.keyCode === DOWN ) {
event.preventDefault();
event.stopPropagation();
onToggle();
}
};

@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 3, 2020

@diegohaz Thank you so much! That clue helped me a lot.

I was able to fix it up. I had to swap ToolbarButton with a regular Button to make it work.
Going through this exercise has helped me understand how... tricky... it is to work with all of these various parts (Toolbar, Dropdown, DropdownMenu, etc...).

Let's work on making these components seamless and better, together! 💪

@ItsJonQ ItsJonQ marked this pull request as ready for review April 3, 2020 14:05
@ItsJonQ ItsJonQ force-pushed the try/cover-alignment-control-composite branch from a4ce5d5 to 22286b0 Compare April 3, 2020 14:07
@ItsJonQ ItsJonQ merged commit 30b7ac4 into try/cover-alignment-control Apr 3, 2020
@ItsJonQ ItsJonQ deleted the try/cover-alignment-control-composite branch April 3, 2020 14:08
@diegohaz
Copy link
Member

diegohaz commented Apr 3, 2020

Yep! I’ll eventually need to update it to ToolbarButton later as part of the toolbar work I’m doing. 😅

@ItsJonQ
Copy link
Author

ItsJonQ commented Apr 3, 2020

@diegohaz You're the best <3

@gziolo
Copy link
Member

gziolo commented Apr 8, 2020

Nice team work 😍🔥

ItsJonQ pushed a commit that referenced this pull request Apr 8, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request Apr 14, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request Apr 15, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request Apr 28, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request May 11, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request May 13, 2020
* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>
ItsJonQ pushed a commit that referenced this pull request May 13, 2020
* Add initial AlignmentControl

* Refactor AlignmentControl utils. Add basic tests for utils

* Move AlignmentControl styled components to dedicated file

* Add AlignControl icon. Refactor and share styles between main Control and Icon

* Update Docs Manifest

* Export AlignControl as __experimentalAlignControl

* Fix export for AlignmentControl (__experimentalAlignmentControl)

* Update packages/components/src/alignment-control/utils.js

Co-Authored-By: Zebulan Stanphill <[email protected]>

* Integrate AlignmentControl with Cover block

Renamed AlignmentControl to AlignmentMatrixControl.

* Update Docs manifest and Storyshot

* Improve AlignmentMatrix rendering in Edit and Save functions

Remove unused code from base AlignmentMatrix component

* Prevent rendering of position className if undefined

* Update Cover content position control label

* Add padding to Cover (rather than inner content)

* Adding a11y and RTL support to AlignmentMatrixControl

Also refactors isRTL / useRTL from the rtl utils.

* Add basic tests for keyboard movements

* Cover: Customizing Alignment with Reakit Composite (#21333)

* WIP: Attempt to use Composite from Reakit

* Fix issues and clean up composite code

* Refactor Composite integration and update tests

* Update snapshots

* Fix keyboard navigation for AlignmentMatrixControl in Toolbar

* Fix keyboard interaction to open Alignment position on keyboard down

Co-authored-by: Haz <[email protected]>

* Update comments in AlignmentMatrixControl

* Remove unnecessary fragment in Story

* Refactor styles. Add knobs to AlignmentMatrixControl story

* Adjust icon style

* Add Tooltip to AlignmentMatrixControl cell

* Remove onBlur handling and border styles

* Account for center position

* Adding code comment for width/height props in AlignmentMatrixControlIcon

* Add prefix support in useInstanceId

* Add code comment regarding icon export from AlignmentMatrixControl

* Remove export of AlignmentMatrixControl icon element

* Update stories + snapshot

* Simplify nested array flattening

* Remove padding from Cover editor.scss

* Adjust BlockAlignmentMatrixToolbar popover className

* Move padding styles to top of wp-block-cover selector

* Fix AlignmentMatrixControl tests for window.focus in JSDOM

* Replace Array.flat with lodash.flattenDeep

Due to lack of polyfill support (at the moment).

Co-authored-by: Zebulan Stanphill <[email protected]>
Co-authored-by: Haz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants