Bug 240834 - Confusing inconsistencies when programmatically applying edit commands to `display:none` elements
Summary: Confusing inconsistencies when programmatically applying edit commands to `di...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-23 15:38 PDT by Tim Horton
Modified: 2022-05-24 10:30 PDT (History)
6 users (show)

See Also:


Attachments
repro case (687 bytes, text/html)
2022-05-23 15:38 PDT, Tim Horton
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2022-05-23 15:38:37 PDT
Created attachment 459689 [details]
repro case

See attached test case. WebKit has an odd behavior where if you try to `insertText` into a contenteditable display:none element, it rejects the editing command, but if you try to do the same into a *child* of the contenteditable display:none element, it works.

*Also* interestingly, while the other browser engines behave internally consistently, they do not agree about the correct behavior:

Blink rejects the editing command in both cases.
Gecko allows the editing command in both cases.
Comment 1 Tim Horton 2022-05-23 15:40:31 PDT
I believe the root of the inconsistency in WebKit is the `display: none` check in computeEditabilityFromComputedStyle, which only checks the style of the immediate starting node, not whether or not you're in a `display: none` subtree.

However, I'm not sure which of the more-consistent behaviors to align us with :)
Comment 2 Tim Horton 2022-05-23 16:53:02 PDT
I *think* that Antti introduced this in https://trac.webkit.org/changeset/160966/webkit
Comment 3 Tim Horton 2022-05-23 17:14:16 PDT
https://bugs.webkit.org/show_bug.cgi?id=133371 suggests that display:none things should be editable.

Humorously, while https://trac.webkit.org/changeset/160966/webkit progressed bug 133371 in the case where you're querying isContentEditable on a child of the display:hidden element, it still says NO if you query *exactly* the display:hidden element.

Also, both Chrome and Firefox say isContentEditable=true for all elements (the direct display:none element and the children). I believe that the fact that Chrome doesn't allow editing in my original test case does not have to do with editability, but rather with selection behavior in display:none, so not necessarily related to this bug.

All of this together, plus the fact that changing this only affects a single test, makes me think that aligning on the Firefox behavior is the way to go. I'm going to prepare a patch to do so. Please shout if you disagree!
Comment 4 Ryosuke Niwa 2022-05-23 17:25:43 PDT
display:none shouldn't affect the result of isContentEditable but I don't think the rest of editing code can deal with the display:none content. If there aren't any editing tests that rely on this, then either approach sounds okay for now.
Comment 5 Tim Horton 2022-05-23 21:02:42 PDT
Pull request: https://github.com/WebKit/WebKit/pull/964
Comment 6 EWS 2022-05-24 10:29:42 PDT
Committed r294753 (250921@main): <https://commits.webkit.org/250921@main>

Reviewed commits have been landed. Closing PR #964 and removing active labels.
Comment 7 Radar WebKit Bug Importer 2022-05-24 10:30:16 PDT
<rdar://problem/93843189>