Add tests for contenteditable=false elements inside contenteditable=true elements and for selection in empty elements #35862

Comandeer · 2022-09-10T20:13:12Z

I've been asked by @johanneswilm to prepare some tests for selection issues in CKEditor as a part of Interop 2022.

There are two main issues for us:

selection of the [contenteditable=false] elements inside the [contenteditable=true] elements (especially at their beginning and end),
placing selection inside empty elements.

Tests

For the first issue I've prepared 4 tests:

automatic:
- selection/contenteditable/cefalse-on-boundaries.html,
manual:
- editing/cefalse-beginning-deletion-manual.html,
- editing/cefalse-boundaries-deletion-manual.html,
- editing/cefalse-end-deletion-manual.html.

Unfortunately, manual tests were needed because Chrome shows the correct selection and it seems to work fine with the JS API (e.g. range#deleteContents()) but trying to interact in person with such selection results in a broken experience (the selected content can't be deleted).

For the issue with putting selection inside empty elements, I've prepared two tests:

automatic:
- selection/caret/empty-elements.html,
manual:
- editing/empty-elements-manual.html.

Once again, a manual test was needed as Firefox allows putting the selection inside such an element but typing inside it does not work (the typed content is moved outside the empty element).

Context

The first issue required us to create dedicated plugins for CKEditor 4 and CKEditor 5 whose task is to add editable elements around non-editable ones (especially ones at the beginning or end of the editor) to fix the incorrect selection of them. However, in CKE4, even with this plugin, the issue is still present, especially in Safari.

The second one requires us to use various fillers (CKEditor 4 version and CKEditor 5 one) to be able to put the selection in such places. There is also a similar issue connected with editing at the edge of elements, e.g. links. As visible in CKEditor's 5 link demo, putting the caret inside any of the links, moving it to the end of the link, and pressing space will produce an   filler, needed to inform the browser that the intention is to stay inside the link.

…ies of cE=true elements.

jgraham · 2022-09-12T10:34:12Z

@masayuki-nakano could you look at these tests? https://wpt.fyi/results/selection?label=pr_head&max-count=1&pr=35862 suggests that the cases covered are where Blink and Gecko agree but WebKit is different.

masayuki-nakano · 2022-09-14T04:53:33Z

editing/empty-elements-manual.html

+<ol>
+  <li>Put the caret inside the red square above.</li>
+  <li>Press any letter to insert it.</li>
+</ol>
+<script>
+  setup( { explicit_timeout: true } );
+
+  promise_test( () => {
+    return new Promise( ( resolve, reject ) => {
+      host.addEventListener( 'input', () => {
+        try {
+          assert_greater_than( strong.innerHTML.length, 0, 'The text was inserted into the <strong> element' );
+        } catch ( error ) {
+          reject( error );
+        }
+        resolve();
+      } );
+    } );
+  } );


Why cannot write this as:

const actions = new test_driver.Actions(); await actions .pointerMove(0, 0, {origin: document.querySelector("strong")}) .pointerDown({button: actions.ButtonType.LEFT}) .pointerUp({button: actions.ButtonType.LEFT}) .send(); document.execCommand("insertText", false, "a");

? It seems that Chrome and Firefox work as same as I do same thing in them.

And should this be work with completely empty inline element (i.e., without margin, padding, border, etc)? Then, it should be tested too.

Thanks for the advice to use the test_driver! I didn't discover it while writing tests and that's why I decided to make them manual. I've rewritten them using test_driver.

And should this be work with completely empty inline element (i.e., without margin, padding, border, etc)? Then, it should be tested too.

I've added one analogous to the test with a styled element. However, I'm not sure if it's the right one – as it emulates the user clicking in the element, yet the element has zero height.

masayuki-nakano · 2022-09-14T05:04:56Z

editing/cefalse-beginning-deletion-manual.html

+    const cefalse = document.querySelector( '#cefalse' );
+    const paragraph = document.querySelector( '#paragraph' );
+    return new Promise( ( resolve, reject ) => {
+        host.addEventListener( 'keyup', ( { key }) => {
+          if ( key !== 'Backspace' && key !== 'Delete' ) {
+            return;
+          }
+
+          try {
+              assert_false( cefalse.isConnected, 'cE=false element was removed' );
+              assert_false( paragraph.isConnected, 'paragraph was removed' );
+          } catch ( error ) {
+              reject( error );
+          }
+          resolve();
+        } );
+    } );


It seems that the following code gets same result as I test this manually:

const cefalse = document.querySelector( '#cefalse' ); const paragraph = document.querySelector( '#paragraph' ); getSelection().collapse(paragraph, 0); document.execCommand("selectAll"); document.execCommand("delete"); assert_false( cefalse.isConnected, 'cE=false element was removed' ); assert_false( paragraph.isConnected, 'paragraph was removed' );

And the other manual tests can be rewritten with this style.

masayuki-nakano · 2022-09-14T05:15:25Z

Currently, Gecko allows to delete non-editable nodes which are children of editable nodes.
https://searchfox.org/mozilla-central/rev/7b36c8b83337c4b4cdfd4ccc2168f3491a86811b/editor/libeditor/HTMLEditUtils.h#103-119

And creating new text node when selection is collapsed in an empty element is reasonable to me. However, I'm not sure about the cases when selection range boundary is in an empty inline element, e.g.,

abc<strong>[</strong>d]ef

In this case, abc[new textef is expected? Or abcnew textef?

Similarly,

ab[c<strong>]</strong>def

In this case, abnew textdef? abnewtextdef or abnew textdef?

I think that in the Chrome's rules, the first expectations are reasonable because Chrome keeps the style of start boundary of the range as far as I've investigated.

Comandeer · 2022-09-17T11:51:32Z

I think that in the Chrome's rules, the first expectations are reasonable because Chrome keeps the style of start boundary of the range as far as I've investigated.

I tend to agree with Chrome on that – keeping the style of start boundary is something we do in CKEditor.

I've also added an additional test for inserting text on the boundaries of a link – browsers jump out of the link, even if the range is still inside it. It's inconsistent with other elements, e.g. strong where the same selection stays inside the element upon inserting anything.

@masayuki-nakano, thanks for the review! I've updated the PR according to your advice. Thanks to that I was able to move all tests into two files: editing/empty-elements-insertion.html and editing/cefalse-boundaries-deletion.html. Could you recheck if everything's ok?

masayuki-nakano · 2022-11-08T02:09:01Z

I apologize the long delay.

I think that in the Chrome's rules, the first expectations are reasonable because Chrome keeps the style of start boundary of the range as far as I've investigated.

I tend to agree with Chrome on that – keeping the style of start boundary is something we do in CKEditor.

Well, but it's something a little bit complicated if first visible content is not text, e.g., <img>.

I've also added an additional test for inserting text on the boundaries of a link – browsers jump out of the link, even if the range is still inside it.

I don't think it's reasonable because it makes it impossible that web apps cannot make users keep tying in a link with managing Selection. Therefore, I think that if Selection is updated by Selection API (including Range API), the position should be used as-is. Then, it provides better ability to web apps.

FYI: Gecko does something special in this case, but for compatibility with the other browsers and traditional behavior which is important for Thunderbird. Gecko respects Selection when caret is moved from inside a link to the link edge by right or left arrow key or user clicks outer half of the character at edges of a link. (I don't think that this native behavior of browsers should be aligned.)

It's inconsistent with other elements, e.g. strong where the same selection stays inside the element upon inserting anything.

Yeah, I don't like the inconsistent behavior because browsers need to handle it specially in some places in builtin editor implementation. However, I think that the inconsistency comes from traditional behaviors of some word processors especially "Word". Therefore, I think that the special handling may be expected by the users.

@masayuki-nakano, thanks for the review! I've updated the PR according to your advice. Thanks to that I was able to move all tests into two files: editing/empty-elements-insertion.html and editing/cefalse-boundaries-deletion.html. Could you recheck if everything's ok?

Really sorry for the long delay. I'll check it soon.

masayuki-nakano · 2022-11-08T02:14:47Z

editing/cefalse-boundaries-deletion.html

+      document.execCommand( 'selectAll' );
+      document.execCommand( 'delete' );
+
+      assert_false( cefalse.isConnected, 'cE=false element was removed' );


nit: Although I don't know whether there are some discussion about WPT test messages. For making the message clearer, I recommend to use "should" or "shouldn't" in the messages. For example, "cE=false element should be removed".

If this test detects a regression, it'll see "cE=false element was removed", but it's not actually. Therefore, this message anyway makes browser developers confused.

Sounds reasonable, I've updated the messages.

masayuki-nakano · 2022-11-08T02:21:16Z

editing/cefalse-boundaries-deletion.html

+<div contenteditable>
+  <div contenteditable="false" id="cefalse-beginning">&nbsp;</div>
+  <p id="paragraph-beginning">Lorem ipsum dolor sit amet.</p>
+</div>


In this test, you define the DOM tree first, then, check it below where it's far from here.

If I were you, I'd put <div contenteditable> only and initialize its content with setting .innerHTML. Then, only reading test() or promise_test() allows developers to understand what it checks easier. Like this.

Thanks, I didn't notice these utils. Tests rewritten with them are more readable now.

masayuki-nakano · 2022-11-08T02:24:12Z

editing/cefalse-boundaries-deletion.html

+    test( () => {
+      const cefalse = document.querySelector( '#cefalse-beginning' );
+      const paragraph = document.querySelector( '#paragraph-beginning' );
+      getSelection().collapse( paragraph, 0 );


I think that just calling focus of div[contenteditable] is faster and enough for the tests.

masayuki-nakano · 2022-11-08T02:34:09Z

editing/empty-elements-insertion.html

+      .send();
+    document.execCommand( 'insertText', false, 'a' );
+    assert_greater_than( target.innerHTML.length, 0, 'The text was inserted into the styled <strong> element' );
+  }, 'Insert text into the inline element styled with border and padding' );


I think that you should separate test for  element which only has border or padding.

On the other hand, you probably intended to check whether it should or should not work with visible vs. implicitly invisible elements. If so, there could be more tricky cases like ::before etc?

And I wonder, this is not directly related to editing. If this test requests some browsers to change the behavior, it seems that they need to change the Selection result when it's clicked. Perhaps, you should add such test to the directory of Selection API.

I've separated tests for border and padding.

I didn't move these tests to the selection API dir, though. As can be seen in a simple demo, the selection seems to be correct (at least in the case of the visible span). In Safari and Chrome this case works, Firefox jumps out of the element when anything is typed inside.

If so, there could be more tricky cases like ::before etc?

Probably but I don't think such use cases are really sensible in the editing context. The ::before pseudoelement is not selectable and I can't think of any real use case for changing that behavior.

I didn't move these tests to the selection API dir, though. As can be seen in a simple demo, the selection seems to be correct (at least in the case of the visible span). In Safari and Chrome this case works, Firefox jumps out of the element when anything is typed inside.

Thank you for explaining the symptom. Indeed, it should work in Firefox too. I'll take a look after current big work ends.

If so, there could be more tricky cases like ::before etc?

Probably but I don't think such use cases are really sensible in the editing context. The ::before pseudoelement is not selectable and I can't think of any real use case for changing that behavior.

If Chrome nor Safari does not treat the elements having only CSS generated content, it makes the fix harder in Firefox because Firefox needs to check whether ::before and/or ::after is the only content in it. Therefore, anyway the behavior is important for me to making it fix in Firefox. Therefore, could you add the cases too?

Sorry for the delayed response.

I think that I initially misunderstood the case with ::before and ::after pseudoelements. For some reason, I thought about putting selection in the pseudoelements. But I suppose you meant being able to put the selection in the empty elements with only the pseudoelements – is my understanding correct? If the latter case is the one you thought of, then I agree that it would make sense to support inserting text also in such empty inline elements.

I added some tests for the second case.

Sorry for the delayed response.

Me too, no worry.

But I suppose you meant being able to put the selection in the empty elements with only the pseudoelements – is my understanding correct?

Yes, that's right. My intention was adding the case which may create a "frame" for empty element only with CSS styling.

I added some tests for the second case.

Thank you!

masayuki-nakano · 2022-11-08T02:38:47Z

editing/link-boundaries-insertion.html

+    selection.collapseToEnd();
+
+    document.execCommand( 'insertText', false, 'a' );
+    assert_equals( target.innerHTML, 'Linka', 'The text was inserted into the link' );


Oh, which do browsers pass or fail these tests?

Firefox passes both tests, Safari and Chrome fail both.

Thanks. Especially Chrome "normalizes" Selection before handling edit operations/commands. So the result is what I can imagine. (When I make Firefox emulate the behavior, odd behavior caused by the normalization make the code complicated...)

masayuki-nakano · 2022-11-08T02:43:35Z

FYI: I'm not a formal reviewer of WPT. Therefore, I'm not sure about the details of the rules around the test message, formatting the code (especially around the white-spaces around ()).

And perhaps, these tests should be in editing/run or editing/other because there is only one test under editing directly. I'm not sure how to group run and other, but run contains tests of various execCommand. Therefore, I usually add tests under editing/other when I add new tests at fixing a bug of Gecko.

wpt-pr-bot · 2022-11-11T16:15:24Z

There are no reviewers for this pull request. Please reach out on the chat room to get help with this. Thank you!

Comandeer · 2022-11-11T16:30:35Z

However, I think that the inconsistency comes from traditional behaviors of some word processors especially "Word". Therefore, I think that the special handling may be expected by the users.

Yes, that sounds like a plausible explanation for the current link behavior.

@masayuki-nakano, thanks for the review. I've further updated the tests, following your advice.

jgraham · 2022-11-22T11:33:31Z

FYI: I'm not a formal reviewer of WPT. Therefore, I'm not sure about the details of the rules around the test message, formatting the code (especially around the white-spaces around ()).

There aren't strongly enforced rules for js formatting because we don't have a toolchain to enforce them, and we have contributions coming from multiple projects, and don't want things to be blocked on formatting choices.

So generally it's up to your discretion, but of course code that looks like the surrounding code is better than code that's introduces new conventions.

masayuki-nakano

Thank you very much!

…rue elements and for selection in empty elements (web-platform-tests#35862) * Add tests for selecting and deleting cE=false elements on the boundaries of cE=true elements. * Add tests for empty inline element.

Comandeer added 2 commits September 10, 2022 21:34

Add tests for selecting and deleting cE=false elements on the boundar…

1db617f

…ies of cE=true elements.

Add tests for selection inside empty elements.

080acbd

wpt-pr-bot added editing selection labels Sep 10, 2022

wpt-pr-bot assigned rniwa Sep 10, 2022

wpt-pr-bot requested review from johanneswilm and rniwa September 10, 2022 20:13

johanneswilm mentioned this pull request Sep 12, 2022

Selection web-platform-tests/interop-2022-editing#2

Open

masayuki-nakano reviewed Sep 14, 2022

View reviewed changes

Comandeer added 2 commits September 17, 2022 13:28

Replace manual test with automatic ones.

c64df6c

Add test for inserting content into the boundaries of a link.

a27a273

Comandeer requested review from masayuki-nakano and removed request for rniwa and johanneswilm September 17, 2022 11:51

jgraham mentioned this pull request Oct 20, 2022

Agenda 2022-10-20 web-platform-tests/interop-2022-editing#11

Closed

masayuki-nakano reviewed Nov 8, 2022

View reviewed changes

Comandeer added 2 commits November 11, 2022 16:25

Rephrase assertions.

1d1312a

Rewrite tests using EditorTestUtils.

0291190

wpt-pr-bot added the status:needs-reviewers label Nov 11, 2022

Move tests to the editing/other directory.

d96a3cf

jgraham mentioned this pull request Nov 17, 2022

Agenda 2022-11-17 web-platform-tests/interop-2022-editing#12

Closed

Add tests for empty inline element with pseudoelements.

1344708

masayuki-nakano approved these changes Dec 5, 2022

View reviewed changes

masayuki-nakano merged commit 3b6b8de into web-platform-tests:master Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for contenteditable=false elements inside contenteditable=true elements and for selection in empty elements #35862

Add tests for contenteditable=false elements inside contenteditable=true elements and for selection in empty elements #35862

Comandeer commented Sep 10, 2022

jgraham commented Sep 12, 2022

masayuki-nakano Sep 14, 2022

masayuki-nakano Sep 14, 2022

Comandeer Sep 17, 2022

masayuki-nakano Sep 14, 2022

masayuki-nakano commented Sep 14, 2022 •

edited

Loading

Comandeer commented Sep 17, 2022

masayuki-nakano commented Nov 8, 2022

masayuki-nakano Nov 8, 2022 •

edited

Loading

Comandeer Nov 11, 2022

masayuki-nakano Nov 8, 2022

Comandeer Nov 11, 2022

masayuki-nakano Nov 8, 2022

masayuki-nakano Nov 8, 2022

Comandeer Nov 11, 2022

masayuki-nakano Nov 21, 2022

Comandeer Dec 3, 2022

masayuki-nakano Dec 5, 2022

masayuki-nakano Nov 8, 2022

Comandeer Nov 11, 2022

masayuki-nakano Nov 21, 2022

masayuki-nakano commented Nov 8, 2022

wpt-pr-bot commented Nov 11, 2022

Comandeer commented Nov 11, 2022

jgraham commented Nov 22, 2022

masayuki-nakano left a comment

Add tests for contenteditable=false elements inside contenteditable=true elements and for selection in empty elements #35862

Add tests for contenteditable=false elements inside contenteditable=true elements and for selection in empty elements #35862

Conversation

Comandeer commented Sep 10, 2022

Tests

Context

jgraham commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayuki-nakano commented Sep 14, 2022 • edited Loading

Comandeer commented Sep 17, 2022

masayuki-nakano commented Nov 8, 2022

masayuki-nakano Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayuki-nakano commented Nov 8, 2022

wpt-pr-bot commented Nov 11, 2022

Comandeer commented Nov 11, 2022

jgraham commented Nov 22, 2022

masayuki-nakano left a comment

Choose a reason for hiding this comment

masayuki-nakano commented Sep 14, 2022 •

edited

Loading

masayuki-nakano Nov 8, 2022 •

edited

Loading