1781994 - Make inline property setter and remover stop touching `Selection` directly

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Currently, it modifies Selection a lot and multiple updates of the DOM tree refers Selection, so the nested for loops in it is stateful and hard to understand.

I think that I can refactor it much simpler than now with the new helper classes and methods.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 1: Make `HTMLEditor::SplitAncestorStyledInlineElementsAt` and `HTMLEditor::ClearStyleAt` stop touching `Selection` directly r=m_kato!

I've already made a caller of HTMLEditor::ClearStyleAt,
HTMLEditor::CreateStyleForInsertText, in bug 1770877, so this fixes a bug of
the patch.

HTMLEditor::ClearStyleAt is still updates Selection only in some cases.
And one of the caller has not handle the Selection update. Therefore, once
it completely stop touching Selection, ComputeEditingHost will fail and
the other paths update Selection, so it should do it too. (Without the
change, test_dragdrop.html fails.)

Finally, we don't need EditResult anymore because we have
Result<EditorDOMPoint, nsresult>.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 2: Make `HTMLEditor::SplitAncestorStyledInlineElementsAtRangeEdges` stop touching `Selection` directly r=m_kato!

Depends on D154343

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 3: Make `HTMLEditor::SetInlinePropertyOnTextNode` stop touching `Selection` directly r=m_kato!

Depends on D154344

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 4: Make `HTMLEditor::RemoveInlinePropertyInternal` use `AutoRangeArray` r=m_kato!

I'd like to get rid of AutoSelectionRangeArray and AutoRestoreSelection,
and I'd like to make it stop touching Selection while it is removing style
of each content nodes in the range. Therefore, this patch rewrites it with
AutoRangeArray. Then, we can reduce the indent level of the nested for
loops.

Note that it creates AutoEditSubActionNotifier, so it's a sub-edit action
handler. Therefore, this patch renames it.

Depends on D154345

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 5: Make `HTMLEditor::RemoveInlinePropertyAsSubAction` handle correct text node after splitting a text to any direction r=m_kato!

This is a pre-fix for bug 1735608. Currently, the loop assumes that collected
nodes are always right node, but it'll be changed, therefore, it creates
new array for the follow up loops handle expected nodes.

Depends on D154346

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 6: Make `HTMLEditor::SetInlinePropertyInternal` use `AutoRangeArray` r=m_kato!

Depends on D154347

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 7: Make `HTMLEditor::RemoveInlinePropertyAsSubAction` handle multiple styles once r=m_kato!

I think that HTMLEditor::SetInlinePropertyAsSubAction and
HTMLEditor::RemoveInlinePropertyAsSubAction should not be called multiple
times in one edit action. Therefore, I'd like to make them take multiple
styles once. Then, we could collect all targets before touching the DOM tree
in the future.

Depends on D154348

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 8: Make `HTMLEditor::SetInlinePropertyAsSubAction` handle multiple styles once r=m_kato!

Depends on D154349

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 9: Make `HTMLEditor::RelativeFontChangeOnNode` and `HTMLEditor::RelativeFontChangeHelper` use `FontSize` r=m_kato!

HTMLEditor::RelativeFontChange() and its helpers are based on
SetInlinePropertiesAsSubAction() and its helpers. Therefore, they may have
similar problem to switch join/split direction. Therefore, this and the
following patches clean them up too.

Depends on D154350

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 10: Make `HTMLEditor::RelativeFontChangeOnNode` and `HTMLEditor::RelativeFontChangeHelper` stop touching `Selection` directly r=m_kato!

They are renamed to SetFontSizeWithBigOrSmallElement and
SetFontSizeOfFontElementChildren.

Depends on D154351

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 11: Clean up `HTMLEditor::RelativeFontChange` r=m_kato!

It should use AutoRangeArray to stop using AutoSelectionRestorer.

Depends on D154352

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1781994 - part 12: Make `HTMLEditor::SetCSSBackgroundColorWithTransaction` use similar approach as `SetInlinePropertiesAsSubAction` etc r=m_kato!

It's a Gecko specific feature, and it sets background color of parent block
elements of selection ranges. This does similar things to
SetInlinePropertiesAsSubAction, but still refers Selection directly and
uses AutoSelectionRestorer. For consistency between similar methods, this
patch makes it use AutoRangeArray.

Depends on D154353

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2c295e5a3ff
part 1: Make `HTMLEditor::SplitAncestorStyledInlineElementsAt` and `HTMLEditor::ClearStyleAt` stop touching `Selection` directly r=m_kato

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52f5e6c77f78
part 2: Make `HTMLEditor::SplitAncestorStyledInlineElementsAtRangeEdges` stop touching `Selection` directly r=m_kato

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46ae9281c1b2
part 3: Make `HTMLEditor::SetInlinePropertyOnTextNode` stop touching `Selection` directly r=m_kato

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99211597b4ee
part 4: Make `HTMLEditor::RemoveInlinePropertyInternal` use `AutoRangeArray` r=m_kato

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f7f2298561e
part 5: Make `HTMLEditor::RemoveInlinePropertyAsSubAction` handle correct text node after splitting a text to any direction r=m_kato

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2001ca2b890e
part 6: Make `HTMLEditor::SetInlinePropertyInternal` use `AutoRangeArray` r=m_kato

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81b5e7d21c52
part 7: Make `HTMLEditor::RemoveInlinePropertyAsSubAction` handle multiple styles once r=m_kato

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e11a81176253
part 8: Make `HTMLEditor::SetInlinePropertyAsSubAction` handle multiple styles once r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ea6146e05cc3
part 9: Make `HTMLEditor::RelativeFontChangeOnNode` and `HTMLEditor::RelativeFontChangeHelper` use `FontSize` r=m_kato

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d08cca9028d6
part 10: Make `HTMLEditor::RelativeFontChangeOnNode` and `HTMLEditor::RelativeFontChangeHelper` stop touching `Selection` directly r=m_kato

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7f8eb028923
part 11: Clean up `HTMLEditor::RelativeFontChange` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/aa6f8692f8d0
part 12: Make `HTMLEditor::SetCSSBackgroundColorWithTransaction` use similar approach as `SetInlinePropertiesAsSubAction` etc r=m_kato

Comment 23

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c2c295e5a3ff
https://hg.mozilla.org/mozilla-central/rev/52f5e6c77f78
https://hg.mozilla.org/mozilla-central/rev/46ae9281c1b2
https://hg.mozilla.org/mozilla-central/rev/99211597b4ee
https://hg.mozilla.org/mozilla-central/rev/4f7f2298561e
https://hg.mozilla.org/mozilla-central/rev/2001ca2b890e
https://hg.mozilla.org/mozilla-central/rev/81b5e7d21c52

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e11a81176253
https://hg.mozilla.org/mozilla-central/rev/ea6146e05cc3
https://hg.mozilla.org/mozilla-central/rev/d08cca9028d6
https://hg.mozilla.org/mozilla-central/rev/f7f8eb028923
https://hg.mozilla.org/mozilla-central/rev/aa6f8692f8d0