Bug 1796732 - Write first shot of douments of the editor module r=m_kato DONTBUILD

Differential Revision: https://phabricator.services.mozilla.com/D159919

editor/docs/EditorModuleSpecificRules.rst

+############################
+Editor module specific rules
+############################
+
+The editor module has not been maintained aggressively about a decade. Therefore, this module needs
+to be treated as a young module or in a transition period to align the behavior to the other
+browsers and take modern C++ style.
+
+Undoubtedly, this editor module is under rewritten for modern and optimized for current specs.
+Additionally, this module does really complicated things which may cause security issues.
+Therefore, there are specific rules:
+
+Treat other browsers behavior as standard if the behavior is reasonable
+=======================================================================
+
+The editing behavior is not standardized since as you see too many lines in the editor classes, the
+number of cases which need to handle edge cases is crazy and that makes it impossible to standardize.
+Additionally, our editor behavior is not so stable. Some behaviors were aligned to Internet Explorer,
+some other behaviors were not for making "better" UX for users of email composer and HTML composer
+which were in SeaMonkey, and the other browser engines (Blink and WebKit) have same roots but the
+behavior is different from both IE and Gecko.
+
+Therefore, there were no reference behavior.
+
+In these days, compatibility between browsers becomes more important, and fortunately, the behavior
+of Blink (Chrome/Chromium) which has the biggest market share is more reasonable than ours in a lot
+of cases. Therefore, if we get web-compat issue reports, we should align the behavior to Blink in
+theory.
+
+However, if Blink's behavior is also odd, this is the worst case. In this case, we should try to
+align the behavior to WebKit if and only if WebKit's behavior is different from Blink and
+reasonable, or doing something "better" for hiding the issue from web-apps and file an issue to the
+Editing Working Group with creating a "tentative" web-platform test.
+
+Don't make methods of editor classes public if they are used only by helper classes
+===================================================================================
+
+Although this is a silly rule. Of course, APIs of editor classes need to be public for the other
+modules. However, the other methods which are used only by helper classes in the editor module --the
+methods may be crashed if called by the other modules because editor classes store and guarantee the
+colleagues (e.g., ``Selection``) when it starts to handle an edit action (edit command or
+operation)--  does not want to do it for the performance reason. Therefore, such methods are now
+declared as protected methods and the caller classes are registered as friends.
+
+For solving this issue, we could split the editor classes one is exported and the other is not
+exposed, and make the former to proxies and own the latter.  However, this approach might cause
+performance regressions and requires a lot of line changes (at least each method definition and
+warning messages at the caller sides).  Tracked in
+`bug 1555916 <https://bugzilla.mozilla.org/show_bug.cgi?id=1555916>`__.
+
+Steps to handle one editor command or operation
+===============================================
+
+One edit command or operation is called "edit action" in the editor module.  Handling it starts
+when an XPCOM method or a public method which is named as ``*AsAction``. Those methods create
+``AutoEditActionDataSetter`` in the stack first, then, call one of ``CanHandle()``,
+``CanHandleAndMaybeDispatchBeforeInputEvent()`` or ``CanHandleAndFlushPendingNotifications()``.
+If ``CanHandleAndMaybeDispatchBeforeInputEvent()`` causes dispatching ``beforeinput`` event and if
+the event is consumed by the web app, it returns ``NS_ERROR_EDITOR_ACTION_CANCELED``. In this case,
+the method can do anything due to the ``beforeinput`` event definition.
+
+At this time, ``AutoEditActionDataSetter`` stores ``Selection`` etc which are required for handling
+the edit action in it and set ``EditorBase::mEditActionData`` to its address. Then all methods of
+editor can access the objects via the pointer (typically wrapped in inline methods) and the lifetime
+of the objects are guaranteed.
+
+Then, the methods call one or more edit-sub action handlers.  E.g., when user types a character
+with a non-collapsed selection range, editor needs to delete the selected content first and insert
+the character there. For implementing this behavior, "insert text" edit action handler needs to call
+"delete selection" sub-action handler and "insert text" sub-action handler. The sub-edit action
+handlers are named as ``*AsSubAction``.
+
+The callers of edit sub-action handlers or the handlers themselves create ``AutoPlaceholderBatch``
+in the stack. This creates a placeholder transaction to make all transactions undoable with one
+"undo" command.
+
+Then, each edit sub-action handler creates ``AutoEditSubActionNotifier`` in the stack and if it's
+the topmost edit sub-action handling, ``OnStartToHandleTopLevelEditSubAction()`` is called at the
+creation and ``OnEndHandlingTopLevelEditSubAction()`` is called at the destruction. The latter will
+clean up the modified range, e.g., remove unnecessary empty nodes.
+
+Finally, the edit sub-actions does something while ``AutoEditSubActionNotifier`` is alive. Helper
+methods of edit sub-action handlers are typically named as ``*WithTransaction`` because they are
+done with transaction classes for making everything undoable.
+
+Don't update Selection immediately
+==================================
+
+Changing the ranges of ``Selection`` is expensive (due ot validating new range, notifying new
+selected or unselected frames, notifying selection listeners, etc), and retrieving current
+``Selection`` ranges at staring to handle something makes the code statefull which is harder to
+debug when you investigate a bug. Therefore, each method should return new caret position or
+update ranges given as in/out parameter of ``AutoRangeArray``.  ``Result<CaretPoint, nsresult>``
+is a good result type for the former, and the latter is useful style if the method needs to keep
+``Selection`` similar to given ranges, e.g., when paragraphs around selection are changed to
+different type of blocks. Finally, edit sub-action handler methods should update ``Selection``
+before destroying ``AutoEditSubActionNotifier`` whose post-processing requires ``Selection``.
+
+Don't add new things into OnEndHandlingTopLevelEditSubAction()
+==============================================================
+
+When the topmost edit sub-action is handled, ``OnEndHandlingTopLevelEditSubAction`` is called and
+it cleans up something in (or around) the modified range. However, this "post-processing" approach
+makes it harder to change the behavior for fixing web-compat issues. For example, it deletes empty
+nodes in the range, but if only some empty nodes are inserted intentionally, it doesn't have the
+details and may unexpectedly delete necessary empty nodes.
+
+Instead, new things should be done immediately at or after modifying the DOM tree, and if it
+requires to disable the post-processing, add new ``bool`` flag to
+``EditorBase::TopLevelEditSubActionData`` and when it's set, make
+``OnEndHandlingTopLevelEditSubAction`` stop doing something.
+
+Don't use NS_WARN_IF for checking NS_FAILED, isErr() and Failed()
+=================================================================
+
+The warning messages like ``NS_FAILED(rv)`` does not help except the line number, and in the cases
+of that we get web-compat reports, somewhere in the editor modules may get unexpected result. For
+saving the investigation time of web-compat issues, each failure should warn which method call
+failed, for example:
+
+.. code:: cpp
+
+  nsresult rv = DoSomething();
+  if (NS_FAILED(rv)) {
+    NS_WARNING("HTMLEditor::DoSomething() failed");
+    return rv;
+  }
+
+These warnings will let you know the stack of failure in debug build. In other words, when you
+investigate a web-compat issue in editor, you should do the steps to reproduce in debug build first.
+Then, you'd see failure point stack in the terminal.
+
+Return NS_ERROR_EDITOR_DESTROYED when editor gets destroyed
+===========================================================
+
+The most critical error while an editor class method is running is what the editor instance is
+destroyed by the web app. This can be checked with a call of ``EditorBase::Destroyed()`` and
+if it returns ``true``, methods should return ``NS_ERROR_EDITOR_DESTROYED`` with stopping handling
+anything. Then, all callers which handle the error result properly will stop handling too.
+Finally, public methods should return ``EditorBase::ToGenericNSResult(rv)`` instead of exposing
+an internal error of the editor module.
+
+Note that destroying the editor is intentional thing for the web app. Thus we should not throw
+exception for this failure reason. Therefore, the public methods shouldn't return error.
+
+When you make a method return ``NS_ERROR_EDITOR_DESTROYED`` properly, you should mark the method
+as ``[[nodiscard]]``. In other words, if you see ``[[nodiscard]]`` in method definition and it
+returns ``nsresult`` or ``Result<*, nsresult>``, the method callers do not need to check
+``Destroyed()`` by themselves.
+
+Use reference instead of pointer as far as possible
+===================================================
+
+When you create or redesign a method, it should take references instead of pointers if they take.
+This rule forces that the caller to do null-check and this avoids a maybe unexpected case like:
+
+.. code:: cpp
+
+  inline bool IsBRElement(const nsINode* aNode) {
+    return aNode && aNode->IsHTMLElement(nsGkAtoms::br);
+  }
+
+  void DoSomethingExceptIfBRElement(const nsINode* aNode) {
+    if (IsBRElement(aNode)) {
+      return;
+    }
+    // Do something for non-BR element node.
+  }
+
+In this case, ``DoSomethingExceptIfBRElement`` expects that ``aNode`` is never ``nullptr`` but it
+could be at least in build time. Using reference fixes this mistake at build time.
+
+Use ``EditorUtils`` or ``HTMLEditUtils`` for stateless methods
+==============================================================
+
+When you create a new static method to the editor classes or a new inline method in cpp file which
+defines the editor classes, please check if it's a common method which may be used from other
+places in the editor module.  If it's possible to be used only in ``HTMLEditor`` or its helper
+classes, the method should be in ``HTMLEditUtils``.  If it's possible be used in ``EditorBase`` or
+``TextEditor`` or their helper classes, it should be in ``EditorUtils``.
+
+Don't use bool argument
+=======================
+
+If you create a new method which take one or more ``bool`` arguments, use ``enum class`` instead
+since ``true`` or ``false`` in the caller side is not easy to read. For example, you must not
+be able to understand what this example mean:
+
+.. code:: cpp
+
+  if (IsEmpty(aNode, true)) {
+
+For avoiding this issue, you should create new ``enum class`` for each.  E.g.,
+
+.. code:: cpp
+
+  if (IsEmpty(aNode, TreatSingleBR::AsVisible)) {
+
+Basically, both ``enum class`` name and its value names explains what it means fluently. However, if
+it's impossible, use ``No`` and ``Yes`` for the value like:
+
+.. code:: cpp
+
+  if (DoSomething(aNode, OnlyIfEmpty::Yes)) {
+
+Don't use out parameters
+========================
+
+In most cases, editor methods meet error of low level APIs, thus editor methods usually return error
+code. On the other hand, a lot of code need to return computed things, e.g., new caret position,
+whether it's handled, ignored or canceled, a target node looked for, etc. We used ``nsresult`` for
+the return value type and out parameters for the other results, but it makes callers scattering a
+lot of auto variables and reusing them makes the code harder to understand.
+
+Now we can use ``mozilla::Result<Foo, nsresult>`` instead.

editor/docs/EditorModuleStructure.rst

+#######################
+Editor module structure
+#######################
+
+This document explains the structure of the editor module and overview of classes.
+
+Introduction
+============
+
+This module implements the builtin editors of editable elements or documents, and this does **not**
+implement the interface with DOM API and visual feedback of the editing UI. In other words, this
+module implements DOM tree editors.
+
+Directories
+===========
+
+composer
+--------
+
+Previously, this directory contained "Composer" UI related code.  However, currently, this
+directory contains ``nsEditingSession`` and ``ComposerCommandsUpdater``.
+
+libeditor
+---------
+
+This is the main directory which contains "core" implementation of editors.
+
+spellchecker
+------------
+
+Despite of the directory name, implementation of the spellchecker is **not** here. This directory
+contains only a bridge between editor classes and the spellchecker and serialized text of editable
+content for spellchecking.
+
+txmgr
+-----
+
+This directory contains transaction items and transaction classes. They were designed for generic
+use cases, e.g., managing undo/redo of bookmarks/history of browser, etc, but they are used only by
+the editor.
+
+Main classes
+============
+
+EditorBase
+----------
+
+``EditorBase`` class is an abstract class of editors.  This inherits ``nsIEditor`` XPCOM interface,
+implement common features which work with instance of classes, and exposed by
+``mozilla/EditorBase.h``.
+
+TextEditor
+----------
+
+``TextEditor`` class is the implementation of plaintext editor which works with ``<input>`` and
+``<textarea>``.  Its exposed root is the host HTML elements, however, the editable root is an
+anonymous ``<div>`` created in a native anonymous subtree under the exposed root elements. This
+creates a ``Text`` node as the first child of the anonymous ``<div>`` and modify its data. If the text
+data ends with a line-break, i.e., the last line is empty, append a ``<br>`` element for making the
+empty last line visible.
+
+This also implements password editor. It works almost same as normal text editor, but each character
+may be masked by masked character such as "●" or "*" by the layout module for the privacy.
+Therefore, this manages masked/unmasked range of password and maybe making typed character
+automatically after a while for mobile devices.
+
+This is exposed with ``mozilla/TextEditor.h``.
+
+Selection in TextEditor
+^^^^^^^^^^^^^^^^^^^^^^^
+
+Independent ``Selection`` and ``nsFrameSelection`` per ``<input>`` or ``<textarea>``.
+
+Lifetime of TextEditor
+^^^^^^^^^^^^^^^^^^^^^^
+
+Created when an editable ``<textarea>`` is created or a text-editable ``<input>`` element gets focus.
+Note that the initialization may run asynchronously if it's requested when it's not safe to run
+script. Destroyed when the element becomes invisible. Note that ``TextEditor`` is recreated when
+every reframe of the host element.  This means that when the size of ``<input>`` or ``<textarea>``
+is changed for example, ``TextEditor`` is recreated and forget undo/redo transactions, but takes
+over the value, selection ranges and composition of IME from the previous instance.
+
+HTMLEditor
+----------
+
+``HTMLEditor`` class is the implementation of rich text editor which works with ``contenteditable``,
+``Document.designMode`` and XUL ``<editor>``. Its instance is created per document even if the
+document has multiple elements having ``contenteditable`` attribute. Therefore, undo/redo
+transactions are shared in all editable regions.
+
+This is exposed with ``mozilla/HTMLEditor.h``.
+
+Selection in HTMLEditor
+^^^^^^^^^^^^^^^^^^^^^^^
+
+The instance for the ``Document`` and ``Window``. When an editable element gets focus, ``HTMLEditor``
+sets the ancestor limit of ``Selection`` to the focused element or the ``<body>`` of the ``Document``.
+Then, ``Selection`` cannot cross boundary of the limiter element.
+
+Lifetime of HTMLEditor
+^^^^^^^^^^^^^^^^^^^^^^
+
+Created when first editable region is created in the ``Document``.  Destroyed when last editable
+region becomes non-editable.
+
+Currently, even while ``HTMLEditor`` is handling an edit command/operation (called edit action in
+editor classes), each DOM mutation can be tracked with legacy DOM mutation events synchronously.
+Thus, after changing the DOM tree from ``HTMLEditor``, any state could occur, e.g., the editor
+itself may have been destroyed, the DOM tree have been modified, the ``Selection`` have been
+modified, etc. This issue is tracked in
+`bug 1710784 <https://bugzilla.mozilla.org/show_bug.cgi?id=1710784>`__.
+
+
+EditorUtils
+-----------
+
+This class has only static utility methods which are used by ``EditorBase`` or ``TextEditor`` and
+may be used by ``HTMLEditor`` too.  I.e., the utility methods which are used **not** only by
+``HTMLEditor`` should be implemented in this class.
+
+Typically, sateless methods should be implemented as ``static`` methods of utility classes because
+editor classes have too many methods and fields.
+
+This class is not exposed.
+
+HTMLEditUtils
+-------------
+
+This class has only static utility methods which are used only by ``HTMLEditor``.
+
+This class is not exposed.
+
+AutoRangeArray
+--------------
+
+This class is a stack only class and intended to copy of normal selection ranges.  In the new code,
+`Selection` shouldn't be referred directly, instead, methods should take reference to this instance
+and modify it.  Finally, root caller should apply the ranges to `Selection`.  Then, `HTMLEditor`
+does not need to take care of unexpected `Selection` updates by legacy DOM mutation event listeners.
+
+This class is not exposed.
+
+EditorDOMPoint, EditorRawDOMPoint, EditorDOMPointInText, EditorRawDOMPointInText
+--------------------------------------------------------------------------------
+
+It represents a point in a DOM tree with one of the following:
+
+* Container node and offset in it
+* Container node and child node in it
+* Container node and both offset and child node in it
+
+In most cases, instances are initialized with a container and only offset or child node.  Then,
+when ``Offset()`` or ``GetChild()`` is called, the last one is "fixed".  After inserting new child
+node before the offset and/or the child node, ``IsSetAndValid()`` will return ``false`` since the
+child node is not the child at the offset.
+
+If you want to keep using after modifying the DOM tree, you can make the instance forget offset or
+child node with ``AutoEditorDOMPointChildInvalidator`` and ``AutoEditorDOMRangeChildrenInvalidator``.
+The reason why the forgetting methods are not simply exposed is, ``Offset()`` and ``GetChild()``
+are available even after the DOM tree is modified to get the cached offset and child node,
+additionally, which method may modify the DOM tree may be not clear for developers.  Therefore,
+creating a block only for these helper classes makes the updating point clearer.
+
+These classes are exposed with ``mozilla/EditorDOMPoint.h``.
+
+EditorDOMRange, EditorRawDOMRange, EditorDOMRangeInTexts, EditorRawDOMRangeInTexts
+----------------------------------------------------------------------------------
+
+It represents 2 points in a DOM tree with 2 ``Editor*DOMPoint(InText)``.  Different from ``nsRange``,
+the instances do not track the DOM tree changes. Therefore, the initialization is much faster than
+``nsRange`` and can be in the stack.
+
+These classes are exposed with ``mozilla/EditorDOMPoint.h``.
+
+AutoTrackDOMPoint, AutoTrackDOMRange
+------------------------------------
+
+These methods updates ``Editor*DOMPoint(InText)`` or ``Editor*DOMRange(InTexts)`` at destruction
+with applying the changes caused by the editor instance.  In other words, they don't track the DOM
+tree changes by the web apps like changes from legacy DOM mutation event listeners.
+
+These classes are currently exposed with ``mozilla/SelectionState.h``, but we should stop exposing
+them.
+
+WSRunScanner
+------------
+
+A helper class of ``HTMLEditor``. This class scans previous or (inclusive) next visible thing from
+a DOM point or a DOM node. This is typically useful for considering whether a `<br>` is visible or
+invisible due to near a block element boundary, finding nearest editable character from caret
+position, etc.  However, the running cost is **not** cheap, thus if you find another way to consider
+it simpler, use it instead, and also this does not check the actual style of the nodes (visible vs.
+invisible, block vs. inline), thus you'd get unexpected result in tricky cases.
+
+This class is not exposed.
+
+WhiteSpaceVisibilityKeeper
+--------------------------
+
+A helper class of ``HTMLEditor`` to handle collapsible white-spaces as what user expected. This
+class currently handles white-space normalization (e.g., when user inputs multiple collapsible
+white-spaces, this replaces some of them to NBSPs), but the behavior is different from the other
+browsers. We should re-implement this with emulating the other browsers' behavior as far as possible,
+but currently it's put off due to not affecting UX (tracked in
+`bug 1658699 <https://bugzilla.mozilla.org/show_bug.cgi?id=1658699>`__.
+
+This class is not exposed.
+
+\*Transaction
+-------------
+
+``*Transaction`` classes represents a small transaction of updating the DOM tree and implements
+"do", "undo" and "redo" of the update.
+
+Note that each class instance is created too many (one edit action may cause multiple transactions).
+Therefore, each instance must be smaller as far as possible, and if you have an idea to collapse
+multiple instances to one instance, you should fix it. Then, users can run Firefox with smaller
+memory devices especially if the transaction is used in ``TextEditor``.

editor/docs/index.rst

@@ -7,4 +7,6 @@ editor. The documents live in editor/docs/.
 .. toctree::
    :maxdepth: 1
 
+   EditorModuleStructure
+   EditorModuleSpecificRules
    IMEHandlingGuide