1542521 - Can't backspace of in front of a line in div[contenteditable] when there is empty DIV before cursor

Status: RESOLVED FIXED

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

Description

[jisong]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.86 Safari/537.36

Steps to reproduce:

<div contenteditable>
<div>line 1</div>
<div></div>
<div>line 2</div>
</div>

Put cursor at beginning of second line (before "line 2"), press Backspace several times

Actual results:

merge second line with first line, becomes "line 1line 2"

Expected results:

nothing happens

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Delete key does merge the blocks but it takes two button press, one to only delete the empty <div> and another one to actually merge them.

https://searchfox.org/mozilla-central/source/editor/libeditor/HTMLEditSubActionHandler.cpp#3217

It seems Firefox currently decides not to join the blocks if it deletes anything not a text node between them. I think we need a way to detect non-visible non-text elements here. Masayuki, do we have a helper function for this?

For Backspace key, the fix suggested in Bug 779067 seems to work here but with the same problem I described above.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami :saschanaz from comment #1)

Delete key does merge the blocks but it takes two button press, one to only delete the empty <div> and another one to actually merge them.

https://searchfox.org/mozilla-central/source/editor/libeditor/HTMLEditSubActionHandler.cpp#3217

It seems Firefox currently decides not to join the blocks if it deletes anything not a text node between them. I think we need a way to detect non-visible non-text elements here. Masayuki, do we have a helper function for this?

I think that EditorBase::IsEmptyNode() is what it is. But it's not cheap if it runs too many times in a path if testing element node has a lot of descendants. You need to be careful to use it.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

I think that EditorBase::IsEmptyNode() is what it is.

I tried it and it kinda works, but I then found that it should be more sophisticated than that.

Consider there is a styled empty <div> where it has no visible child but got some height by CSS. This <div> is visible and thus should take a deletion step, while an unstyled empty <div> is invisible and thus should not.

Styled repro: https://codepen.io/SaschaNaz/pen/BaNxJgq
Unstyled repro: https://codepen.io/SaschaNaz/pen/bGdvVbg

You can see Chrome treats them in different way.

Is there a relevant helper function for this purpose?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Probably, there is no such utility method. One of the biggest difference between Gecko and Blink/WebKit is, Gecko does not refer style information for considering whether an element node is inline or block, but Blink/WebKit does. So, I can say same for checking empty element node.

I think that you shouldn't think about the cases of styled cases because referring style information from editor is pretty difficult. For example, if you want the information, but editor has already touched the DOM tree, there are pending reflow so that editor needs to request to flush pending reflow. However, if editor does it, anything can occur since script can run. For example, document may be unloaded, editor may be destroyed, focused element may be changed, etc. In other words, it's pretty risky from point of view of security.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

For example, if you want the information, but editor has already touched the DOM tree, there are pending reflow so that editor needs to request to flush pending reflow. However, if editor does it, anything can occur since script can run.

I guess those information can be read before touching DOM tree at least in this case, but I see there is some philosophical difference between Gecko and Blink/Webkit.

I think removing styled empty node still can be surprising, so I'd like to discuss more about this. Can't find an existing bug, should I file one or is there already a consensus not to read style information?

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1542521 - Join blocks with empty elements in between

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02f8bdc5339532c5bfd95819cd100e6668e5799d

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami :saschanaz from comment #5)

For example, if you want the information, but editor has already touched the DOM tree, there are pending reflow so that editor needs to request to flush pending reflow. However, if editor does it, anything can occur since script can run.

I guess those information can be read before touching DOM tree at least in this case, but I see there is some philosophical difference between Gecko and Blink/Webkit.

I guess that if edit action (I call each handling operation or command in editor) is nested by mutation event listeners, the DOM tree may have already touched unfortunately. Only the safe point is, immediately after editor flushes the pending reflow.

I think removing styled empty node still can be surprising,

Yeah, could be, but I guess that it's not occur so many case.

so I'd like to discuss more about this. Can't find an existing bug, should I file one or is there already a consensus not to read style information?

Yeah, filing any bug is fine. It's important to keep our work open. And there is no consensus for that as far as I know. But when only Gecko behaves differently, Gecko needs to change the behavior for web-compat unless the others' behavior does not make sense.

Do you investigate what style causes Blink/WebKit editor considering not empty?

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

I guess that if edit action (I call each handling operation or command in editor) is nested by mutation event listeners, the DOM tree may have already touched unfortunately.

Currently the code checks and deletes nodes one-by-one, maybe it could check their visibility first before removing any of them? I think that way mutation events won't fire before visibility check.

Yeah, could be, but I guess that it's not occur so many case.

True, it shouldn't be frequent.

Do you investigate what style causes Blink/WebKit editor considering not empty?

Didn't try everything but it seems any blocks with non-zero width/height/padding seems to be considered as not empty. margin is somehow ignored, and padding is also ignored when it's inline-block.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Kagami :saschanaz from comment #8)

Do you investigate what style causes Blink/WebKit editor considering not empty?

Didn't try everything but it seems any blocks with non-zero width/height/padding seems to be considered as not empty. margin is somehow ignored, and padding is also ignored when it's inline-block.

It seems that one of these functions are what Blink checks it.
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/editing/visible_units.cc;l=546-571;drc=38e9fb55b9545fc369b8731b52215022ccd73d9e?originalUrl=https:%2F%2Fcs.chromium.org%2F
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/editing/visible_units.cc;l=894-964;drc=38e9fb55b9545fc369b8731b52215022ccd73d9e?originalUrl=https:%2F%2Fcs.chromium.org%2F

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9526f24bab2b
Join blocks with empty elements in between r=masayuki

Comment 11

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

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

Comment 12

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9526f24bab2b

Comment 14

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

Upstream PR merged by moz-wptsync-bot

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

Remaining backspace bug will most probably be fixed in Bug 779067.