Closed Bug 1789967 Opened 3 months ago Closed 3 months ago

Redesign `EditorBase::InitEditorContentAndSelection` for `HTMLEditor`

Categories

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

defect

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Regressed 1 open bug)

Details

Attachments

(5 files)

It calls CollapseSelectionToEndOfLastLeafNode() even for HTMLEditor, however, it works with GetRoot(). Therefore, it may set selection outside editing host and do it without focus of an editing host.

Severity: -- → S3
Priority: -- → P3

The method is enough simple, and uses bad cast from point of view of OOP.
Therefore, this patch make the sub classes implement the method only for each.

Depends on D157405

It does different thing for TextEditor and HTMLEditor, and used almost
internally. Therefore, it should be implemented in the sub classes and
we should name them better.

Depends on D157406

They and their callees work with the result of GetRoot() which is the document
element or the body element. If the body is not editable, Selection should
not be updated in non-editable region nor <br> elements should not be
inserted in both non-focused editable elements and non-editable elements.
Therefore, they should run only when the document element or the <body>
element is editable.

To keep testing crashtests as reported, this patch makes tests which have
contenteditable except <html> and <body> initialize Selection as
what we've done. And clean up the tests for helping to port them to WPT
in the future (bug 1725850).

Depends on D157407

It may be called even when there is no selection range and focused element.
However, it assumes that there is a selection range, and an editable element
has focus. Therefore, now, if there is an editing host and user tries to
do "Select All" without clicking somewhere before doing it, "Select All" does
nothing.

Depends on D157408

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/edac234f1eba
part 1: Make `TextEditor` and `HTMLEditor` implement `EditorBase::InitEditorContentAndSelection` by themselves r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f2f431ee78a3
part 2: Make `TextEditor` and `HTMLEditor` implement `EditorBase::CollapseSelectionToEndOfLastLeafNode` by themselves r=m_kato

Backed out for causing wd failures on content_editable.py.

Push with failures

Failure log

Backout link

