1651874 - Split `WhiteSpaceVisibilityKeeper::MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` to computation part and modifying DOM part

Status: RESOLVED FIXED

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

Description

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

WhiteSpaceVisibilityKeeper::MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange() (formerly WSRunObject::PrepareToDeleteRangePriv()) computes everything before modifying the DOM tree, and then, may touch the DOM tree at most twice. This is wrong design. Each computation should be done immediately before touching the DOM tree.

Comment 1

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

So, this fix will change editor behavior, but only in edge cases (e.g., mutation event listener changed target text node at first modification).

Comment 2

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

Attachment: Bug 1651874 - part 1: Duplicate the definition of `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` r=m_kato!

This patch is preparation for easier to review.

This patch duplicates the body of
MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange() and calls the
copies from new MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange().
The one is for handling start of deleting range, and the other is for handling
end of deleting range.

As you see, this patch and the following patches do not work because of the
duplicated code. Perhaps, part 2 works if there is no mutation event listeners.
Part 9 works even if there is mutation event listeners which touch the DOM
tree in the worst case (when it touches both end of delete range and start of
delete range, and around the start of deleting range is modified by JS).
Finally, part 9 takes back the original performance.

Depends on D82715

Comment 3

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

Attachment: Bug 1651874 - part 2: Get rid of unnecessary blocks in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato!

Depends on D83217

Comment 4

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

Attachment: Bug 1651874 - part 3: Remove unnecessary variables in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato!

Depends on D83218

Comment 5

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

Attachment: Bug 1651874 - part 4: Get rid of check of out of range in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato!

Depends on D83219

Comment 6

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

Attachment: Bug 1651874 - part 5: Make `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` stop collecting all data first r=m_kato!

Depends on D83220

Comment 7

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

Attachment: Bug 1651874 - part 6: Make the last blocks of `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` use early-return style r=m_kato!

Depends on D83221

Comment 8

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

Attachment: Bug 1651874 - part 7: Rename `ReplaceASCIIWhiteSpacesWithOneNBSP()` to `ReplaceTextAndRemoveEmptyTextNodes()` r=m_kato!

Depends on D83223

Comment 9

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

Attachment: Bug 1651874 - part 8: Make helper methods of `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` return replace or delete data r=m_kato!

For representing delete or replace data which have the range and replace string,
this patch creates ReplaceRangeDataBase class which is stack-only, has
EditorDOMRange or EditorDOMRangeInTexts as the range and has replace string
which can be empty string.

Then, MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange() should
modify the DOM tree by itself. With the following patch, the behavior is
corrected for multiple DOM tree changes.

Depends on D83224

Comment 10

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

Attachment: Bug 1651874 - part 9: Make `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` correct delete range when it modifies the DOM tree at end of deleting range r=m_kato!

This is what the original method should've done. Mutation event listeners may
change the DOM tree at the first modification, but it hasn't checked whether
the range to delete or replace around start of deleting range is still valid
or not.

Depends on D83226

Comment 11

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

Attachment: Bug 1651874 - part 10: Move `GetReplaceRangeDataAtEndOfDeletionRange()` and `GetReplaceRangeDataAtStartOfDeletionRange()` to `TextFragmentData` r=m_kato!

With this patch, the creation cost of TextFragmentData is minimized, but
MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange() can work
with the latest DOM tree information at touching start of the deleting range
even after touching end of the range.

Depends on D83228

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0de6c5accfb2
part 1: Duplicate the definition of `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/526b2c243a61
part 2: Get rid of unnecessary blocks in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5f92809ee7e7
part 3: Remove unnecessary variables in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/cd11ae9b62b4
part 4: Get rid of check of out of range in `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f1d32700df16
part 5: Make `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` stop collecting all data first r=m_kato
https://hg.mozilla.org/integration/autoland/rev/91a43ab116a8
part 6: Make the last blocks of `MakeSureToKeepVisibleStateOfWhiteSpacesAtEndOfDeletingRange()` and `MakeSureToKeepVisibleStateOfWhiteSpacesAtStartOfDeletingRange()` use early-return style r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f2f1d527be0e
part 7: Rename `ReplaceASCIIWhiteSpacesWithOneNBSP()` to `ReplaceTextAndRemoveEmptyTextNodes()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9862b5b2184c
part 8: Make helper methods of `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` return replace or delete data r=m_kato
https://hg.mozilla.org/integration/autoland/rev/76330247b90c
part 9: Make `MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()` correct delete range when it modifies the DOM tree at end of deleting range r=m_kato
https://hg.mozilla.org/integration/autoland/rev/b1f1014ba56d
part 10: Move `GetReplaceRangeDataAtEndOfDeletionRange()` and `GetReplaceRangeDataAtStartOfDeletionRange()` to `TextFragmentData` r=m_kato

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/0de6c5accfb2
https://hg.mozilla.org/mozilla-central/rev/526b2c243a61
https://hg.mozilla.org/mozilla-central/rev/5f92809ee7e7
https://hg.mozilla.org/mozilla-central/rev/cd11ae9b62b4
https://hg.mozilla.org/mozilla-central/rev/f1d32700df16
https://hg.mozilla.org/mozilla-central/rev/91a43ab116a8
https://hg.mozilla.org/mozilla-central/rev/f2f1d527be0e
https://hg.mozilla.org/mozilla-central/rev/9862b5b2184c
https://hg.mozilla.org/mozilla-central/rev/76330247b90c
https://hg.mozilla.org/mozilla-central/rev/b1f1014ba56d