Closed Bug 1774704 Opened 2 years ago Closed 2 years ago

Get rid of `mNewBlockElement`

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
105 Branch
Tracking Flags:
Tracking Status
firefox105 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(21 files)

Bug 1774704 - part 0: Add automated tests of preserving selection while handling some commands r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 1: Make `HTMLEditor::EnsureCaretInBlockElement` only computes new caret point in the given element r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 2: Make `CreateOrChangeBlockContainerElement`, `FormatBlockContainerWithTransaction` and `WrapContentsInBlockquoteElementsWithTransaction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 3: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 4-1: Move `HTMLEditor::GetParentListElementAtSelection` into `AutoRangeArray` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 4-2: Make `HTMLEditor::ChangeListElementType` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 4-3: Make `HTMLEditor::MaybeSplitAncestorsForInsertWithTransaction` stop computing editing host with current selection r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 4-4: Make `HTMLEditor::ChangeSelectedHardLinesToList` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 5-1: Make `HTMLEditor::ChangeMarginStart` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 5-2: Make `HTMLEditor::IndentListChild` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 5-3: Make `HTMLEditor::HandleCSSIndentAtSelectionInternal` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 6: Make `HTMLEditor::HandleHTMLIndentAtSelectionInternal` stop touching `Selection` directly and stop setting `mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-1: Make `HTMLEditor::EnsureHardLineBeginsWithFirstChildOf` and `HTMLEditor::EnsureHardLineEndsWithLastChildOf` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-2: Make `CSSEditUtils::RemoveCSSInlineStyleWithTransaction` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-3: Make `HTMLEditor::RemoveAlignFromDescendants` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-4: Make `HTMLEditor::SetBlockElementAlign` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-5: Make `HTMLEditor::AlignContentsAtSelectionWithEmptyDivElement` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-6: Make `HTMLEditor::AlignContentsInAllTableCellsAndListItems` and `HTMLEditor::AlignBlockContentsWithDivElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 7-7: Make `HTMLEditor::AlignNodesAndDescendants` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 8: Make `HTMLEditor::SetSelectionToAbsoluteAsSubAction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1774704 - part 9: Get rid of `EditorBase::TopLevelEditSubActionData::mNewBlockElement` due to unused r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review

https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3AEditorBase%3A%3ATopLevelEditSubActionData%3E_mNewBlockElement&redirect=false

It's used only for ensuring caret in the block at the post-processing of top level edit sub-actions. First, the name is hard to understand the meaning, and this runs as stateful so that it may change selection after sub-action handlers explicitly collapsing Selection.

So I think that each sub-action handler should manage Selection instead.

The following patches touch the logic to restore selection after handling
commands. However, it seems that they are not tested because I found some
regressions by manual testing, but didn't cause orange on tryserver.

outdent-preserving-selection.tentative.html causes a crash due to the
MOZ_ASSERTION in HTMLEditor::HandleOutdentAtSelectionInternal. It means
that I misunderstand the logic and had put the assertion. I should fix it
later with reading the complicated code again. For now, I just change it
to NS_WARNING_ASSERTION instead.

And also test_cmd_absPos.html is the first test to check toggling position
between static and absolute. Therefore, it detects wrong MOZ_ASSERTs
which test whether EditActionData or TopLevelEditSubActionData is created
or not before they create them by themselves. So, this patch removes the
wrong assertions.

They set TopLevelEditSubActionData::mNewBlockElement and their root callers
in edit sub-action level are only InsertParagraphSeparatorAsSubAction and
FormatBlockContainerAsSubAction, and they are called each other. Therefore,
this patch changes these 3 methods once.

Depends on D152963

It oddly retrieve an ancestor list element which contains one range in the
selection ranges. So, working it with AutoRangeArray which is initialized
with Selection makes HTMLEditor smaller...

Depends on D152965

With taking an AutoRangeArray which is initialized with Selection, it can
restore selection and check if it's collapsed and if collapsed in the expected
list element. Then, its caller can apply the returned range to Selection
because it's an edit sub-action handler.

Depends on D152968

And also this patch makes it and an its caller stop computing editing host
with the latest Selection.

Depends on D152969

