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

Rich Text: Avoid activeElement focus call #20594

Merged
merged 3 commits into from
Mar 2, 2020
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 2, 2020

Fixes: https://core.trac.wordpress.org/ticket/49519

This pull request attempts to fix Trac#49519, where an error occurs when deleting blocks in the editor in Internet Explorer.

The error manifests itself as:

Unable to get property 'focus' of undefined or null reference

It was traced back to this line of code in the applySelection function:

activeElement.focus();

This was recently changed/introduced as part of #19536.

It's not immediately clear to me whether this explicit focus is required. It may be that the manipulation of selections on the lines preceeding the focus() call would cause focus to be lost, so an explicit focus is intended in restoring the focus which had existed before those selection manipulations occurred.

Ultimately, it seems that activeElement is unset in Internet Explorer, but not other browsers.

If removing the focus is not acceptable, the following alternatives may be considered:

  • Perform a truthy test of activeElement, to at least verify existence before calling focus
    • Why would this be any different or allowable in Internet Explorer? Is it a specific difference in browser implementation of activeElement? Specifically, I have found that activeElement will tend to fall back to document.body in many other browsers (see related example).
  • Focus an element relative to the new selection, e.g. range.commonAncestorContainer

Testing Instructions:

In Internet Explorer and your preferred browser, verify no errors when deleting block:

  1. Navigate to Posts > Add New
  2. Insert a paragraph block with some text
  3. Press Enter to create a second paragraph
  4. Delete the second paragraph by pressing Backspace

@aduth aduth added [Type] Bug An existing feature does not function as intended Browser Issues Issues or PRs that are related to browser specific problems Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 2, 2020
@aduth aduth requested a review from ellatrix March 2, 2020 18:39
@github-actions
Copy link

github-actions bot commented Mar 2, 2020

Size Change: +32 B (0%)

Total Size: 865 kB

Filename Size Change
build/rich-text/index.js 14.3 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.5 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/editor-rtl.css 7.4 kB 0 B
build/block-library/editor.css 7.4 kB 0 B
build/block-library/index.js 116 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.5 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.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.9 kB 0 B
build/edit-post/style-rtl.css 8.53 kB 0 B
build/edit-post/style.css 8.53 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 4.01 kB 0 B
build/editor/style.css 4 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 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.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 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.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member Author

aduth commented Mar 2, 2020

  • Why would this be any different or allowable in Internet Explorer? Is it a specific difference in browser implementation of activeElement? Specifically, I have found that activeElement will tend to fall back to document.body in many other browsers (see related example).

Related (reinforcing):

When there is no selection, the active element is the page's <body> or null.

https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/activeElement

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the problem in my tests 👍 Block removal was possible on IE and other browsers. I verified the formatting library still works as before.

@aduth
Copy link
Member Author

aduth commented Mar 2, 2020

An additional finding from some last-minute testing: This resolves the error by avoiding calling on a property which may be null when there is no focused element. However, there is still a regression in WordPress 5.4 in how selections are applied in Internet Explorer. Specifically, in Internet Explorer, when pressing Backspace from an empty second paragraph, the selection is placed at the beginning of the first paragraph, instead of the end. It works correctly in other browsers. It would be good to investigate a fix here, though at least the error is resolved as of these changes. I expect the issue may be a result of the changes in #19536, or perhaps in how we rely on a focus here that cannot occur in Internet Explorer as a result of how it assigns activeElement. I'll plan to open a follow-up issue.

@aduth aduth merged commit 59da7c7 into master Mar 2, 2020
@aduth aduth deleted the fix/trac-49519-ie-delete-block branch March 2, 2020 21:04
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 2, 2020
@aduth
Copy link
Member Author

aduth commented Mar 2, 2020

I'll plan to open a follow-up issue.

See: #20598

jorgefilipecosta pushed a commit that referenced this pull request Mar 2, 2020
* Rich Text: Avoid activeElement focus call

* Rich Text: Restore focus, accounting for null activeElement

* Rich Text: Verify activeElement instanceof HTMLElement
jorgefilipecosta pushed a commit that referenced this pull request Mar 2, 2020
* Rich Text: Avoid activeElement focus call

* Rich Text: Restore focus, accounting for null activeElement

* Rich Text: Verify activeElement instanceof HTMLElement
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 3, 2020
activeElement.focus();
}
} else if ( document.activeElement instanceof window.HTMLElement ) {
document.activeElement.blur();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth Why was this added? This seems to be causing the IE bug #20598.

Copy link
Member Author

@aduth aduth Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellatrix If I recall correctly, the idea was that if this logic should not cause a change in focus, and there was no focused element before the selection applied, we should guarantee that there is no focused element after it's applied by calling blur on whichever element is focused at that point.

I'm not able to remember if there was a specific case in mind this was addressing, or if it was only here to deal with the hypothetical technical possibility.

Based on the debugging information from the original pull request comment, I suspect the main fix for Trac#49519 is largely in adding the condition of if ( activeElement ) { (accounting for the fact that activeElement can be null). If that's enough to accommodate all of the current scenarios and fix both Trac#49519 and #20598, then maybe it would be fine to remove this blur call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants