Closed Bug 1540037 Opened 4 years ago Closed 2 years ago

Move `TextEditor` methods which are used with `HTMLEditor` instances for splitting TextEditor and HTMLEditor classes

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Whiteboard: [h2review-noted])

Attachments

(47 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Currently, we have 3 editor classes.

EditorBase class is an abstract class.

TextEditor class is derived from EditorBase and represents text editor in <input> and <textarea> elements.

HTMLEditor class is derived from TextEditor and represents all editable content in a document, e.g., multiple <div contenteditable> elements or designMode document.

So, even though EditorBase is an abstract class, it's always instanced with TextEditor class.

Therefore, I don't think that we should keep separating them because when we take a look at a handling path, we may need to go EditorBase.cpp and TextEditor.cpp again and again.

If you have some ideas about this issue, let me know.

Priority: -- → P3
Summary: Should we merge EditorBase into TextEditor? → Split TextEditor and HTMLEditor

They are used only by HTMLEditor so that we should hide them from
TextEditor for making it clearer that they are not used by TextEditor.

Note that there are 2 DeleteNodeWithTransaction() in HTMLEditor class.
One is EditorBase's method and the other is HTMLEditor's method.
HTMLEditor's one is check whether the removing node is editable or not,
but in some cases, we need to move non-editable nodes. Therefore, this
patch makes ReplaceContainerWithTransaction() call EditorBase's one
for keeping current behavior.

Depends on D72586

Its users are only HTMLEditor and CSSEditUtils so that we should move it
into HTMLEditor.

Depends on D72822

They are not used by TextEditor so that we should move them into HTMLEditor.

Depends on D72823

It's only used by HTMLEditor so that we can move it.

Depends on D72825

It's used only by HTMLEditor so that we can move it into HTMLEditor.

Depends on D72826

Although this is called by
EditorBase::InsertPaddingBRElementForEmptyLastLineWithTransaction(), it's
mainly designed for HTMLEditor, but used by TextEditor too only when it
puts <br> element to the end of its anonymous <div> element. Therefore,
when TextEditor calls
InsertPaddingBRElementForEmptyLastLineWithTransaction(), aPointToInsert
is not in a text node:
https://searchfox.org/mozilla-central/rev/158bac3df3a1890da55bdb6ffdaf9a7ffc0bfb0a/editor/libeditor/TextEditSubActionHandler.cpp#917-918,920

And it means that PrepareToInsertBRElement() does nothing:
https://searchfox.org/mozilla-central/rev/158bac3df3a1890da55bdb6ffdaf9a7ffc0bfb0a/editor/libeditor/EditorBase.cpp#1561,1569-1570

Therefore, we can move it into HTMLEditor and makes it possible to move
EditorBase::SplitNodeWithTransaction() called by PrepareToInsertBRElement().

Depends on D72830

Now, it's used only by HTMLEditor so that we can move it.

Depends on D72831

And then, SplitNodeTransaction needs to be store HTMLEditor instead of
EditorBase.

Depends on D72832

Then, we can make JoinNodeTransaction store HTMLEditor instead of
EditorBase.

Depends on D72833

It's used only by HTMLEditor so that we can move it.

Depends on D72835

It's used only by HTMLEditor so that we can move it.

Depends on D72836

And this patch renames it to MoveChildrenBetween() for avoiding overload.

Depends on D72844

The other Move*() are not move nodes without transactions. For making
developers to distinguish the difference, this patch appends WithTransaction
postfix like other methods which modify DOM tree with transactions.

Depends on D72846

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e2f1fad33d99
part 1: Move `EditorBase::ReplaceContainerWithTransaction()` and related methods to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1d6a086d101a
part 2: Move `EditorBase::RemoveContainerWithTransaction()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3741ec9234d0
part 3: Move `EditorBase::InsertContainerWithTransaction()` and related methods to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1ba97dfc2ddd
part 4: Move `EditorBase::SplitNodeDeepWithTransaction()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ff18fa29d992
part 5: Move `EditorBase::JoinNodesWithTransaction()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/705c53101ce1
part 6: Move `TextEditor::DeleteSelectionAndCreateElement()` and `TextEditor::DeleteSelectionAndPrepareToCreateNode()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/69e74ec4f750
part 7: Move `EditorBase::PrepareToInsertBRElement()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/eb080a8b68e9
part 8: Move `EditorBase::SplitNodeWithTransaction()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/33c98b169ef2
part 9: Move `EditorBase::DoSplitNode()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/004a3f6df7d9
part 10: Move `EditorBase::DoJoinNodes()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9ea49a36a809
part 11: Move `EditorBase::MoveAllChilren()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3ab6dfbeffce
part 12: Move `EditorBase::MovePreviousSiblings()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5febc73cdbd1
part 13: Move `EditorBase::MoveChildren()` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b68862b930cd
part 14: Move `EditorBase::MoveNodeWithTransaction()` and `EditorBase::MoveNodeToEndWithTransaction()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9be5da1e7893
part 15: Rename `HTMLEditor::MoveChildren()` and `HTMLEditor::MoveNodeOrChildren()` to `*WithTransaction()` r=m_kato
Whiteboard: [h2review-noted]

The leave-open keyword is there and there is no activity for 6 months.
:jstutte, maybe it's time to close this bug?

Flags: needinfo?(jstutte)

:masayuki, is the leave-open here still meaningful? What is the remaining work to do?

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

Still there are a lot of methods which should be moved to another class, for example, from TextEditor to EditorBase. Such methods block to make HTMLEditor stop inheriting TextEditor.

Flags: needinfo?(masayuki)

The list of TextEditor methods.
https://docs.google.com/spreadsheets/d/1QO6FljlIMOaSDxTQrNJuwBHbI8ZToDw0s_328LXEolQ/edit?usp=sharing

Some of them should be moved explicitly, but I'm not sure for some others of them.

IME is available in both TextEditor and HTMLEditor, and the handling
code is almost same (they partially do different things with checking
IsHTMLEditor()). Therefore, we should move them to EditorBase for
making HTMLEditor possible to inherit only EditorBase in the future.

This method is semi-public method, meaning that this is commonly used by
public methods which handle various user input and that causes inputting
text, both in TextEditor and HTMLEditor.

Therefore, for making HTMLEditor stop inheriting TextEditor class in the
future, we should move it into EditorBase.

Depends on D115784

They are used by setting text value of TextEditor or replacing a misspelled
word with a new word in both TextEditor and HTMLEditor. Therefore,
they should be in the EditorBase rather than TextEditor.

Note that the path of the former case may be in a hot path. Therefore, we need
to keep redirecting to TextEditor for keeping the performance only in the
case.

Depends on D115785

Currently, EditorBase::GetDocumentIsEmpty() is implemented by TextEditor,
and it refers only IsEmpty() which is implemented both by TextEditor and
HTMLEditor. So, IsEmpty() should be a virtual method of EditorBase,
then, EditorBase can implement GetDocumentIsEmpty().

Depends on D115786

They just work with a transaction manager and transactions, and they are used
by both TextEditor and HTMLEditor. Therefore, they should be in
EditorBase for making HTMLEditor stop inheriting TextEditor in the
future.

Depends on D115787

It calls only its helper method, and the helper method,
IsCopyToClipboardAllowedInternal(), needs to check complicated things only
when it's a password editor.

This patch makes IsCopyToCLipboardAllowedInternal() a virtual method. But
this shouldn't cause performance regression because this should be called
only when updating menu items or handling "copy" commands. So, this shouldn't
be in any hot paths.

Depends on D115788

Of course, they are used for HTMLEditor instances too. Therefore, they
should be in EditorBase rather than TextEditor.

Depends on D115789

This is used by HTMLEditor too. So, this should be a method of EditorBase.

Depends on D115790

It's used by HTMLEditor too. So, we should move it into EditorBase.

Depends on D115791

It's a common method to cut selection. Therefore, it should be in EditorBase.

Depends on D115792

It's common method of TextEditor and HTMLEditor, but implemented by
TextEditor even though it's an override of nsIEditor's method.

Therefore, it should be implemented in EditorBase instead.

Depends on D115793

It's used with both TextEditor and HTMLEditor instances. So, it should be
implemented in EditorBase instead.

Depends on D115794

TextEditor declares some virtual methods newly. However, for moving some
methods from TextEditor to EditorBase, they should be accessible from
EditorBase. Therefore, this patch adds declarations of abstract virtual
methods of them to EditorBase.

Depends on D115795

Attachment #9223186 - Attachment description: Bug 1540037 - part 28: Add abstract virtual methods to `EditorBase`, which are originated from `TextEditor` r=m_kato! → Bug 1540037 - part 28: Add pure virtual methods to `EditorBase`, which are originated from `TextEditor` r=m_kato!
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c7e4abc8057a
part 16: Move composition event handlers from `TextEditor` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6d3b833e084b
part 17: Move `TextEditor::OnInputText()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9d9084863822
part 18: Move `ReplaceTextAsAction()` and `ReplaceSelectionAsSubAction()` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8f1b67bbc5af
part 19: Make `IsEmpty()` be a virtual method of `EditorBase` and implement `nsIEditor::GetDocumentIsEmpty()` in `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/08b0dccfddf7
part 20: Move `UndoAsAction()` and `RedoAsAction()` from `TextEditor` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/85911d6eabbb
part 21: Move `TextEditor::IsCopyToClipboardAllowed()` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0b48dcb60529
part 22: Move `IsCopyCommandEnabled()` and `IsCutCommandEnabled()` from `TextEditor` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d14c17436d81
part 23: Move `TextEditor::AreClipboardCommandsUnconditionallyEnabled()` into `EditorBase` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0faf5e8f142b
part 24: Move `TextEditor::FireClipboardEvent()` to `EditorBase` r=m_kato
Regressions: 1712811
No longer regressions: 1712811

The attribute is used only with HTMLEditor, and it does not make sense to
access document's character-set via TextEditor. Therefore, this patch
makes it implement in HTMLEditor (EditorBase will return
NS_ERROR_NOT_AVAILABLE both getter and setter).

Note that EditorBase::GetDocumentCharsetInternal() is required by
TextEditor::ComputeValueInternal(). Therefore, it needs to stay in
EditorBase.

Depends on D115796

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2d6956046a4e
part 25: Move `TextEditor::CutAsAction()` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1bf9011c27c2
part 26: Get rid of `TextEditor::Copy()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/16ece4ec2890
part 27: Move `TextEditor::CanDeleteSelection()` to `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/adb0d91643e6
part 28: Add pure virtual methods to `EditorBase`, which are originated from `TextEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3c85e6dc75bf
part 29: Move `nsIEditor.documentCharacterSet` definitions into `HTMLEditor` r=m_kato

It's now used only by HTMLEditor::Rewrap() and it does simple thing with
TextEditor::ComputeValueInternal(). So, we can make Rewrap() directly
do it instead.

It's a helper method of TextEditor::ComputeValueInternal() which is used by
TextEditor and HTMLEditor::Rewrap(). So, before we move
ComputeValueInternal(), we need to move this first.

Depends on D116347

It's used both with TextEditor instance and HTMLEditor instance. So, it
should be implemented in EditorBase.

Depends on D116349

In any types of editor, EditorBase handles if it's in the read-only mode.
For making HandleKeyPressEvent relation between classes simpler, let's
handle it in an independent method.

Depends on D116350

Such events shouldn't be fired, but for now, we should make them handled in
the overrides. Then, we can avoid the skipping TextEditor case from
HTMLEditor.

Depends on D116351

Delete and Backspace keys are handled by same code. So, the code should
be in EditorBase instead of TextEditor.

If HTMLEditor is in the plaintext editing mode of mail composer, Tab key
is also handled by the same code as TextEditor. So, the code in TextEditor
should be moved to EditorBase too and HTMLEditor should call EditorBase's
method only when it's in the plaintext mode.

Depends on D116352

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/219a530441fd
part 30: Get rid of `TextEditor::SharedOutputString()` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/99e98ac9c89c
part 31: Move `TextEditor::GetAndIniitDocEncoder()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9d28dcb2d4d4
part 32: Move `TextEditor::ComputeValueInternal()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7ccd13d2a6fd
part 33: Implement `nsIEditor::OutputToString()` in `EditorBase` rather than `TextEditor` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4c550655dda5
part 34: Create `HandleKeyPressEventInReadOnlyMode()` r=m_kato
Summary: Split TextEditor and HTMLEditor → Move `TextEditor` methods which are used with `HTMLEditor` instances for splitting TextEditor and HTMLEditor classes
Status: NEW → ASSIGNED
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e902ab5efc57
part 35: Make `TextEditor` and `HTMLEditor` handle modifier keys' `eKeyPress` event directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a438fd03bdb8
part 36: Make `EditorBase` handle `Delete`, `Backspace` and `Tab` of plaintext editor mode r=m_kato
Regressions: 1713867

It just creates an nsITransferable instance and add 2 flavors for storing
plain text. Therefore, it can be in EditorUtils instead.

Depends on D116566

It should be a virtual method derived from EditorBase, and TextEditor
and HTMLEditor should override it. Then, nsIEditor::Paste() requires
referring vtable again if we keep implementing it only in EditorBase.
Therefore, this patch avoid it with implementing it in both TextEditor
and HTMLEditor.

Depends on D116567

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0e61485ac614
part 37: Move `TextEditor::PrepareToInsertContent()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/52b13b5f2c4d
part 38: Move `TextEditor::InsertTextAt()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7beac38583d0
part 39: Move `TextEditor::IsSafeToInsertData()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f9c0c941ca36
part 40: Move `TextEditor::PrepareTransferable()` into `EditorUtils` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/dcac769feba1
part 41: Split `TextEditor::PasteAsAction()` r=m_kato

TextEditor::OnDrop() handles both cases, in TextEditor and in HTMLEditor
because the common part is too complicated to duplicate. However, most
different part is inserting the dropped items part. So, let's make them
into a virtual method.

In this patch, creating a method only in HTMLEditor and moves the part
into it.

Depends on D116569

This patch makes the previously created handler method to virtual (derived from
EditorBase), and makes TextEditor override it.

Depends on D116801

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7ada44a617ae
part 42: Move `TextEditor::DeleteSelectionByDragAsAction()` into `EditorBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0ca825772b7b
part 43: Move inserting dropped items part on `HTMLEditor` to `HTMLEditor` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0d2966d1b817
part 44: Move the inserting dropped items part of `TextEditor::OnDrop()` to the new virtual method r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f8fa3223188f
part 45: Move `TextEditor::OnDrop()` into `EditorBase` r=m_kato

It's used by TextEditor and HTMLEditor in the plaintext mode. Therefore,
it should be moved into EditorBase.

Severity: normal → N/A

The last 2 patches are the final ones for this bug.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/446c8ee9a6ba
part 46: Move `TextEditor::EnsurePaddingBRElementInMultilineEditor()` into `EditorBase` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4173082d8402
part 47: Move `TextEditor::InitEditorContentAndSelection()` to `EditorBase` r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.