It's called only by HTMLEditor::HandleCSSIndentAtSelection which is called
only by HTMLEditor::HandleIndentAtSelection. They don't touch Selection
after calling it. Therefore, we can make it adjust collapsing selection point
by itself.

Depends on D152971

The change of expected assertion count in the crash test is caused by that the
test calls execCommand recursively from the legacy mutation event listeners.
Therefore, stopping updating Selection for each DOM tree change causes that
the target range in the nested edit action is changed. However, the nested
handling has already been disabled by default, so this should not be a problem
for the users.

Depends on D152972

It's only caller is HTMLEditor::AlignContentsAtSelection which is called only
by HTMLEditor::AlignAsSubAction at almost last of it. Therefore, it's safe
to handle mNewBlockElement at the caller of
AlignContentsAtSelectionWithEmptyDivElement is safe.

Depends on D152977

For doing that, this patch also making HTMLEditor::AlignContentsAtSelection
which the only caller of HTMLEditor::AlignNodesAndDescendants handle
Selection with AutoRangeArray because it uses AutoSelectionRestorer and
we need to adjust Selection after restored.

Depends on D152979

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d554ca7850eb
part 0: Add automated tests of preserving selection while handling some commands r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b9a3b6f27a8d
part 1: Make `HTMLEditor::EnsureCaretInBlockElement` only computes new caret point in the given element r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35332 for changes under testing/web-platform/tests
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/bb54603e50c7
part 2: Make `CreateOrChangeBlockContainerElement`, `FormatBlockContainerWithTransaction` and `WrapContentsInBlockquoteElementsWithTransaction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9893b8c357a2
part 3: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d61ac9d43588
part 4-1: Move `HTMLEditor::GetParentListElementAtSelection` into `AutoRangeArray` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/310cd44cff95
part 4-2: Make `HTMLEditor::ChangeListElementType` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/23d50b5617de
part 4-3: Make `HTMLEditor::MaybeSplitAncestorsForInsertWithTransaction` stop computing editing host with current selection r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/32d794b2a45a
part 4-4: Make `HTMLEditor::ChangeSelectedHardLinesToList` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/661e25343a15
part 5-1: Make `HTMLEditor::ChangeMarginStart` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/80ed14c716a2
part 5-2: Make `HTMLEditor::IndentListChild` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/695d708318a5
part 5-3: Make `HTMLEditor::HandleCSSIndentAtSelectionInternal` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1e9e0b9b8536
part 6: Make `HTMLEditor::HandleHTMLIndentAtSelectionInternal` stop touching `Selection` directly and stop setting `mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f9d7d41f5043
part 7-1: Make `HTMLEditor::EnsureHardLineBeginsWithFirstChildOf` and `HTMLEditor::EnsureHardLineEndsWithLastChildOf` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3efbfdf889b4
part 7-2: Make `CSSEditUtils::RemoveCSSInlineStyleWithTransaction` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/511a7e64402c
part 7-3: Make `HTMLEditor::RemoveAlignFromDescendants` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/808c433c7774
part 7-4: Make `HTMLEditor::SetBlockElementAlign` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/27d5779ea393
part 7-5: Make `HTMLEditor::AlignContentsAtSelectionWithEmptyDivElement` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7f56b90bd017
part 7-6: Make `HTMLEditor::AlignContentsInAllTableCellsAndListItems` and `HTMLEditor::AlignBlockContentsWithDivElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a0dd784acd4c
part 7-7: Make `HTMLEditor::AlignNodesAndDescendants` stop touching `Selection` directly and setting `mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8896439912aa
part 8: Make `HTMLEditor::SetSelectionToAbsoluteAsSubAction` stop setting `TopLevelEditSubActionData::mNewBlockElement` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3ea6eaf0dc16
part 9: Get rid of `EditorBase::TopLevelEditSubActionData::mNewBlockElement` due to unused r=m_kato
Upstream PR merged by moz-wptsync-bot

== Change summary for alert #35027 (as of Sat, 06 Aug 2022 07:46:45 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
3% google-slides LastVisualChange macosx1015-64-shippable-qr bytecode-cached fission warm webrender 1,805.00 -> 1,753.33

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

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

Attachment

General

Creator:
Created:
Updated:
Size: