When TextEditor::CreateBRImpl() splits a text node before inserting new <br>
element, it initializes pointToInsertBrNode only with the right text node.
Then, it refers its Offset() after inserting new <br> node before the point.
Therefore, the offset is computed with the new DOM tree. So, adding 1 to
the offset is redundant only in this case.
So, before calling CreateNode(), it needs to make pointToInsertBrNode store
offset with calling its Offset().
Note that this ugly code will be replaced with patches for bug 1408227.
Additionally, this doesn't use AutoEditorDOMPointChildInvalidator because
it's not available in 58 but we need to uplift this patch.
Finally, I'm not sure how to check this in automated tests. Therefore, this
patch doesn't include automated tests.
MozReview-Commit-ID: IaQBonoGawR
--HG--
extra : rebase_source : a89559932f27d98a02baf3e207c6be3c2a545aad
HTMLEditRules::MaybeSplitAncestorsForInsert() may be called with a point in a
text and it needs to return given split point as is. Additionally, the given
point may be in a text node. So, it may not be represented with an
nsCOMPtr<nsIContent>.
Therefore, we need to add new member, EditorDOMPoint, to SplitNodeResult and
when MaybeSplitAncestorsForInsert() needs to return the given point as is,
it should use it.
Note that if the methods which return SplitNodeResult split some nodes actually,
the left node and/or the right node may be removed from the DOM tree. In this
case, EditorDOMPoint cannot store such orphan node. Therefore, we cannot
make mNextNode nor mPreviousNode EditorDOMPoint.
MozReview-Commit-ID: LwH8RZzkrmT
--HG--
extra : rebase_source : a5ae2328bef3d887c0bf4e1b9c4a4247b93a4ac0
Now, we can make HTMLEditRules::SplitAsNeeded() use |SplitNodeResult| as its
result and |const EditorRawDOMPoint&| as specifying start of the deepest right
node. Then, the implementation becomes simpler.
And I think that we should rename it to MaybeSplitAncestorsForInsert().
Additionally, this patch makes it stop calling
EditorBase::IsDescendantOfEditorRoot() and HTMLEditor::GetActiveEditingHost()
because they are really expensive. Instead, it should check if the given start
point of the deepest right node is in active editing host before the loop and
if the loop reaches editing host.
MozReview-Commit-ID: KKpj5uyT2J
--HG--
extra : rebase_source : 0c9e9e9e28505b0fb5752e1cd4d42f64d22af3e7
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
First of all, this patches fixes a bug of EditorBase::CreateNode(). It takes
|EditorRawDOMPoint&| but it should be |const EditorRawDOMPoint&| for making
callers can specify temporary instances.
Next, this patch creates |SplitNodeResult| stack class for result of
EditorBase::SplitNodeDeep(). SplitNodeDeep() needs to return previous node
and next node of split point. They are called as left node and right node,
but these calls are really different term usage. Therefore, this patch names:
aOutLeftNode -> SplitNodeResult::GetPreviousNode()
aOutRightNode -> SplitNodeResult::GetNextNode()
and also declares SplitNodeResult::GetLeftNode() and
SplitNodeResult::GetRightNode() which are same meaning as left/right node of
other splitting methods.
Additionally, of course, this patch makes SplitNodeDeep() use
|const EditorRawDOMPoint&| to receive the start point of right node.
MozReview-Commit-ID: FnJpeKgtzm4
--HG--
extra : rebase_source : 3829e5528ef837b13fed305e0df1dbbf00e02a07
Make the loop in EditorBase::SplitNodeDeep() use EditorDOMPoint for making the
code simpler.
MozReview-Commit-ID: 3do3rWV4eIh
--HG--
extra : rebase_source : 480a5b5b133d8a735bda6ddec07e4edf9ef34035
Now, we can merge two if blocks in the loop of EditorBase::SplitNodeDeep() and
get rid of |didSplit| variable.
MozReview-Commit-ID: LJZHF6x2GLR
--HG--
extra : rebase_source : 5f58508f6fc0928236252670c85383a13200fc2c
This doesn't change any meaning of the loop.
It is a bug if the loop meets orphan node before meeting the most ancestor
node to be split which is given as aNode. So, we can check it before trying
to split it.
MozReview-Commit-ID: 1TD7WHCoZh1
--HG--
extra : rebase_source : 17b8d7b3db458e29fb52be5cedb900560e1b70a4
EmptyContainers::yes and EmptyContainers::no are not so clear name what they
mean.
They means whether NodeSplitDeep() creates or won't create empty nodes when
split point is at start edge or end edge of an element.
This patch renames them to SplitAtEdges::eDoNotCreateEmptyContainer and
SplitAtEdges::eAllowToCreateEmptyContainer and make
HTMLEditor::InsertNodeAtPoint() use it instead of an bool argument.
Additionally, the argument of NodeSplitDeep() is now not optional because
this difference is really important when you read around the callers.
MozReview-Commit-ID: 9hpMRkVDvCg
--HG--
extra : rebase_source : ee892361d66c9c9c5ed45ee9d3321474257ac417
Although, we need to make WSRunObject::PrepareToSplitAcrossBlocks() in
another bug, we should do it now for making HTMLEditRules::ReturnInParagraph()
implementation simpler.
MozReview-Commit-ID: AoMYqAEgCaV
--HG--
extra : rebase_source : cdfb16379f4ecab2a2694aeb283edd111fc81e95
nsIEditActionListner::DidSplitNode() takes 4 arguments, the right node,
old offset in the old right node before splitting, the new left node and
nsresult.
Computing offset for this doesn't make sense because it's always same as
the length of the left node. Additionally, nobody currently use nsersult.
So, we can get rid of it now.
Fortunately, nobody including comm-central and BlueGriffon implements
WillSplitNode(). So, we can get rid of it. However, removing interface
method should be done in a follow up bug. So, we can remove offset computation
in EditorBase::SplitNode() completely in the future.
MozReview-Commit-ID: JWj34SjBNJh
--HG--
extra : rebase_source : f0e1ed0e466dc8217c1a0ab1722790883a7efd1f
EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() takes a set of
container and offset in it for specifying a point to split.
Instead, they should take EditorRawDOMPoint for specifying start of right node.
MozReview-Commit-ID: 5VwS8oudzIT
--HG--
extra : rebase_source : 727948e5cf95f0713019f57ae9a007b85569fa56
Make SplitNodeTransaction stores start of existing right node (which will be
split) instead of split point as a pair of the right node and offset in it.
MozReview-Commit-ID: 2DIpJGSuNaC
--HG--
extra : rebase_source : 13949bdddc30c59462e7fea7fadf29f015ab8d3a
EditorBaseSplitNodeImpl() should be clean up with EditorDOMPoint which should
be an argument to point the first point of right node (existing split node).
MozReview-Commit-ID: DN0yHm9G9yT
--HG--
extra : rebase_source : 256b4e2125e831b7be9e5c4aefc6f04c80e3c1f5
We hit assertion which were added by bug 1414713. That tells us an ancient bug.
There is comment which claims that we should move all list items in the right
list node to left list node if the list nodes should be merged. However, this
has never been done actually since 2002. The code tried to move *some* list
items in the right list node to the list list node. However, retrieving first
list item at an offset almost always failed because the offset variable has
never been initialized.
If we believe the comment, we should move all children of the right list node
to the left list node. However, until we get a testcase to reach this case,
we should keep current behavior, i.e., do nothing, for unexpected regression.
MozReview-Commit-ID: 1r81q1m44oW
--HG--
extra : rebase_source : fc02520f76440d5900fec0517aa6bcd188e1a14f
m-c, c-c and bluegriffon don't use this interface. So we should remove this.
MozReview-Commit-ID: 92VwGKlrOw5
--HG--
extra : rebase_source : 003763c6abc1406bcffabbffc3fe0696985ee97a
No one uses out parameter of InsertBR, so we should remove it. Also, CreateBR
has direction parameter for selection, so we should use it.
MozReview-Commit-ID: 8heqaXpR9He
--HG--
extra : rebase_source : 27b952f080a70fe039c9b43d7ae9f1394d4de01e
ReturnInListItem and ReturnInHeader uses Element as parameter, but
ReturnInParagraph doesn't use Element parameter even if it is Element.
So we should use Element for parameter.
MozReview-Commit-ID: 35JJTETFK45
--HG--
extra : rebase_source : aa101c921d176b7ae7807c461bca1c4a51f9aecc
Now, we know HTMLEditRules::ReturnInParagraph() always splits the parent block
at start of selection. So, it doesn't need to receive the position from the
caller because the cost to get start of selection from first range of Selection
is really cheap.
MozReview-Commit-ID: EvNb6lUBLdt
--HG--
extra : rebase_source : 448fcb4e3a3c6df777825e8747839d6cd9ee2d56
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
|selNode| and |selOffset| in HTMLEditRules::ReturnInParagraph() store the
point to split |aParentDivOrP|. So, they should be |containerAtSplitPoint|
and |offsetAtSplitPoint|.
Then, |newSelNode| can be renamed to |splitAfterNewBR|.
MozReview-Commit-ID: 1DcmLNx1Cff
--HG--
extra : rebase_source : 38e7e1ec68c060da3ea31f694243adc004eeece7
HTMLEditRules::ReturnInParagraph() stores point to insert new <br> element with
a set of |parent|, |node| and |offset|. Their names are really unclear and
they're really messy.
So, let's use new local variable, |EditorRawDOMPoint pointToInsertBR|. This is
set only when new <br> element is necessary. So, we can get rid of
|newBRneeded| too.
MozReview-Commit-ID: EdnhnlokurN
--HG--
extra : rebase_source : d97a985d71a7b4552ccdeedde55f8b7fcb0be590
This cleans up |aSelection| and |aPara| of HTMLEditRules::ReturnInParagraph().
|aSelection| should be reference for avoiding nullptr check.
|aPara| is so too. Additionally, the name is not clear. We should rename it
to |aParentDivOrP| because it's a block parent of the point and has to be
<div> or <p> element.
MozReview-Commit-ID: 8LbKGlrvaIj
--HG--
extra : rebase_source : e0593c916791ec5b39b4138e21b6ce8c680dc4d8
The local variable |sibling| in HTMLEditRules::ReturnInParagraph() is set to
aBRNode of SplitParagraph(). So, the name should be renamed to |brNode|.
Then, the |brNode| should be cleared if it's not a <br> node actually. However,
SplitParagraph() doesn't allow aBRNode to be nullptr. aBRNode is only used to
remove it from the document if it's not necessary. So, it should be able to be
nullptr. Therefore, this patch changes SplitParagraph() too.
Note that SplitParagrah() is called only by ReturnInParagraph(). So, we don't
need to worry about other callers.
MozReview-Commit-ID: 7Ynk9m5F8Mi
--HG--
extra : rebase_source : e2583c70ad274fe74f48df687796ed71a66bdf98
For reducing the number of arguments of HTMLEditRules::ReturnInParagraph(),
let's make it return EditActionResult and get rid of |aCancel| and |aHandled|.
MozReview-Commit-ID: HU1SthEjonn
--HG--
extra : rebase_source : 072865996aaccc63924c34f91014b41b64aa1a16
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
nsIEditActionListener::WillCreateElement() and
nsIEditActionListener::DidCreateElement() are implemented only by m-c.
So, we can remove a set of container node and offset in it from their argument.
Instead, WillCreateElement() should take a node which will be next sibling of
the new node.
Note that only implementation of them is, HTMLEditRules::DidCreateElement().
So, we can get rid of them and can call HTMLEditRules::DidCreateElement()
directly from EditorBase::CreateNode(). However, such change should be done
in another bug which checks all nsIEditActionListener method implementations.
MozReview-Commit-ID: 4LQEs2WwrVC
--HG--
extra : rebase_source : ee1bee1413c578b2873a291c712b8ef46221db0f
The constructor of CreateElementTransaction now takes EditorRawDOMPoint instead
of a set of container node and offset in it. So, its only user,
EditorBase::CreateTxnForCreateElement(), should take EditorRawDOMPoint too.
MozReview-Commit-ID: A8QfPM3LRii
--HG--
extra : rebase_source : 4a99e5cb58230649d19faca788a330fe02eb6bb1
The constructor of CreateElementTransaction should use EditorRawDOMPoint to
receive insertion point of the new element. Then, it should be stored with
RangeBoundary. With this change, CreateElementTransaction doesn't need to
compute child node at insertion point.
Additionally, this creates InsertNode() method. Current code works differently
when DoTransaction() is called and RedoTransaction() is called.
MozReview-Commit-ID: 8ujhmzn65Wg
--HG--
extra : rebase_source : 30cc045a30a3836f211d11e5f70a85804f44a72a
By the fix of bug 1407352, DeleteRangeTransaction::CreateTxnsToDeleteBetween()
starts to use child node instead of a set of container node and offset in it.
However, this is ugly and using RawRangeBoundary may reduce computing offset
more. So, they should use RawRangeBoundary to refer delete points.
MozReview-Commit-ID: pwDHOxpz0E
--HG--
extra : rebase_source : 3871ba4fa3cfb174d5dac57822ae69c40a305323
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
Bug 1402904 added nullptr check for parent node, but I forgot to add this
nullptr check of selected node for outdent command.
So I need to add more nullptr check.
MozReview-Commit-ID: Au9wrG6htk8
--HG--
extra : rebase_source : bd73b1b667b6dc869ebcea3fdfbe1eb3b04a8cde
EditorUtils::IsDescendantOf() current takes a pointer to offset or a pointer to
child content if the caller needs to know the child of the most ancestor node.
However, some callers should get a child as EditorDOMPoint or EditorRawDOMPoint.
Then, they can be used for some editor methods which need to take child node
for performance optimization.
This patch makes EditorUtils::IsDescendantOf() as only two overloads. One takes
pointer to EditorRawDOMPoint as optional out argument. The other takes pointer
to EditorDOMPoint as an out param.
Additionally, this creates new constructor of AutoTrackDOMPoint for making it
can treat EditorDOMPoint directly.
MozReview-Commit-ID: IsAGTUvKI19
--HG--
extra : rebase_source : 97469a21b974c6a1dd515ab472bbc4a88c1899c8
RangeBoundaryBase shouldn't compute mRef when it's initialized with offset.
E.g., some users of the template class may initialize it with a container and
offset in it but it may not refer mRef or may refer after modifying offset.
On the other hand, RangeBoundaryBase::InvalidateOffset() is a special method.
It assumes that mRef is always initialized when it's called but can be
invalidate mOffset until retrieved actually. This is necessary for
nsRange::mStart and nsRange::mEnd since the offset may be changed when
some nodes are inserted before the referring node.
So, now, InvalidateOffset() should be a protected method and make nsRange a
friend class of RangeBoundaryBase. Then, when nsRange sets mStart and/or mEnd,
nsRange itself should guarantee that their mRefs are initialized.
MozReview-Commit-ID: Alr4YkDXIND
--HG--
extra : rebase_source : 7e6828374db7989ae91b9e485571ec553f7435af
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
HTMLEditRules::BustUpInlinesAtRangeEndpoints() tries to split all inline nodes
at range start and range end. It uses EditorBase::SplitNodeDeep() to split
the nodes and HTMLEditRules::GetHighestInlineParent() to retrieve the highest
inline parent of them.
Currently, HTMLEditRules::GetHighestInlineParent() may return editing host or
ancestor of it if active editing host is not block. Then, it may cause
splitting editing host or its parents and following methods of HTMLEditRules
will fail to modify the nodes created outside the editing host.
So, HTMLEditRules::GetHighestInlineParent() should return only one of the
descendants of active editing host.
Unfortunately, even if just adding the test case as a crash test, I cannot
reproduce the crash with automated tests. Therefore, this patch doesn't
include any automated tests.
And this patch changes a crash test, 1402196.html, which expects that an inline
editing host is split by execCommand("insertOrderedList"). However, this patch
fixes this wrong behavior. Therefore, this patch changes the event target of
event listener from <p> inside the editing host to the editing host itself.
MozReview-Commit-ID: 8i5ci1fcrDd
--HG--
extra : rebase_source : 572a7b22550a38ca71c954f62eefa695addd53c2
And remove unreachable code after MOZ_CRASH().
MozReview-Commit-ID: 6ShBtPRKYlF
--HG--
extra : rebase_source : 0fe45a59411bda663828336e2686707b550144ae
extra : source : 8473fd7333d2abe1ea1cc176510c292a5b34df45
Currently, HTMLEditRules::WillInsertBreak() checks if the editing host can
contain a <p> element as a child or a descendant. However, this is not enough.
If an inline element has a block element which can contain a <p> element,
current implementation considers to insert a <br>. This is possible when
* The editing host is an unknown element including user defined element.
* The editing host is an inline element and its children and/or descendants
were added by JS. E.g., <span contenteditable> element can have <div>
element.
I think that we should consider to insert a <br> element when:
- There is no block ancestors in the editing host.
- The editing host is the only block element and it cannot contain <p> element
or the default paragraph separator is <br> element.
- The nearest block ancestor isn't a single-line container declared in the
execCommand spec and there are no block elements which can contain <p>
element.
Note that Chromium checks if CSS box of ancestors is block too. However,
it must be out of scope of this bug.
MozReview-Commit-ID: HdjU9t83Nd1
--HG--
extra : rebase_source : 7030671268a610613359b5d8ae5d126e120f59bd
Perhaps, most callers don't need parent block outside active editing host.
Therefore, callers of these methods should be able to specify the editing
host for making those methods stop looking for a block ancestor.
Then, callers can avoid using EditorUtils::IsDescendantOf() and
nsContentUtils::IsContentDescendantOf().
MozReview-Commit-ID: 7IK4gAVHY5d
--HG--
extra : rebase_source : 31cd55026f0ce005d906499de4ebe5d1c39555e9
Currently, HTMLEditor::GetBlockNodeParent(nsIDOMNode*) is used only by
HTMLEditor::DoInsertHTMLWithContext() and there is a variable of nsINode*.
So, we don't need to keep it anymore.
MozReview-Commit-ID: LEWaiR5BEB9
--HG--
extra : rebase_source : 64ef772f3b7883bd4aae48dec737663e6036553b
Masayuki Nakano
Makoto Kato
Bogdan Tara
Ryan VanderMeulen
Kyle Machulis
Narcis Beleuzu
Margareta Eliza Balazs
Boris Zbarsky
Sebastian Hengst
Nika Layzell
Chris Peterson