Closed Bug 1726064 Opened 3 years ago Closed 3 years ago

`HTMLEditUtils::Get(Inclusive)?AncestorBlockElement` may cross editing host boundaries

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
93 Branch
Tracking Flags:
Tracking Status
firefox93 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(21 files)

Bug 1726064 - part 1: Redesign `HTMLEditUtils::ClosestEditableBlockElementOrEditingHost()` with `enum class` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 2: Get rid of `HTMLEditUtils::GetInclusiveAncestorBlockElementExceptHRElement` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 3: Make `HTMLEditor::MaybeCollapseSelectionAtFirstEditableNode()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 4: Make `HTMLEditor::HandleKeyPressEvent()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 5: Make `HTMLEditor::GetCSSBackgroundColorState()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 6: Make `HTMLEditor::RemoveEmptyInclusiveAncestorInlineElements()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 7: Make `HTMLEditor::SetCSSBackgroundColorWithTransaction()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 8: Make `HTMLEditor::HTMLWithContextInserter::InsertContents()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 9: Make `TextServicesDocument::HasSameBlockNodeParent()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 10: Make the constructor of `AlignStateAtSelection` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 11: Make `AutoBlockElementsJoiner::PrepareToDeleteNonCollapsedRanges()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 12: Make `HTMLEditor::CanMoveOrDeleteSomethingInHardLine()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 13: Make `AutoEmptyBlockAncestorDeleter::ScanEmptyBlockInclusiveAncestor()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 14: Make `AutoDeleteRangesHandler::ExtendRangeToIncludeInvisibleNodes()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 15: Make `HTMLEditor::EnsureCaretNotAfterPaddingBRElement()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 16: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` and `HTMLEditUtils::IsSplittableNode()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 17: Make `HTMLEditor::FormatBlockContainerWithTransaction()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 18: Make `HTMLEditor::HandleCSSIndentAtSelectionInternal()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 19: Make `HTMLEditor::SelectBRElementIfCollapsedInEmptyBlock()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 20: Make `HTMLEditor::RemoveBlockContainerElements()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1726064 - part 21: Make `HTMLEditor::AdjustCaretPositionAndEnsurePaddingBRElement()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato!
(deleted), text/x-phabricator-request
Details

These utility methods take only ancestor limiter, but it does not check whether it meets another editing host boundary. E.g., when the start content is in a child editing host of focused editing host, they may return non-editable block element.

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

There are a lot of ancestor scanners in HTMLEditUtils. This is good thing
for the performance, but it makes us hard to maintain. Therefore, we should
merge them as far as possible.

It does not check whether it meets a non-editable parent or not. Therefore,
it may cross another editing host boundary when aContent is in a nested
editing host.

So, this patch fixes some edge cases when editing hosts are nested and
scanning from inner editing host.

Depends on D122939

With the new tests, I see the implementation is completely broken. It tries
to return block level element's background color, but it assumes that inline
elements are never parent of block elements. Therefore, this patch fixes the
bug too.

Note that this feature (retrieving only block level background color) is
Gecko specific and not available from the web. Therefore, it tests this change
with XPCOM interfaces directly.

Depends on D122942

This patch fixes a bug of setting background color of block parent. It should
not work with contenteditable in the comm-central applications, but we should
make it modify only editable block parent.

Depends on D122944

For keeping current behavior, the options should be set to
HTMLEditUtils::ClosestEditableBlockElement, but it may cause returning
nullptr if the text node is in an inline editing host, and also cause
returning true from the method even when the text nodes are in different
inline editing hosts. Therefore, this patch uses
HTMLEditUtils::ClosestEditableBlockElementOrInlineEditingHost instead.

Depends on D122946

The "state" is not limited by editing host nor editable nodes. Therefore, it
should keep scanning ancestor block element without checking editable state.

Depends on D122947

I have following patches for them, but I still don't add automated tests for them. I'll try to post them tomorrow.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/df2b5e5da3b3
part 1: Redesign `HTMLEditUtils::ClosestEditableBlockElementOrEditingHost()` with `enum class` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ff0e2e535916
part 2: Get rid of `HTMLEditUtils::GetInclusiveAncestorBlockElementExceptHRElement` r=m_kato

I have no idea how to test this, but I believe that checking non-editable
block element must be correct here because it checks whether the block is
empty or not for merging lines.

Depends on D123029

Additionally, the given content may be in a nested editing host. Then, this
method may delete a nested-block editing host if it's empty. Therefore,
this patch get rid of aEditingHost and check same thing with
HTMLEditUtils::IsRemovableFromParentNode().

Depends on D123066

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/117469e0f097
part 3: Make `HTMLEditor::MaybeCollapseSelectionAtFirstEditableNode()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato

