1822860 - Can't type space or enter when editing a contenteditable element inside a button/summary

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Albert Juhé]

Currently, it's not possible to type space characters inside an element with the contenteditable attribute when it's inside a button.

Steps to reproduce:

1. Load this HTML in Firefox: <button><div contenteditable>Helloworld!</div></button>.
2. Position the caret between Hello and world.
3. Type 'Space' in your keyboard.

Expected result: the space is added and the button text is now Hello world!. That's also how Google Chrome and GNOME Web behave.

Actual result: the space is not added and the button text is still Helloworld!.

Tested with Firefox 110.0 and 113.0a1 (2023-03-15) on Fedora Linux.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0671d3ac5672614bc516cf42968f605ff4a97ba1&tochange=d76885757ec1127bdeaceddca62b1739493c24f2

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1726454, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This is an existing (super-old) issue for the enter keypress. Masayuki it seems the editor is handling the event after the activation, do you know if that's expected?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I think that partially it's intentional but the others are not. HTMLEditor was designed when there was not contenteditable. Therefore, HTMLEditor always add events listeners to the document node for the bubbling phase. However, this has not been changed. Therefore, HTMLEditor handles all keyboard events after all elements in the document handled. We could change the target node of the event listeners. However, I guess that it'd cause some regressions in the edge cases. E.g., we don't re-set focus when contenteditable is changed in ancestor chain of focused element and/or we don't have concrete design of selection move with nested contenteditable elements (e.g., <div contenteditable>abc<div contenteditable="false"><div contenteditable>ghi</div></div></div>).

So I think that we cannot fix this only with a simple patch, and I don't think we should fix this immediately because editable features in <button> must have a lot of other problems.

Comment 6

[daniel.p.richards]

According to the serverity table:
"S3 - (Normal) Blocks non-critical functionality and a work around exists"

What's the workaround for this issue?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to daniel.p.richards from comment #6)

What's the workaround for this issue?

Currently, the workaround is not use contenteditable in the elements or prevent default of space keydown events and do document.execCommand("insertText", false, " ").

What's the scenario which you want to use editable elements in <button> and <summary>?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Ah, the event listener paths are not hot paths. We could use nsContentUtils::GetActiveEditor.

Comment 9

[daniel.p.richards]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

What's the scenario which you want to use editable elements in <button> and <summary>?

The WordPress block editor is primarily implemented using contenteditable, and this bug is potentially a blocking issue for introducing a details block for users .

Thank you for sharing the workaround!

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you for pointing the issue of WordPress. It's a major product, so we should give higher priority to this bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Trying: https://treeherder.mozilla.org/jobs?repo=try&revision=0eaf8718d3effc09339b12b999306e6aea1f2320

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1822860 - Make elements handle space key only when it's focused r=emilio!

They have a default action for a space key press. Chrome works it as-is when
the element has focus even if it's editable. Let's follow Chrome's behavior
for now because the bug blocks a feature of WordPress.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcef8c68028c
Make elements handle space key only when it's focused r=emilio

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39979 for changes under testing/web-platform/tests

Comment 15

[Serban Stanca [:SerbanS]]

Backed out for causing wpt failures in focus-tabindex-event.html.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-TIMEOUT | /html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-event.html | expected OK

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Hmm, I couldn't catch the failure in tryserver due to high-frequent failure of another test, sorry.

On the other hand, it looks like that the test is buggy.

• It assumes that blur never happens
• I have no idea what the author intended though, the test presses Alt key at starting the test, but it make Firefox UI steal focus
• The test fails even in Chrome, although a Chromium developer made it automated from a manual test

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1822860 - Make `focus-tabindex-event.html` never use `Alt` key which may be handled by chrome UI r=emilio!

Looks like that the test is buggy.

It was introduced as a manual test.
https://github.com/web-platform-tests/wpt/pull/8697/commits/a184705d166a0ffe28b2641b34cc7dc480167dfa

At this time, the steps to test it are

1. Focus the <button> with Tab key press(es)
2. Type Enter key

Then, this test is rewritten to run automatically.
https://github.com/web-platform-tests/wpt/pull/28043/commits/8ca894412821f0b18f96dc7a47809c18bd2cf672

At this time, oddly the test synthesizes Alt key press first. An Alt key
press makes chrome UI steals focus from content both in Firefox and Chrome.
Therefore, this test fails in Chrome and Edge.
https://wpt.fyi/results/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-event.html?run_id=5181334153068544&run_id=5143947435835392&run_id=5147113699147776&run_id=5118592029294592

I don't know why the Alt key press is added at the automating. I believe
that it's not necessary to check the behavior. Therefore, I remove the
Alt key press synthesizer and make the test check focus without focus
event listener (it's odd not to listen to blur event).

Depends on D177757

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9b1ac133205
Make elements handle space key only when it's focused r=emilio
https://hg.mozilla.org/integration/autoland/rev/a60b703136fb
Make `focus-tabindex-event.html` never use `Alt` key which may be handled by chrome UI r=emilio

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c9b1ac133205
https://hg.mozilla.org/mozilla-central/rev/a60b703136fb

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

This could cause new breakage in some web apps which move focus trickily. Therefore, we should not uplift this.