[task 2022-09-21T01:00:37.732Z] 01:00:37     INFO - STDOUT: =================================== FAILURES ===================================
[task 2022-09-21T01:00:37.733Z] 01:00:37     INFO - STDOUT: _______________________ test_sets_insertion_point_to_end _______________________
[task 2022-09-21T01:00:37.734Z] 01:00:37     INFO - STDOUT: session = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.742Z] 01:00:37     INFO - STDOUT: inline = <function inline.<locals>.inline at 0x7fdc1eb96378>
[task 2022-09-21T01:00:37.742Z] 01:00:37     INFO - STDOUT:     def test_sets_insertion_point_to_end(session, inline):
[task 2022-09-21T01:00:37.743Z] 01:00:37     INFO - STDOUT:         session.url = inline('<div contenteditable=true>Hello,</div>')
[task 2022-09-21T01:00:37.743Z] 01:00:37     INFO - STDOUT:         input = session.find.css("div", all=False)
[task 2022-09-21T01:00:37.743Z] 01:00:37     INFO - STDOUT:         input.send_keys(' world!')
[task 2022-09-21T01:00:37.744Z] 01:00:37     INFO - STDOUT:         text = session.execute_script('return arguments[0].innerText', args=[input])
[task 2022-09-21T01:00:37.744Z] 01:00:37     INFO - STDOUT: >       assert "Hello, world!" == text.strip()
[task 2022-09-21T01:00:37.744Z] 01:00:37     INFO - STDOUT: E       AssertionError: assert 'Hello, world!' == 'world!Hello,'
[task 2022-09-21T01:00:37.745Z] 01:00:37     INFO - STDOUT: E         - world!Hello,
[task 2022-09-21T01:00:37.745Z] 01:00:37     INFO - STDOUT: E         + Hello, world!
[task 2022-09-21T01:00:37.746Z] 01:00:37     INFO - STDOUT: inline     = <function inline.<locals>.inline at 0x7fdc1eb96378>
[task 2022-09-21T01:00:37.746Z] 01:00:37     INFO - STDOUT: input      = <Element 6c616a28-73c9-4e4b-91d6-6b6e0328ff46>
[task 2022-09-21T01:00:37.746Z] 01:00:37     INFO - STDOUT: session    = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.747Z] 01:00:37     INFO - STDOUT: text       = '\xa0world!Hello,'
[task 2022-09-21T01:00:37.747Z] 01:00:37     INFO - STDOUT: tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py
[task 2022-09-21T01:00:37.747Z] 01:00:37     INFO - STDOUT: :6: AssertionError
[task 2022-09-21T01:00:37.748Z] 01:00:37     INFO - STDOUT: ______________ test_sets_insertion_point_to_after_last_text_node _______________
[task 2022-09-21T01:00:37.748Z] 01:00:37     INFO - STDOUT: session = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.748Z] 01:00:37     INFO - STDOUT: inline = <function inline.<locals>.inline at 0x7fdc1eb60840>
[task 2022-09-21T01:00:37.748Z] 01:00:37     INFO - STDOUT:     def test_sets_insertion_point_to_after_last_text_node(session, inline):
[task 2022-09-21T01:00:37.749Z] 01:00:37     INFO - STDOUT:         session.url = inline('<div contenteditable=true>Hel<span>lo</span>,</div>')
[task 2022-09-21T01:00:37.749Z] 01:00:37     INFO - STDOUT:         input = session.find.css("div", all=False)
[task 2022-09-21T01:00:37.750Z] 01:00:37     INFO - STDOUT:         input.send_keys(" world!")
[task 2022-09-21T01:00:37.751Z] 01:00:37     INFO - STDOUT:         text = session.execute_script("return arguments[0].innerText", args=[input])
[task 2022-09-21T01:00:37.751Z] 01:00:37     INFO - STDOUT: >       assert "Hello, world!" == text.strip()
[task 2022-09-21T01:00:37.752Z] 01:00:37     INFO - STDOUT: E       AssertionError: assert 'Hello, world!' == 'Hello world!,'
[task 2022-09-21T01:00:37.753Z] 01:00:37     INFO - STDOUT: E         - Hello world!,
[task 2022-09-21T01:00:37.753Z] 01:00:37     INFO - STDOUT: E         ?             -
[task 2022-09-21T01:00:37.759Z] 01:00:37     INFO - STDOUT: E         + Hello, world!
[task 2022-09-21T01:00:37.759Z] 01:00:37     INFO - STDOUT: E         ?      +
[task 2022-09-21T01:00:37.760Z] 01:00:37     INFO - STDOUT: inline     = <function inline.<locals>.inline at 0x7fdc1eb60840>
[task 2022-09-21T01:00:37.761Z] 01:00:37     INFO - STDOUT: input      = <Element aecf351a-6fc8-48f2-9731-11757cc799bf>
[task 2022-09-21T01:00:37.761Z] 01:00:37     INFO - STDOUT: session    = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.762Z] 01:00:37     INFO - STDOUT: text       = 'Hello world!,'
[task 2022-09-21T01:00:37.762Z] 01:00:37     INFO - STDOUT: tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py
[task 2022-09-21T01:00:37.763Z] 01:00:37     INFO - STDOUT: :18: AssertionError
[task 2022-09-21T01:00:37.763Z] 01:00:37     INFO - STDOUT: =========================== short test summary info ============================
[task 2022-09-21T01:00:37.763Z] 01:00:37     INFO - STDOUT: FAILED tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py::test_sets_insertion_point_to_end
[task 2022-09-21T01:00:37.764Z] 01:00:37     INFO - STDOUT: FAILED tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py::test_sets_insertion_point_to_after_last_text_node
[task 2022-09-21T01:00:37.764Z] 01:00:37     INFO - STDOUT: ============================== 2 failed in 5.59s ===============================
[task 2022-09-21T01:00:37.764Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO - TEST-UNEXPECTED-FAIL | /webdriver/tests/element_send_keys/content_editable.py | test_sets_insertion_point_to_end - AssertionError: assert 'Hello, world!' == 'world!Hello,'
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO - session = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO - inline = <function inline.<locals>.inline at 0x7fdc1eb96378>
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO -     def test_sets_insertion_point_to_end(session, inline):
[task 2022-09-21T01:00:37.765Z] 01:00:37     INFO -         session.url = inline('<div contenteditable=true>Hello,</div>')
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO -         input = session.find.css("div", all=False)
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO -         input.send_keys(' world!')
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO -         text = session.execute_script('return arguments[0].innerText', args=[input])
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - >       assert "Hello, world!" == text.strip()
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - E       AssertionError: assert 'Hello, world!' == 'world!Hello,'
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - E         - world!Hello,
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - E         + Hello, world!
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.766Z] 01:00:37     INFO - inline     = <function inline.<locals>.inline at 0x7fdc1eb96378>
[task 2022-09-21T01:00:37.767Z] 01:00:37     INFO - input      = <Element 6c616a28-73c9-4e4b-91d6-6b6e0328ff46>
[task 2022-09-21T01:00:37.767Z] 01:00:37     INFO - session    = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.767Z] 01:00:37     INFO - text       = '\xa0world!Hello,'
[task 2022-09-21T01:00:37.767Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.767Z] 01:00:37     INFO - tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py:6: AssertionError
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO - TEST-UNEXPECTED-FAIL | /webdriver/tests/element_send_keys/content_editable.py | test_sets_insertion_point_to_after_last_text_node - AssertionError: assert 'Hello, world!' == 'Hello world!,'
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO - session = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO - inline = <function inline.<locals>.inline at 0x7fdc1eb60840>
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO -     def test_sets_insertion_point_to_after_last_text_node(session, inline):
[task 2022-09-21T01:00:37.768Z] 01:00:37     INFO -         session.url = inline('<div contenteditable=true>Hel<span>lo</span>,</div>')
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO -         input = session.find.css("div", all=False)
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO -         input.send_keys(" world!")
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO -         text = session.execute_script("return arguments[0].innerText", args=[input])
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO - >       assert "Hello, world!" == text.strip()
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO - E       AssertionError: assert 'Hello, world!' == 'Hello world!,'
[task 2022-09-21T01:00:37.769Z] 01:00:37     INFO - E         - Hello world!,
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - E         ?             -
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - E         + Hello, world!
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - E         ?      +
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - inline     = <function inline.<locals>.inline at 0x7fdc1eb60840>
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - input      = <Element aecf351a-6fc8-48f2-9731-11757cc799bf>
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - session    = <Session 6d9ca2ac-7e52-4f26-8b6a-be7faa41d15d>
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - text       = 'Hello world!,'
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - 
[task 2022-09-21T01:00:37.770Z] 01:00:37     INFO - tests/web-platform/tests/webdriver/tests/element_send_keys/content_editable.py:18: AssertionError
[task 2022-09-21T01:00:37.771Z] 01:00:37     INFO - TEST-OK | /webdriver/tests/element_send_keys/content_editable.py | took 5838ms
Flags: needinfo?(masayuki)
Pushed by imoraru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/555d2ca16477
part 1: Make `TextEditor` and `HTMLEditor` implement `EditorBase::InitEditorContentAndSelection` by themselves r=m_kato CLOSED TREE