I have no idea how to test this without making unrealistic cases with legacy
mutation event listeners because now, Gecko ignores selection ranges which
crosses editing host boundaries when it tries to delete selection.

And invisible white-spaces appear end of a block or start of a block, i.e.,
both block parents in the range boundaries are always editable in normal cases.
So, such range should be ignored first.

Depends on D123067

The method assumes two wrong things:

  • The padding <br> element flag may not be set (another bug)
  • There may be <br> element which is created by the web app

But its callers want to put caret at invisible <br> element if selection
is collapsed after it. Therefore, this patch fixes the method for passing
the new tests and rename it.

And also this patch changes the expected result of some tests in inserthtml.js
because their expected result are based on Gecko, i.e., both Blink/WebKit
fail, but their result is better for keeping the invisible <br> element
visibility.
https://wpt.fyi/results/editing/run/inserthtml.html?run_id=5747864689967104&run_id=5201845715730432&run_id=5735315550502912&run_id=5763864667881472

Depends on D123068

Comparing found editable block with editing host does not make sense if
focused editing host has another editing host and selection is in the
child editing host since the child editing host should be compared with the
found block element. Therefore, this patch makes it use IsSplittableNode()
instead of the comparing.

Depends on D123069

I have no idea how to write test for this kind of preparation method of
following range computation to do something.

Depends on D123072

I have no idea how to test this because this method is a clean up method
after handling everything. This patch just adds same editing host check,
but the others are not changed.

Finally, the legacy APIs are not used anymore. So, let's remove them!

Depends on D123074

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30086 for changes under testing/web-platform/tests
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ae7ce0245ffb
part 4: Make `HTMLEditor::HandleKeyPressEvent()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/020ce29803ef
part 5: Make `HTMLEditor::GetCSSBackgroundColorState()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0bd2f11802ca
part 6: Make `HTMLEditor::RemoveEmptyInclusiveAncestorInlineElements()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/82b658998176
part 7: Make `HTMLEditor::SetCSSBackgroundColorWithTransaction()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ae248eba41f4
part 8: Make `HTMLEditor::HTMLWithContextInserter::InsertContents()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d2c427de0939
part 9: Make `TextServicesDocument::HasSameBlockNodeParent()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato
Upstream PR merged by moz-wptsync-bot
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3b9b5769152e
part 10: Make the constructor of `AlignStateAtSelection` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30114 for changes under testing/web-platform/tests
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7af78405aade
part 11: Make `AutoBlockElementsJoiner::PrepareToDeleteNonCollapsedRanges()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/62290dcae04e
part 12: Make `HTMLEditor::CanMoveOrDeleteSomethingInHardLine()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/30478b345a7e
part 13: Make `AutoEmptyBlockAncestorDeleter::ScanEmptyBlockInclusiveAncestor()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b016b162fde1
part 14: Make `AutoDeleteRangesHandler::ExtendRangeToIncludeInvisibleNodes()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/61c466d08db1
part 15: Make `HTMLEditor::EnsureCaretNotAfterPaddingBRElement()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato
Regressions: 1727008
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/24080f662446
part 16: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` and `HTMLEditUtils::IsSplittableNode()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/35c243277dad
part 17: Make `HTMLEditor::FormatBlockContainerWithTransaction()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ede71729f08c
part 18: Make `HTMLEditor::HandleCSSIndentAtSelectionInternal()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/895581338857
part 19: Make `HTMLEditor::SelectBRElementIfCollapsedInEmptyBlock()` use `HTMLEditUtils::GetInclusiveAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/54ef3dd91e3d
part 20: Make `HTMLEditor::RemoveBlockContainerElements()` use `HTMLEditUtils::GetAncestorElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/571fa8e5ae51
part 21: Make `HTMLEditor::AdjustCaretPositionAndEnsurePaddingBRElement()` use `HTMLEditUtils::Get(Inclusive)AncestorElement()` r=m_kato
Upstream PR merged by moz-wptsync-bot
Regressions: 1737540
Regressions: 1740492
Regressions: 1750588
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: