Update Dropdown’s toggle/popover coordination to be UA-agnostic #30786
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +112 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
try/enhance-dropdown-2
branch
from
9f5feb8
to
4b29c65
Compare
To toggle properly it should return a single-child from renderToggle render prop. The prior use seems to be a hack around style concerns.
It was a hack to avoid toggle behavior bug in some UAs. Also, an unused classname is removed.
stokesman
force-pushed
the
try/enhance-dropdown-2
branch
from
1ab7f99
to
4ab48b9
Compare
stokesman
added
[Package] Components
stokesman
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
youknowriad
reviewed
Apr 21, 2021
| props.onMouseUp?.( event ); | ||
| }, | ||
| }; | ||
| toggleElement = cloneElement( toggleElement, pressHandlers ); |
There was a problem hiding this comment.
Clever solutions, Unfortunately, this is a breaking change in WordPress and something we can't ship just like that. Potentially we can try to detect whether we receive just a single element.
The problem though is even if it's just a single element, there's no guarantee that this element supports these props. So for me instead of doing this like that, we could potentially add these two handlers as new args to the render callback and have the consumer add them.
There was a problem hiding this comment.
there's no guarantee that this element supports these props
True, one of the adjustments made in this PR was a to ButtonBlockAppender which didn't work even though it already returned a single element.
we could potentially add these two handlers as new args to the render callback and have the consumer add them
I'm happy to polish up my draft PR #30397 which does just that.
Thank you for reviewing 🙇
There was a problem hiding this comment.
@youknowriad, I found a one-liner solution for this in #31170. Pinging you here for the context. It's much appreciated if you'll have a look.
|
Closing in favor of #30397 which still needs some work. |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Dropdown (and DropdownMenu) components have broken toggle behavior in some browsers (notably Safari and Firefox on macOS).
To fix it I've explored alternate approaches (a couple in #30397) and this PR is my favored solution as it requires no changes to the vast majority of
Dropdownimplementors because they happen to already meet the new requirement of returning a single child from therenderTogglecallback. Only four (of the more than twenty) in core needed updates, and being rather minor, those updates have been included in this PR. Mostly they constitute slight code quality improvements.How has this been tested?
Manually in Chrome, Safari and Firefox on macOS. In the Post Editor and Site Editor, checking various instances of
DropdownandDropdownMenuto verify they toggle as expected when pressing the toggle,Esckey, and when opening modals from within (more menu). Also, running the tests.Screenshots
Before: the broken toggling of various dropdowns in Safari on macOS
dropdowns-in-safari.mp4
Types of changes
Bug fix: #29746
Breaking change: Implementors that don't return a single child from the
renderTogglecallback would exhibit broken toggle (closing) behavior in all browsers but judging by the usage in Gutenberg most implementations should already be compatible.Checklist:
*.native.jsfiles for terms that need renaming or removal).