I have re-landed part 1 part 1: Make TextEditor and HTMLEditor implement EditorBase::InitEditorContentAndSelection by themselves because the backfill shows that only part 2 is responsible for the failures. Sorry for any inconvenience.

Hmm, it's odd. Chrome passes the tests:
https://wpt.fyi/results/webdriver/tests/element_send_keys/content_editable.py?label=experimental&label=master&aligned

However, with checking the behavior of Chrome in data:text/html,<div contenteditable autofocus>abc</div> and data:text/html,<div contenteditable autofocus>abc</div><script>document.querySelector("div").focus();</script>, the caret appears at start of the editor and typing text is inserted at start of the editor as the failed result.

Therefore, I'd like to check what happens before sending the key events. However, according to the spec, send_keys do not change focus automatically and I don't find the code moving focus, so somebody else has already been changed before the call of send_keys.

jgraham: Do you have any ideas what's going on? My patches make HTMLEditor stop initializing selection when HTMLEditor is being initialized unless entire the document is editable. Therefore, HTMLEditor stops collapsing Selection to the end of the last editable text node in the test cases. That's the reason why the text is inserted at start of the first text node of the focused editor.

Flags: needinfo?(masayuki) → needinfo?(james)

(In reply to Iulian Moraru from comment #9)

I have re-landed part 1 part 1: Make TextEditor and HTMLEditor implement EditorBase::InitEditorContentAndSelection by themselves because the backfill shows that only part 2 is responsible for the failures. Sorry for any inconvenience.

Oh, thank you. It's find either the patch is also backed out for managing the regression easier, or keeping the simple refactoring for the other developers who are writing their own patches.

Oh, I'm confused with the part.3 patch. The part.2 should not change the initializing behavior of the HTMLEditor...

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

(In reply to Iulian Moraru from comment #9)

I have re-landed part 1 part 1: Make TextEditor and HTMLEditor implement EditorBase::InitEditorContentAndSelection by themselves because the backfill shows that only part 2 is responsible for the failures. Sorry for any inconvenience.

Oh, thank you. It's find either the patch is also backed out for managing the regression easier, or keeping the simple refactoring for the other developers who are writing their own patches.

Do you want me to backout part 1 again? If you think it will be easier for you to fix the issue, I can do it.

Flags: needinfo?(masayuki)

Yeah, could you back out the part.1 patch again? I thought that the failure caused by a simple mistake, and indeed, the part.2 contains only one mistake. However, anyway part.3 and part.4 break the behavior again and intentionally. Therefore, I cannot re-land the remaining patches immediately.

Flags: needinfo?(masayuki)

The part.2 patch contains a simple mistake so that I'll fix it with a single line change:

    return HTMLEditUtils::IsContainerNode(*lastLeafContent)
               ? EditorRawDOMPoint::AtEndOf(*lastLeafContent)
               : EditorRawDOMPoint(lastLeafContent);

to

    return lastLeafContent->IsText() ||
                   HTMLEditUtils::IsContainerNode(*lastLeafContent)
               ? EditorRawDOMPoint::AtEndOf(*lastLeafContent)
               : EditorRawDOMPoint(lastLeafContent);

However, the part.3 intentionally changes the behavior to what the failed result of element_send_keys/content_editable.py, so I need to investigate more.

Backout of part 1 done, as per your request.

Backout link

The test comes from this issue.

I believe that we need to find the last Text node that is a child of element and then call setSelection on that if dealing with a Content Editable. In addition, we probably don't want the select event firing.

So he thinks that it should be done automatically by web driver. However, before landing these patches, the tests unexpectedly pass that our HTMLEditor initially sets Selection to end of the last leaf child of <body>, and the tests do not have non-editable elements at last of the <body> and do not update selection before testing (e.g., collapsing before the editor, etc).

So probably, we should mark them as known failure because of existing GeckoDriver's bug. WDYT, jgraham?

According to the issue which added the test, the test intended that web driver
collapsing selection to end of the last editable text node at sending the text
do an editable element. However, it seems that GeckoDriver does not do it but
the test accidentally passed since HTMLEditor has collapsed selection to end
of last leaf node even if the node is not editable. Therefore, the test does
not check what the author expected enough (e.g., when there is another node
at end of the <body>, when there is a collapsed selection range outside
editor, when another editable element has focus).

Therefore, we can just mark the tests as known failures since we've not
explicitly support the behavior yet.

Attachment #9295512 - Attachment description: Bug 1789967 - part 5: Mark all tests in `webdriver/tests/element_send_keys/content_editable.py` as known failures r=jgraham! → Bug 1789967 - part 5: Mark all tests in `webdriver/tests/element_send_keys/content_editable.py` as known failures
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cc2e8b7c548e
part 1: Make `TextEditor` and `HTMLEditor` implement `EditorBase::InitEditorContentAndSelection` by themselves r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cdcef4f3f635
part 2: Make `TextEditor` and `HTMLEditor` implement `EditorBase::CollapseSelectionToEndOfLastLeafNode` by themselves r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9b15baab6f77
part 3: Make `HTMLEditor::CollapseSelectionToEndOfLastLeafNodeOfDocument` and `HTMLEditor::InitEditorContentAndSelection` do nothing if the document is partially editable r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6a9ce91072fd
part 4: Make `HTMLEditor::SelectAllInternal` work without selection range r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4bfcb4ab080f
part 5: Mark all tests in `webdriver/tests/element_send_keys/content_editable.py` as known failures r=webdriver-reviewers,whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36012 for changes under testing/web-platform/tests
Regressions: 1791976
Upstream PR merged by moz-wptsync-bot

The webdriver bug around focus/selection seems similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1791736; it looks like the spec is broken and our implementation even more so. But it should be fixable.

Flags: needinfo?(james)
See Also: → 1791736

jgraham: Thank you, and I'm sorry that I should've canceled the ni when I post the patch and canceled the review request to you because a group reviewer is available for it. In the review, whimboo told me bug 1364426, it's what I see the trouble in the test.

No longer regressions: 1791976

(In reply to Pulsebot from comment #22)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9b15baab6f77
part 3: Make HTMLEditor::CollapseSelectionToEndOfLastLeafNodeOfDocument
and HTMLEditor::InitEditorContentAndSelection do nothing if the document
is partially editable r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6a9ce91072fd
part 4: Make HTMLEditor::SelectAllInternal work without selection range
r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4bfcb4ab080f
part 5: Mark all tests in
webdriver/tests/element_send_keys/content_editable.py as known failures
r=webdriver-reviewers,whimboo

== Change summary for alert #35538 (as of Wed, 28 Sep 2022 09:37:29 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
56% perf_reftest_singletons line-iterator.html windows10-64-shippable-qr e10s fission stylo webrender 2,036.15 -> 903.73
55% perf_reftest_singletons line-iterator.html windows10-64-shippable-qr e10s fission stylo webrender 2,040.13 -> 917.03
54% perf_reftest_singletons line-iterator.html linux1804-64-shippable-qr e10s fission stylo webrender 1,990.66 -> 913.80
52% perf_reftest_singletons line-iterator.html macosx1015-64-shippable-qr e10s fission stylo webrender 2,266.86 -> 1,090.76

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35538

(In reply to Andrej (:andrej) from comment #28)

(In reply to Pulsebot from comment #22)
== Change summary for alert #35538 (as of Wed, 28 Sep 2022 09:37:29 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
56% perf_reftest_singletons line-iterator.html windows10-64-shippable-qr e10s fission stylo webrender 2,036.15 -> 903.73
55% perf_reftest_singletons line-iterator.html windows10-64-shippable-qr e10s fission stylo webrender 2,040.13 -> 917.03
54% perf_reftest_singletons line-iterator.html linux1804-64-shippable-qr e10s fission stylo webrender 1,990.66 -> 913.80
52% perf_reftest_singletons line-iterator.html macosx1015-64-shippable-qr e10s fission stylo webrender 2,266.86 -> 1,090.76

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35538

Ah, Selection::Modify does nothing when there is no selection range:
https://searchfox.org/mozilla-central/rev/12a34801fbaa6f14e20515a44b1fd77179b02617/dom/base/Selection.cpp#3311-3313

Therefore, the test does nothing. It needs to create a selection range first.

Regressions: 1797948
Regressions: 1803626
You need to log in before you can comment on or make changes to this bug.