1415800 - EditorBase::GetPriorNode(), EditorBase::GetNextNode(), HTMLEditor::GetPriorHTMLNode(), HTMLEditor::GetNextHTMLNode() should use EditorRawDOMPoint

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is a follow up bug of bug 1407305.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9ec3ebed47de04655b6e6bca2fcc87a6ba6378

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3049d3c8493a92b603fb23dd67988ffbd24ee3df

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66886535122733b11cbf760963cb4e49e03ad6d1

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94aaf7ed33b63ee6f25a9cc1ce15164f446d379a

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8d7f2fa8691afdaf17286c31687dc103cc0cd01

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09bd8b88e6161c3940a3575b29b56b7fa91dfee

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50bbca49f7b0ad3b87c6e7057d6f4a26bb9ef1aa

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67f50b1fe938109ee64c9611a8d2045a5a3a316f

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7c0effd5987f3c92c085faa113330a38345ec7

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1415800 - part 1: Redesign EditorBase::GetPriorNode() with EditorRawDOMPoint

An overload of EditorBase::GetPriorNode() takes a set of container, child node
and offset of the child in the container.  Replacing it with EditorRawDOMPoint
makes the caller simpler.

Additionally, it has two bool arguments, one is for searching if editable node,
the other is for searching if in same block.  So, they can be hidden with
some human readable inline methods.

Finally, "Prior" isn't a term of DOM.  So, let's use "Previous" instead.

Review commit: https://reviewboard.mozilla.org/r/198354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198354/

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1415800 - part 2: Redesign EditorBase::GetNextNode() with EditorRawDOMPoint

An overload of EditorBase::GetNextNode() takes a set of container, child node
and offset of the child in the container.  Replacing it with EditorRawDOMPoint
makes the caller simpler.

Additionally, it has two bool arguments, one is for searching if editable node,
the other is for searching if in same block.  So, they can be hidden with
some human readable inline methods.

When I was creating this patch, I realized that
GetNextNodeInternal(const EditorRawDOMPoint& aPoint) may return
aPoint.GetChildAtOffset().  I.e., it starts to search from the specified point
rather than next node.  On the other hand, GetNextNodeInternal(nsINode& aNode)
never returns aNode itself.  So, it there is better name instead of "Next",
we should take it.  But I have no better idea.  So, this patch just explains
the difference with comments in EditorBase.h.

Review commit: https://reviewboard.mozilla.org/r/198356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198356/

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1415800 - part 3: Redesign HTMLEditor::GetPriorHTMLNode() as similar to EditorBase::GetPrevious*()

HTMLEditor::GetPriorHTMLNode() methods are similar to EditorBase::GetPriorNode()
which was redesigned with the previous patch.

So, it should be redesigned as
HTMLEditor::GetPreviousEditableHTMLNode(nsINode&),
HTMLEditor::GetPreviousEditableHTMLNode(const EditorRawDOMPoint&),
HTMLEditor::GetPreviousEditableHTMLNodeInBlock(nsINode&) and
HTMLEditor::GetPreviousEditableHTMLNodeInBlock(const EditorRawDOMPoint&).

Note that HTMLEditor::GetPriorHTMLNode() are always return editable node.
So, we don't need to create non-editable node methods for them.

Although, I don't like the word "HTMLNode" because this can return SVG element
or something too.  The additional feature of those methods is just checking
given node is in active editing host.  So, they are for HTML editor, but not
returning only HTML nodes.  However, I have no better idea with shorter name.

Review commit: https://reviewboard.mozilla.org/r/198358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198358/

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1415800 - part 4: Redesign HTMLEditor::GetNextHTMLNode() same as similar to EditorBase::GetNext*()

HTMLEditor::GetNextHTMLNode() should be redesigned as
HTMLEditor::GetNextEditableHTMLNode(nsINode&),
HTMLEditor::GetNextEditableHTMLNodeInBlock(nsINode&),
HTMLEditor::GetNextEditableHTMLNode(const EditorRawDOMPoint&) and
HTMLEditor::GetNextEditableHTMLNodeInBlock(const EditorRawDOMPoint&).

Same as GetPreviousEditableHTMLNode*(), we don't need the methods to find
non-editable nodes too.

Review commit: https://reviewboard.mozilla.org/r/198360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198360/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1415800 - part 5: Redesign HTMLEditRules::FindNearSelectableNode()

First, the method name is not correct.  It tries to find an editable node near
the given DOM point.  Therefore, it should be FindNearEditableNode().

Next, the implementation did something odd.  E.g., in the first |if| block,
when |nearNode| is nullptr, it returns nullptr.  However, following |if| block
does something only when |nearNode| is nullptr.  So, we can get rid of the
second |if| block.  Then, nobody will change aDirection.  So, we can make it
not a reference now.

Similarly, in |while| block, if |nearNode| becomes nullptr, it returns error.
However, following block checks if |nearNode| is NOT nullptr.  So, we can get
rid of this |if| statement and outdent its block.

Additionally, |curNode| isn't necessary.  It only increments the refcount
redundantly.  So, we can get rid of it.

Finally, FindNearEditableNode() can return found node directly instead of
error code because error code doesn't make sense.  Not found an editable
node is not illegal.  And also it can take EditorRawDOMPoint instead of
a set of container, child and offset of the child in the container.

Review commit: https://reviewboard.mozilla.org/r/198362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198362/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf51aa102e9e596604c5ea1ad5d44a959ed8729

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8927129 [details]
Bug 1415800 - part 1: Redesign EditorBase::GetPriorNode() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/198354/#review203656

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8927129 [details]
Bug 1415800 - part 1: Redesign EditorBase::GetPriorNode() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/198354/#review203662

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8927130 [details]
Bug 1415800 - part 2: Redesign EditorBase::GetNextNode() with EditorRawDOMPoint

https://reviewboard.mozilla.org/r/198356/#review203666

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8927131 [details]
Bug 1415800 - part 3: Redesign HTMLEditor::GetPriorHTMLNode() as similar to EditorBase::GetPrevious*()

https://reviewboard.mozilla.org/r/198358/#review203668

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8927132 [details]
Bug 1415800 - part 4: Redesign HTMLEditor::GetNextHTMLNode() same as  similar to EditorBase::GetNext*()

https://reviewboard.mozilla.org/r/198360/#review203680

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8927133 [details]
Bug 1415800 - part 5: Redesign HTMLEditRules::FindNearSelectableNode()

https://reviewboard.mozilla.org/r/198362/#review203686

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8aab8a9a0f7
part 1: Redesign EditorBase::GetPriorNode() with EditorRawDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5a622e094403
part 2: Redesign EditorBase::GetNextNode() with EditorRawDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ed0dad4054fa
part 3: Redesign HTMLEditor::GetPriorHTMLNode() as similar to EditorBase::GetPrevious*() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/682cda40a859
part 4: Redesign HTMLEditor::GetNextHTMLNode() same as  similar to EditorBase::GetNext*() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/459541207265
part 5: Redesign HTMLEditRules::FindNearSelectableNode() r=m_kato

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8aab8a9a0f7
https://hg.mozilla.org/mozilla-central/rev/5a622e094403
https://hg.mozilla.org/mozilla-central/rev/ed0dad4054fa
https://hg.mozilla.org/mozilla-central/rev/682cda40a859
https://hg.mozilla.org/mozilla-central/rev/459541207265