Skip to content

Conversation

@andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Apr 12, 2021

Related to: #29243

Description

Fix the pattern inserter closing on iOS when switching pattern categories.

As raised in #29243 in iOS when you go to change pattern categories within the pattern inserter, selecting an option from the Select drop-down fires an onBlur event that bubbles up to the parent (in this case I believe it's the useDialog attached to the inserter panel).

This unexpected onBlur event bubbling causes the inserter to close when the user switches pattern categories. To prevent this from happening, in this change, we update the Pattern Inserter Panel to add an onBlur function that stops event propagation if the blur event does not have a related target. When a user clicks from the select element into the editor canvas (e.g. in a web browser) then there will typically be an event.relatedTarget. We can use the absence of this as a check to prevent the inserter from closing on iOS.

Note: we might be able to pursue a similar fix directly in the useDialog / use, but this didn't seem quite as safe, since we can't know for certain how the useDialog or useFocusOutside is being used in other contexts, whereas for the pattern category drop-down, if we are accidentally a bit greedy about stopping propagation, the side-effect is likely going to be negligible.

How has this been tested?

Manually, in a web browser, and in iOS Simulator via XCode.

In iOS simulator (or running this PR on a real iOS mobile device):

  • Open up the post editor, and go to insert a pattern.
  • Switch pattern categories and confirm that the inserter isn't closed automatically.
  • In web browser, with the pattern category selected, click on the editor canvas — the pattern inserter should close automatically since you've intentionally focused outside of the inserter.

Screenshots

iOS Simulator Web
change-pattern-categories-ios-small change-pattern-categories-web-small

Types of changes

Bug fix

Checklist:

  • My code is tested. (manually tested)
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers. (tested via keyboard, this change doesn't affect screen readers)
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@andrewserong andrewserong requested a review from ellatrix as a code owner April 12, 2021 07:02
@andrewserong
Copy link
Contributor Author

CC: @apeatling @glendaviesnz

@andrewserong andrewserong changed the title Pattern Categories Select: Stop event propagation for the select element's onBlur to fix behaviour is iOS Pattern Categories Select: Stop event propagation for the select element's onBlur to fix behaviour in iOS Apr 12, 2021
@apeatling apeatling self-requested a review April 12, 2021 20:11
@apeatling
Copy link
Contributor

apeatling commented Apr 12, 2021

✅ Tested and confirmed this fixes the problem. No knock on effects for selecting a pattern in the post editor.

@apeatling
Copy link
Contributor

@andrewserong There is one test failing related to the site editor that should be checked first.

@ramonjd
Copy link
Member

ramonjd commented Apr 13, 2021

This is the failing test.

FAIL specs/experiments/document-settings.test.js (12.612 s)
  ● Document Settings › when a template is selected from the navigation sidebar › and a template part is clicked in the template › should display the selected template part's name in the document header

I've tested these changes in the site editor as well (both iOS and web) and cannot see how they are related to this particular failing test.

Before
without-ios

After
with-ios

@andrewserong andrewserong force-pushed the fix/pattern-category-switching-ios-mobile branch from 80f1178 to c13a4a3 Compare April 13, 2021 01:31
@andrewserong
Copy link
Contributor Author

Thanks for testing @ramonjd! I've just given this a rebase to see if that gets it passing...

@andrewserong
Copy link
Contributor Author

That test is still failing after a rebase, but just double-checked and it's failing on other PRs, so I believe it's unrelated.

@andrewserong andrewserong force-pushed the fix/pattern-category-switching-ios-mobile branch from c13a4a3 to d07ae0f Compare April 16, 2021 05:50
@andrewserong
Copy link
Contributor Author

@apeatling I've rebased, and the tests are passing on this one now. Are you happy to merge this in (I don't have access yet), or do you think we should get another pair of 👀 from other folks?

@apeatling
Copy link
Contributor

Let's merge it because it's a bit of a nasty one and I think this is a good fix.

@apeatling apeatling merged commit fd2a138 into WordPress:trunk Apr 19, 2021
@github-actions github-actions bot added this to the Gutenberg 10.5 milestone Apr 19, 2021
@andrewserong andrewserong deleted the fix/pattern-category-switching-ios-mobile branch May 13, 2021 05:11
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.

3 participants