This patch tries to implement Blink-compat white-space normalizer for
`HTMLEditor`.
It's difficult to list up our traditional white-space normalization rules
because `WSRunObject` touches white space sequence only when there is not
acceptable case, e.g., an ASCII white-spaces will be adjacent to another
one, and replaces only unacceptable white-space only. Therefore, whether
white-space sequence may start with either an ASCII white-space or an NBSP.
On the other hand, Blink and WebKit makes white-space sequence always
starts with an NBSP or an ASCII white-space (unfortunately, they behave
differently!). So, for web-compat, we should simulate Blink's behavior
because either behavior is reasonable but Blink have more market share.
This patch simply adds new white-space normalization path for the new one,
and it's switchable with a pref, and still disabled by default.
The other reason why we should do this is, our traditional white-space
normalizer touches the DOM a lot of times per edit action, and the timing
is both before and after touches the DOM tree. Therefore, it's difficult
to compute actual deleting range for `InputEvent.getTargetRanges()` and
touching a lot of times causes running mutation event listeners a lot and
creates a lot of transaction class instances. So, new one have a lot of
merits:
1. Improve web-compat
2. Improve the peformance
3. Improve the security
4. Improve the footprint (but this is now worse then traditional one)
5. Simplify the implementation
The new normalizer is mostly implemented with only 3 `HTMLEditor` methods.
One is `HTMLEditor::DeleteTextAndNormalizeSurroundingWhiteSpaces()`. This is
semi-public method for the edit action handlers. This takes a range with
2 `EditorDOMPoinInText` to delete the range simply. This also replaces
surrounding white-space sequence if necessary. For inserting text case,
this method also handles only white-space normalization when it's called
with collapsed range, i.e., same `EditorDOMPointInText`. This tries to use
`RepaceTextWithTransaction()` as far as possible to reduce creation cost of
transaction classes and the footprint.
Another one is `HTMLEditor::ExtendRangeToDeleteWithNormalizingWhiteSpaces()`.
This tries to extend the given range to normalize surrounding white-spaces.
This is currently not optimized for footprint because this may include
white-spaces which do not need to be replaced. This optimization should be
done before shipping, but for now, enabling `InputEvent.getTargetRanges()` in
Nightly channel is more important. So that it should be done in a follow-up
bug.
The other is `HTMLEditor::GenerateWhitepaceSequence()`. This creates
normalized white-space sequence with surrounding character information.
For keeping this method simple as far as possible, we shouldn't optimize
the range of generation even in follow-ups.
Finally, the white-space sequence is not tested in mochitests, so that we
can enable this new normalizer when we run mochitests under
`editor/libeditor/tests`. However, WPT has some tests. We should keep
them running with current normalizer for checking regression. Instead,
we should enable the pref only for the new WPT added by the previous patch.
Depends on D78655
Differential Revision: https://phabricator.services.mozilla.com/D78656
`HTMLEditor::IsBlockNode()` is a virtual method derived from
`EditorBase::IsBlockNode()`. For the performance, `EditorBase`'s one just
return false for text node and `<br>` element. However, these check cost
is really cheap in these days. Therefore, we can make `TextEditor` also
use rich API.
This patch moves `HTMLEditor::NodeIsBlockStatic()` and
`HTMLEditor::NodeIsInlineStatic()` to `HTMLEditUtils`, and removes the wrapper
of the fommer, `HTMLEditor::IsBlockNode()` and `WSRunScanner::IsBlockNode()`.
Differential Revision: https://phabricator.services.mozilla.com/D70874
--HG--
extra : moz-landing-system : lando
Only `TypeInState` stores last caret position. So, only it can detect the
case caret is moved from outside of `<a href>` element and is now at start
or end of it.
Note that this patch does not assume `<a href>` has an empty text node with
another inline element. If we supported it, the loop would be more complicated
and it's really unrealistic edge case. Therefore, it's reasonable to ignore
the case.
And also this patch works with `ArrowUp`/`ArrowDown`/`PageUp`/`PageDown`
cases. However, I have no idea to reject such cases. But I guess that
it does not so bad behavior for users because caret does not moved in
a text node in `<a href>`.
Depends on D69479
Differential Revision: https://phabricator.services.mozilla.com/D69480
--HG--
extra : moz-landing-system : lando
With adding new type, `EditorDOMPointInText` whose container is
`RefPtr<dom::Text>`, we can replace `WSRunScanner::WSPoint` and make
`WSRunScanner` and `WSRunObject` can use its various API. Then, this
patch adds a lot of `NS_ASSERTION`s which can detect existing bugs.
Note that it may look like redundant that `EditorDOMPointInText::IsChar*()`
requires `EditorDOMPointInText::IsEndOfContainer()` check before that.
However, this makes what the callers check clearer.
Differential Revision: https://phabricator.services.mozilla.com/D64336
--HG--
extra : moz-landing-system : lando
Some methods of `EditorDOMPointBase` assumes the container node type is
`nsINode`. However, it's not good for reuse. Therefore, this patch makes
most methods of them be template methods.
Differential Revision: https://phabricator.services.mozilla.com/D64331
--HG--
extra : moz-landing-system : lando
They are really messy because they take a lot of out parameters, and these
out parameter meaning is really unclear. Therefore, they should return
a stack only class instance which explain the meaning with getter methods.
And also it should store the result node as `nsIContent`.
And also this patch adds static methods for them for their users which don't
need `WSRunScanner` instance.
Differential Revision: https://phabricator.services.mozilla.com/D63613
--HG--
extra : moz-landing-system : lando
Additionally, `WSRunObject::ScrabBlockBoundary()` and
`WSRunObject::PreparetToJoinBlocks()` are used only the it. Therefore, we
can clean them up.
Differential Revision: https://phabricator.services.mozilla.com/D44201
--HG--
extra : moz-landing-system : lando
For avoiding confusion between API of `nsRange` and `StaticRange`, I'd like to
rename `nsRange::CreateRange()` to `nsRange::Create()` because
`StaticRange::CreateStaticRange()` is too long name and
`StaticRange::CreateRange()` sounds odd. This patch renames it and changes
related methods to template methods to avoid runtime cost of temporary
`RawRangeBoundary` instance creation.
Differential Revision: https://phabricator.services.mozilla.com/D35141
--HG--
extra : moz-landing-system : lando
`HTMLEditRules::ApplyBlockStyle()` stores `curBlock` and `newBlock` during its
loop to keep handling from deeper child to ancestor, and may do two things for
a `curNode`. If `curBlock` and/or `newBlock` is moved from expected container
when it sets one of or both of them, this patch check whether mutation event
listeners change the DOM tree. Additionally, this patch also checks whether
`curNode' is moved by mutation event listener at first step of two jobs for it.
Differential Revision: https://phabricator.services.mozilla.com/D32689
--HG--
extra : moz-landing-system : lando
This is a simple mistake of `MOZ_ASSERTION()` in it. When `mParent` is a node
which can have children, `mChild` should be non-`nullptr` or `mOffset` should
/ be set to the end of `mParent`. But when `mParent` is not a container, any
`mOffset` value should be allowed.
Differential Revision: https://phabricator.services.mozilla.com/D30014
--HG--
extra : moz-landing-system : lando
TextEditor::OnDrop() calls TextEditor::InsertFromDataTransfer() or
HTMLEditor::InsertFromDataTransfer() and they are called only by
TextEditor::OnDrop().
TextEditor::InsertFromDataTransfer() calls only TextEditor::InsertTextAt()
and TextEditor::InsertTextAt() calls only TextEditor::InsertTextAsSubAction() if
insertion point is nullptr. Therefore, if the callers sets nullptr, they
should call TextEditor::InsertTextAsSubAction() directly. Then, we can
make TextEditor::InsertTextAt() require non-nullptr insertion point.
HTMLEditor::InsertFromDataTransfer() calls HTMLEditor::InsertObject(),
HTMLEditor::DoInsertHTMLWithContext() or TextEditor::InsertTextAt().
HTMLEditor::InsertObject() calls HTMLEditor::DoInsertHTMLWithContext()
directly or via BlobReader (in this case, calls asynchronously).
Unfortunately both HTMLEditor::InsertObject() and
HTMLEditor::DoInsertHTMLWithContext() are called by
HTMLEditor::InsertFromTransferable() which is paste event handler. Therefore,
we cannot make them require non-nullptr insertion point, though, anyway,
they cannot become simpler even if we could do that.
This patch marks them as MOZ_CAN_RUN_SCRIPT as far as possible and
makes them take |const EditorDOMPoint&| for insertion point.
Differential Revision: https://phabricator.services.mozilla.com/D11283
--HG--
extra : moz-landing-system : lando
HTMLEditor::InsertElementAtSelection() doesn't check if selection is in
native anonymous subtree. Therefore, it could insert element into <input>
element etc.
This patch adds new API to EditorDOMPointBase to compute ancestor point which
is not in native-anonymous subtree and make
HTMLEditor::GetBetterInsertionPointFor() use it to not return point in any
native-anonymous subtrees.
This method is not a virtual call, and also looks nicer.
This patch was mostly generated by a Python script, but I manually
cleaned up the code in a few places where statements didn't need to be
split across multiple lines any more.
MozReview-Commit-ID: 8JExxqSRc59
--HG--
extra : rebase_source : df6330a89e8d65dfe7a6fda0c8cb9f9732302efc
A lot of methods take |const EditorRawDOMPoint&| as their argument. However,
some of them are called with EditorDOMPoint::AsRaw(). This is not good for
performance because:
1. Needs to create temporary instance of EditorRawDOMPoint.
2. EditorRawDOMPoint::AsRaw() may be used by simple mistake.
3. Such methods may call EditorRawDOMPoint::Offset(), however, it's not copied
to the original EditorDOMPoint instance. So, callers may need to compute
offset again.
So, such methods should be changed to template methods if they are not virtual
method and AsRaw() should be gotten rid of for prevent it looking not expensive.
MozReview-Commit-ID: DAqqW4a2EMS
--HG--
extra : rebase_source : 120b920477fe6e7231271411a00994325acdb842
Editor(Raw)DOMPoint::IsEndOfContainer() checks mIsChildInitialized before
referring mChild. However, it may be true even if mParent is not a container
node like a text node. Therefore, if mParent is a text node and
mIsChildInitialized is true, it always returns true (i.e., even if mOffset
isn't same as length of mParent).
This patch makes it check mParent->IsContainerNode() before checking
mIsChildInitialized because after checking mIsChildInitialized, it validates
the relation of the members. So, this keeps the validation simple.
MozReview-Commit-ID: K2XrAZoNv2I
--HG--
extra : rebase_source : cba8190f199ab2e9d99547948ab50f0cc8c8ff88
EditorBase::JoinNodeDeep() returns a DOM point which was start of right node.
Currently, this is set before actually joining the nodes. Therefore, we see
warnings since right node which becomes joined node may have less children
than left node.
This patch also makes the NS_WARNING_ASSERTION to NS_ASSERTION since no tests
hit this. So, we can use it to detect regressions.
MozReview-Commit-ID: 3RmRP588AkF
--HG--
extra : rebase_source : 2cab458ffac566c81eb8d3f47e755ffacb1553d2
This patch only fixes warning, not changing actual behavior of editor.
HTMLEditRules::ReturnInParagraph() splits paragraph *around* given point.
Therefore, from point of view of caller, offset of setting point may be
invalid after HTMLEditRules::ReturnInParagraph() handled the edit action.
In this case, invalidating stored child of the point may cause warning
since offset may be larger than length of its container.
So, if HTMLEditRules::ReturnInParagraph() handles the edit action,
the caller, HTMLEditRules::WillInsertBreak(), should cancel invalidating
the stored child for avoiding unnecessary warning.
MozReview-Commit-ID: DKJlr0Awwlo
--HG--
extra : rebase_source : 8fbefff3b46e55daf8db6342f00de904315d7fcc
EditorDOMPointBase::RewindOffset() treats it as error when offset is
same as length of container. However, this is wrong because offset can
be same as length of its container since it means after the last child
or last character. So, if an instance points end of a container node,
Rewind() always fails unexpectedly.
MozReview-Commit-ID: A9bvsxETDoo
--HG--
extra : rebase_source : f05994f2924adaa29711da3916f431688eaa79b7
When Enter key is pressed at start or end of <a href="foo"> element, we
shouldn't split it (in other words, we shouldn't create empty <a href="foo">
element in new paragraph) because users must not want to keep editing *same*
link in new paragraph in most cases.
This patch adjusts HTMLEditRules::ReturnInParagraph() selection start point
locally when it gets selection. If caret is at start of an <a href="foo">
element, moves caret to before the element. If caret is at end of an
<a href="foo"> element, moves to after the element.
MozReview-Commit-ID: 3L3eDzc6Dk
--HG--
extra : rebase_source : dfb22486d2709e84ef7ec1eee3a9bfdef11cbe99
Different from RangeBoundaryBase, EditorDOMPointBase can store only child node.
I.e., mOffset may be invalid until its Offset() is called. So,
GetChildAtOffset() isn't good name. It should be just GetChild().
Similarly, GetNextSiblingOfChildAtOffset() and
GetPreviousSiblingOfChildAtOffset() should be renamed to GetNextSiblingOfChild()
and GetPreviousSiblingOfChild().
MozReview-Commit-ID: WpkPmDwkrL
--HG--
extra : rebase_source : 2be1fcbee129f4c96e9b166be5a924845340708f
EditorDOMPointBase::Container() may return nullptr. So, it should be renamed
to GetContainer().
Then, the method name becomes longer and a lot of callers access the result
directly. So, there should be following methods to make the callers shorter:
- GetContainerAsContent() to return the container as nsIContent*.
- GetContainerAsElement() to return the container as dom::Element*.
- GetContainerAsText() to return the container as dom::Text.
- GetContainerAsDOMNode() to return the container as nsIDOMNode*.
- CanContainerHaveChildren() to check if the container can have child nodes.
- IsInDataNode() to check if the point is in a data node including text node.
- IsInTextNode() to check if the point is in a text node.
- IsContainerHTMLElement() to check if the container is specific HTML element.
- IsContainerAnyOfHTMLElements() to check if the container is one of the
specified HTML elements.
MozReview-Commit-ID: LkN2WBbCPj0
--HG--
extra : rebase_source : 9da1e70d4c74f7ad573e244cf6c7b21a0c7b4410
Editor code sometimes sets a DOM point to end of a node. In this case, we
need to write |Set(node, node->Length())|. So, it should have
|void SetToEndOf(const nsINode* aContainer)| for making meaning of the code
clearer.
MozReview-Commit-ID: 91shMCD2d84
--HG--
extra : rebase_source : 3401af50aabc1b677deeae664e8c56506182d28c
HTMLEditor::NormalizeEOLInsertPosition() takes a set of container node and
offset in it for specifying insertion point. So, this should be replaced
with |const EditorRawDOMPoint&| and it should return |EditorDOMPoint| rather
than modifying the argument.
Additionally, perhaps, GetBetterInsertionPointFor() is better name for it.
MozReview-Commit-ID: IB1FhrkzK2G
--HG--
extra : rebase_source : 55f8c1d9fa1f149e81114c929f6e6232aba03905
HTMLEditor::InsertNodeAtPoint() should take |const EditorRawDOMPoint&| as
an argument which specifies point to insert. Additionally, it should take
|EditorDOMPoint*| to return the next point of actual insertion point.
Additionally, this patch renames it to InsertNodeAtProperAncestor() for
explaining what it will do.
MozReview-Commit-ID: HYUzSlyPxAd
--HG--
extra : rebase_source : 57033cdc4458b45a5fe9d6aa642c185133a79304
EditorDOMPoint was a subclass of RangeBoundary. Therefore, for heavy
use of it in editor, we've implemented a lot of complicated feature
into RangeBoundary. However, EditorDOMPoint stopped inheriting
RangeBoundary. So, we can get simple RangeBoundary implementation
back now.
This patch makes RangeBoundary.h almost same as 3f7cbec2446b except
keeps implementing GetPreviousSiblingOfChildAtOffset() and
GetNextSiblingOfChildAtOffset() because the former is used by
Selection and both of them are simple. And also keeps making it
a friend of EditorDOMPoint because EditorDOMPoint still needs to
copy to/from RangeBoundary.
MozReview-Commit-ID: Hr5SA52ScK0
--HG--
extra : rebase_source : a00a46ff931a5c4fcabf910510fa448b513f50c5
While moving children of a container to another container, mutation observer
may move children before moving and/or move node immediately after the
insertion point. Therefore, EditorBase should store all children which
should be moved with a local variable. Then, move one by one carefully.
E.g., if a child before being moved is moved to different container, it
shouldn't be moved because JS already handles it as expected for the web
app. If next sibling of the insertion point has been moved, EditorBase
should stop moving the remaining children because it may move children to
odd position and it may cause dataloss.
This patch creates EditorBase::MoveChildren() and it moves children carefully
with above checks. Additionally, making its callers simpler, this patch also
creates EditorBase::MovePreviousSiblings() and MoveAllChildren().
MozReview-Commit-ID: FJDdSqXIlqD
--HG--
extra : rebase_source : 54bded29dbbd9547339a2c6e1a1264e22fbdc740
There are 3 spam warning assertion in EditorDOMPointBase.
One is in constructor. Which warns if IsSet() returns false, but it checks
before finishing initializing.
The others are in IsStartOfContainer() and IsEndOfContainer(). They try to
check if mOffset is valid value with current DOM tree if it's initialized
when it's at start or end of the container. However, they do it even when
it's not start nor end of the container.
This patch fixes those spammers.
MozReview-Commit-ID: DvFa501m9V0
--HG--
extra : rebase_source : c97ac371054a54eabc07b3ec726b924d6be61e25
CreateElementTransaction and SplitNodeTransaction store DOM point with
RangeBoundary now. However, it refers next child of referring point.
Therefore, if it's moved to different container or removed from the DOM
tree, they cannot do undo/redo. We can say for the child node, however,
it doesn't make sense depending on a node which is not directly referring.
MozReview-Commit-ID: Baohbub3RNZ
--HG--
extra : rebase_source : 6776420271cdaf91aae6213fb6e1bcf1841112b1
EditorDOMPointBase should store child node at offset directly rather than
previous sibling of child node at offset because if referring child node or
its previous sibling is moved to different DOM point,
EditorDOMPointBase::GetChildAtOffset() may return different node. However,
users of this class assumes that after it stores child node at offset, it'll
return same node.
So, EditorDOMPointBase should store child node instead of "ref". Additionally,
when it stores nullptr as the child, it should set mOffset to length of the
mContainer because it means it refers after the last child.
MozReview-Commit-ID: 7ZdkwKLJEjo
--HG--
extra : rebase_source : c2a0a1be1285466e5fa96162779e86c37ae78655
EditorDOMPointBase should store child at offset for editor uses. E.g., when
editor wants to refer a node as child node of an EditorDOMPoint instance,
even if the node is unexpectedly moved to different container, editor wants
to keep referring the child node rather than its previous sibling.
Therefore this patch makes EditorDOMPointBase not a sub class of
RangeBoundaryBase but copying all methods and members of RangeBoundaryBase
into EditorDOMPointBase for keeping current behavior completely.
MozReview-Commit-ID: LIyPFkCfsZ9
--HG--
extra : rebase_source : fd76c4808625f8f8a86f7b4e4c1ac22fbdc11dd5
In the following patch, we need to invalidate offset a lot of places after
splitting nodes. Therefore, there should be a helper stack class before that.
MozReview-Commit-ID: BgijAU7OizU
--HG--
extra : rebase_source : 520f29dacdffe5f7137ba7f11b289241b5fbface
The only caller of HTMLEditRules::ReturnInParagraph() is
HTMLEditRules::WillInsertBreak(). It should use EditorDOMPoint for storing
selection start.
Then, this patch makes what is set to ReturnInParagraph() clear. So, the
point ReturnInParagraph() is always start of selection.
MozReview-Commit-ID: 6X0P5gpwXKr
--HG--
extra : rebase_source : d1814e5b7c8e4005886607691b37fe6a83e8e5be
EditorBase::InsertTextImpl() takes |nsCOMPtr<nsINode>*|, |nsCOMPtr<nsIContent>*|
and |int32_t| as in/out arguments for container, child and offset of the child
in the container. But this makes the callers really hard to read and ugly.
So, we should make the method take |const EditorRawDOMPoint&| argument as input
and |EditorRawDOMPoint*| as out argument.
MozReview-Commit-ID: 2ijIfGl4Zo7
--HG--
extra : rebase_source : b309d9bdc04aac620f138769ba18ad7e4597fe6c
EditorBase::CreateNode() should take EditorRawDOMPoint as insertion point of
the new element instead of a set of container, child and offset of the child
in the container.
This patch initializes EditorRawDOMPoint with original 3 arguments as far as
possible. If the relation of them are broken, MOZ_ASSERT in RawRangeBoundary
constructor detects existing bugs.
MozReview-Commit-ID: 2N55S6pRv7k
--HG--
extra : rebase_source : 2b14a7715815ca0007635b8f791ca9edbe5b65f1
In some places, editor computes index from child node for collapsing selection
at the child node. However, it's expensive. Therefore, editor should use
Selection::Collapse(const RawRangeBoundary&) as far as possible.
MozReview-Commit-ID: LF2MwASuXzZ
--HG--
extra : rebase_source : b7afc35c0d9d88845391b6f18de57cbff1935ae4
A lot of methods in editor returns a child offset with an out param when it
returns its container and offset in the container. This is ugly hack for
performance of nsINode::IndexOf(). However, there are a lot of regression
since the relation between offset and child node can be broken really easily.
So, we should make EditorDOMPoint as a subclass of RangeBoundary and manage
a set of container, reference child and its offset in it (e.g.,
SetNextSibling() added by this patch).
Note that RangeBoundary's performance is not good for temporary use if we set
a point with offset, it immediately retrieves mRef. The following patch will
improve this performance.
MozReview-Commit-ID: 7mcJ1P1OjVr
--HG--
rename : editor/libeditor/EditorUtils.h => editor/libeditor/EditorDOMPoint.h
extra : rebase_source : 785094fcfc592d9e5b48cbc36ed225dbb8bb4111
Masayuki Nakano
Mirko Brodesser
Makoto Kato
Sylvestre Ledru
Emilio Cobos Álvarez
Boris Zbarsky
Andrew McCreight
Andrea Marchesini