editor/docs/EditorModuleSpecificRules.rst

   1 ############################
   2 Editor module specific rules
   3 ############################
   4
   5 The editor module has not been maintained aggressively about a decade. Therefore, this module needs
   6 to be treated as a young module or in a transition period to align the behavior to the other
   7 browsers and take modern C++ style.
   8
   9 Undoubtedly, this editor module is under rewritten for modern and optimized for current specs.
  10 Additionally, this module does really complicated things which may cause security issues.
  11 Therefore, there are specific rules:
  12
  13 Treat other browsers behavior as standard if the behavior is reasonable
  14 =======================================================================
  15
  16 The editing behavior is not standardized since as you see too many lines in the editor classes, the
  17 number of cases which need to handle edge cases is crazy and that makes it impossible to standardize.
  18 Additionally, our editor behavior is not so stable. Some behaviors were aligned to Internet Explorer,
  19 some other behaviors were not for making "better" UX for users of email composer and HTML composer
  20 which were in SeaMonkey, and the other browser engines (Blink and WebKit) have same roots but the
  21 behavior is different from both IE and Gecko.
  22
  23 Therefore, there were no reference behavior.
  24
  25 In these days, compatibility between browsers becomes more important, and fortunately, the behavior
  26 of Blink (Chrome/Chromium) which has the biggest market share is more reasonable than ours in a lot
  27 of cases. Therefore, if we get web-compat issue reports, we should align the behavior to Blink in
  28 theory.
  29
  30 However, if Blink's behavior is also odd, this is the worst case. In this case, we should try to
  31 align the behavior to WebKit if and only if WebKit's behavior is different from Blink and
  32 reasonable, or doing something "better" for hiding the issue from web-apps and file an issue to the
  33 Editing Working Group with creating a "tentative" web-platform test.
  34
  35 Don't make methods of editor classes public if they are used only by helper classes
  36 ===================================================================================
  37
  38 Although this is a silly rule. Of course, APIs of editor classes need to be public for the other
  39 modules. However, the other methods which are used only by helper classes in the editor module --the
  40 methods may be crashed if called by the other modules because editor classes store and guarantee the
  41 colleagues (e.g., ``Selection``) when it starts to handle an edit action (edit command or
  42 operation)--  does not want to do it for the performance reason. Therefore, such methods are now
  43 declared as protected methods and the caller classes are registered as friends.
  44
  45 For solving this issue, we could split the editor classes one is exported and the other is not
  46 exposed, and make the former to proxies and own the latter.  However, this approach might cause
  47 performance regressions and requires a lot of line changes (at least each method definition and
  48 warning messages at the caller sides).  Tracked in
  49 `bug 1555916 <https://bugzilla.mozilla.org/show_bug.cgi?id=1555916>`__.
  50
  51 Steps to handle one editor command or operation
  52 ===============================================
  53
  54 One edit command or operation is called "edit action" in the editor module.  Handling it starts
  55 when an XPCOM method or a public method which is named as ``*AsAction``. Those methods create
  56 ``AutoEditActionDataSetter`` in the stack first, then, call one of ``CanHandle()``,
  57 ``CanHandleAndMaybeDispatchBeforeInputEvent()`` or ``CanHandleAndFlushPendingNotifications()``.
  58 If ``CanHandleAndMaybeDispatchBeforeInputEvent()`` causes dispatching ``beforeinput`` event and if
  59 the event is consumed by the web app, it returns ``NS_ERROR_EDITOR_ACTION_CANCELED``. In this case,
  60 the method can do anything due to the ``beforeinput`` event definition.
  61
  62 At this time, ``AutoEditActionDataSetter`` stores ``Selection`` etc which are required for handling
  63 the edit action in it and set ``EditorBase::mEditActionData`` to its address. Then all methods of
  64 editor can access the objects via the pointer (typically wrapped in inline methods) and the lifetime
  65 of the objects are guaranteed.
  66
  67 Then, the methods call one or more edit-sub action handlers.  E.g., when user types a character
  68 with a non-collapsed selection range, editor needs to delete the selected content first and insert
  69 the character there. For implementing this behavior, "insert text" edit action handler needs to call
  70 "delete selection" sub-action handler and "insert text" sub-action handler. The sub-edit action
  71 handlers are named as ``*AsSubAction``.
  72
  73 The callers of edit sub-action handlers or the handlers themselves create ``AutoPlaceholderBatch``
  74 in the stack. This creates a placeholder transaction to make all transactions undoable with one
  75 "undo" command.
  76
  77 Then, each edit sub-action handler creates ``AutoEditSubActionNotifier`` in the stack and if it's
  78 the topmost edit sub-action handling, ``OnStartToHandleTopLevelEditSubAction()`` is called at the
  79 creation and ``OnEndHandlingTopLevelEditSubAction()`` is called at the destruction. The latter will
  80 clean up the modified range, e.g., remove unnecessary empty nodes.
  81
  82 Finally, the edit sub-actions does something while ``AutoEditSubActionNotifier`` is alive. Helper
  83 methods of edit sub-action handlers are typically named as ``*WithTransaction`` because they are
  84 done with transaction classes for making everything undoable.
  85
  86 Don't update Selection immediately
  87 ==================================
  88
  89 Changing the ranges of ``Selection`` is expensive (due ot validating new range, notifying new
  90 selected or unselected frames, notifying selection listeners, etc), and retrieving current
  91 ``Selection`` ranges at staring to handle something makes the code statefull which is harder to
  92 debug when you investigate a bug. Therefore, each method should return new caret position or
  93 update ranges given as in/out parameter of ``AutoRangeArray``.  ``Result<CaretPoint, nsresult>``
  94 is a good result type for the former, and the latter is useful style if the method needs to keep
  95 ``Selection`` similar to given ranges, e.g., when paragraphs around selection are changed to
  96 different type of blocks. Finally, edit sub-action handler methods should update ``Selection``
  97 before destroying ``AutoEditSubActionNotifier`` whose post-processing requires ``Selection``.
  98
  99 Don't add new things into OnEndHandlingTopLevelEditSubAction()
 100 ==============================================================
 101
 102 When the topmost edit sub-action is handled, ``OnEndHandlingTopLevelEditSubAction`` is called and
 103 it cleans up something in (or around) the modified range. However, this "post-processing" approach
 104 makes it harder to change the behavior for fixing web-compat issues. For example, it deletes empty
 105 nodes in the range, but if only some empty nodes are inserted intentionally, it doesn't have the
 106 details and may unexpectedly delete necessary empty nodes.
 107
 108 Instead, new things should be done immediately at or after modifying the DOM tree, and if it
 109 requires to disable the post-processing, add new ``bool`` flag to
 110 ``EditorBase::TopLevelEditSubActionData`` and when it's set, make
 111 ``OnEndHandlingTopLevelEditSubAction`` stop doing something.
 112
 113 Don't use NS_WARN_IF for checking NS_FAILED, isErr() and Failed()
 114 =================================================================
 115
 116 The warning messages like ``NS_FAILED(rv)`` does not help except the line number, and in the cases
 117 of that we get web-compat reports, somewhere in the editor modules may get unexpected result. For
 118 saving the investigation time of web-compat issues, each failure should warn which method call
 119 failed, for example:
 120
 121 .. code:: cpp
 122
 123   nsresult rv = DoSomething();
 124   if (NS_FAILED(rv)) {
 125     NS_WARNING("HTMLEditor::DoSomething() failed");
 126     return rv;
 127   }
 128
 129 These warnings will let you know the stack of failure in debug build. In other words, when you
 130 investigate a web-compat issue in editor, you should do the steps to reproduce in debug build first.
 131 Then, you'd see failure point stack in the terminal.
 132
 133 Return NS_ERROR_EDITOR_DESTROYED when editor gets destroyed
 134 ===========================================================
 135
 136 The most critical error while an editor class method is running is what the editor instance is
 137 destroyed by the web app. This can be checked with a call of ``EditorBase::Destroyed()`` and
 138 if it returns ``true``, methods should return ``NS_ERROR_EDITOR_DESTROYED`` with stopping handling
 139 anything. Then, all callers which handle the error result properly will stop handling too.
 140 Finally, public methods should return ``EditorBase::ToGenericNSResult(rv)`` instead of exposing
 141 an internal error of the editor module.
 142
 143 Note that destroying the editor is intentional thing for the web app. Thus we should not throw
 144 exception for this failure reason. Therefore, the public methods shouldn't return error.
 145
 146 When you make a method return ``NS_ERROR_EDITOR_DESTROYED`` properly, you should mark the method
 147 as ``[[nodiscard]]``. In other words, if you see ``[[nodiscard]]`` in method definition and it
 148 returns ``nsresult`` or ``Result<*, nsresult>``, the method callers do not need to check
 149 ``Destroyed()`` by themselves.
 150
 151 Use reference instead of pointer as far as possible
 152 ===================================================
 153
 154 When you create or redesign a method, it should take references instead of pointers if they take.
 155 This rule forces that the caller to do null-check and this avoids a maybe unexpected case like:
 156
 157 .. code:: cpp
 158
 159   inline bool IsBRElement(const nsINode* aNode) {
 160     return aNode && aNode->IsHTMLElement(nsGkAtoms::br);
 161   }
 162
 163   void DoSomethingExceptIfBRElement(const nsINode* aNode) {
 164     if (IsBRElement(aNode)) {
 165       return;
 166     }
 167     // Do something for non-BR element node.
 168   }
 169
 170 In this case, ``DoSomethingExceptIfBRElement`` expects that ``aNode`` is never ``nullptr`` but it
 171 could be at least in build time. Using reference fixes this mistake at build time.
 172
 173 Use ``EditorUtils`` or ``HTMLEditUtils`` for stateless methods
 174 ==============================================================
 175
 176 When you create a new static method to the editor classes or a new inline method in cpp file which
 177 defines the editor classes, please check if it's a common method which may be used from other
 178 places in the editor module.  If it's possible to be used only in ``HTMLEditor`` or its helper
 179 classes, the method should be in ``HTMLEditUtils``.  If it's possible be used in ``EditorBase`` or
 180 ``TextEditor`` or their helper classes, it should be in ``EditorUtils``.
 181
 182 Don't use bool argument
 183 =======================
 184
 185 If you create a new method which take one or more ``bool`` arguments, use ``enum class`` instead
 186 since ``true`` or ``false`` in the caller side is not easy to read. For example, you must not
 187 be able to understand what this example mean:
 188
 189 .. code:: cpp
 190
 191   if (IsEmpty(aNode, true)) {
 192
 193 For avoiding this issue, you should create new ``enum class`` for each.  E.g.,
 194
 195 .. code:: cpp
 196
 197   if (IsEmpty(aNode, TreatSingleBR::AsVisible)) {
 198
 199 Basically, both ``enum class`` name and its value names explains what it means fluently. However, if
 200 it's impossible, use ``No`` and ``Yes`` for the value like:
 201
 202 .. code:: cpp
 203
 204   if (DoSomething(aNode, OnlyIfEmpty::Yes)) {
 205
 206 Don't use out parameters
 207 ========================
 208
 209 In most cases, editor methods meet error of low level APIs, thus editor methods usually return error
 210 code. On the other hand, a lot of code need to return computed things, e.g., new caret position,
 211 whether it's handled, ignored or canceled, a target node looked for, etc. We used ``nsresult`` for
 212 the return value type and out parameters for the other results, but it makes callers scattering a
 213 lot of auto variables and reusing them makes the code harder to understand.
 214
 215 Now we can use ``mozilla::Result<Foo, nsresult>`` instead.