1621592 - Delete key reorders elements

Status: NEW

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

Description

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Repro: https://codepen.io/SaschaNaz/pen/ZEGrLOq

1. Put a <p> inside <span>, like <span>foo<p>bar</p></span>
2. Focus on the first line
3. Tap End key
4. Tap Delete key.

Expected: <p> should be merged into <span>.
Actual: <p> moves to the front of <span>.

This is essentially Bug 200416 but with Delete key.

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia), offline until 8th of October]

Reproducible on Ubuntu 18.04 too.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

atLeftBlockChild inside TryToJoinBlocksWithTransaction gets wrong value:

• The code tries merging the parent <div> of <span> with <p>, instead of just <span> with <p>
• The offset is computed against <div> and <span> using atLeftBlockChild, where <span> is in the parent of <p>.
• So the code moves <p> to the offset:
  • SplitInlinesAndCollectEditTargetNodesInOneHardLine splits <span> to make a new <span> that only includes <p>, and store it in arrayOfContents.
  • As HTMLEditor::NodeIsBlockStatic check fails, the code moves the new <span> without peeling any block.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Tried reusing the utility added by Bug 200416 but it's broken too. Backspace also splits <span> and even adds additional unexpected empty spans. Still investigating.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

Oh wait. Chrome and Safari also do not join <span> and <p>, instead they peels <p> and moves its children next to <span>.

Seems WPT has no relevant test for this.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

I think the right fix here is to merge <span> and <p>. The proper result should be <span>foobar</span> instead of current Chrome/Safari behavior resulting <span>foo</span>bar, because the latter gives more complex tree change and also potentially affect styling.

But some questions:

• Am I right that there is no spec defining this behavior, as contentEditable=true spec is archived and defines nothing?
• Could there be a good reason to keep Chrome/Safari behavior e.g. consistency with other existing behavior?
• If not, should we still keep compatibility with Chrome or can we fix this in my suggested way? I know this is just an edge case that shouldn't occur frequently, but would there still be a web compat problem from this?

Comment 6

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

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

I think the right fix here is to merge <span> and <p>. The proper result should be <span>foobar</span> instead of current Chrome/Safari behavior resulting <span>foo</span>bar, because the latter gives more complex tree change and also potentially affect styling.

I also think that Chrome/Safari's behavior does not make sense. But I wonder, what's your expected result for <span>foo[]<p>bar</p>baz</span> when Delete pressed at the caret position []. I think that it's reasonable to make them 2 lines, first line is "foobar" and second line is "baz" from point of view of users. But if <p> is merged to <span>, sounds like that "baz" also coming to the first line. Perhaps, needs to insert a <br> for the second line?

But some questions:

• Am I right that there is no spec defining this behavior, as contentEditable=true spec is archived and defines nothing?

IIRC, there has not been any WD spec defining each edit action behavior details.

• Could there be a good reason to keep Chrome/Safari behavior e.g. consistency with other existing behavior?
• If not, should we still keep compatibility with Chrome or can we fix this in my suggested way? I know this is just an edge case that shouldn't occur frequently, but would there still be a web compat problem from this?

For web-compat and if Chrome/Safari's behavior is reasonable, we should follow them. But like this case, if their behavior does not make sense, it's difficult to consider. But I guess that it's fine to take different behavior for this case because the situation must be rare in existing web apps.

Once we take hacky fix for emulating odd behavior of them, such hack must block more important fixes.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them) (OOO until 2024-10-06)]

I think that it's reasonable to make them 2 lines, first line is "foobar" and second line is "baz" from point of view of users.

True, I filed Bug 1624883 for that.

Once we take hacky fix for emulating odd behavior of them, such hack must block more important fixes.

Agreed. I'll try making this right.