1419745 - Editor(Raw)DOMPoint should store child node at offset by itself

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

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

I realized this big bug.

Editor(Raw)DOMPoint does not store anything by itself. Its base class, (Raw)RangeBoundary stores container, *previous sibling of* child at offset and offset.

If users of Editor(Raw)DOMPoint want to keep referring child node but the child node is moved different point unexpectedly (e.g., by mutation observer), Editor(Raw)DOMPoint keeps referring the *old* previous sibling.  Therefore, GetChildAtOffset() will return different child node with current DOM tree.

So, Editor(Raw)DOMPoint should cache the child node by itself.

So, this bug is, we've changed HTML editor behavior in a lot of places if web apps changes position of new DOM nodes which are created by editor.

Comment 1

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

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

Comment 2

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

Hmm, I was thinking that Editor(Raw)DOMPoint should override some methods of RangeBoundaryBase to cache actual child node at offset.  However, that causes making EditorDOMPointBase complicated and riskier to change RangeBoundaryBase. So, probably, we should stop EditorDOMPointBase inherits RangeBoundaryBase since it's not useful base class for editor.

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

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

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

Comment 9

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

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

Comment 10

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

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

Comment 11

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

The patch is too risky to uplift and fortunately, we have no regression reports depending on this bug. So, we should give up to fix this on 58. (FYI: The fixes causing this regression fixed some regressions which are new in 58. So, we shouldn't back them out.)

Comment 12

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

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

Comment 13

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

Attachment: Bug 1419745 - part 1: Make EditorDOMPointBase not a sub class of RangeBoundaryBase and duplicate methods of RangeBoundaryBase into EditorDOMPointBase

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.

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

Comment 14

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

Attachment: Bug 1419745 - part 2: Make EditorDOMPointBase store child node at offset instead of previous sibling of child node at offset

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.

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

Comment 15

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

Attachment: Bug 1419745 - part 3: CreateElementTransaction and SplitNodeTransaction should store DOM point with EditorDOMPoint rather than RangeBoundary

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.

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

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8932450 [details]
Bug 1419745 - part 1: Make EditorDOMPointBase not a sub class of RangeBoundaryBase and duplicate methods of RangeBoundaryBase into EditorDOMPointBase

https://reviewboard.mozilla.org/r/203496/#review209274

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8932451 [details]
Bug 1419745 - part 2: Make EditorDOMPointBase store child node at offset instead of previous sibling of child node at offset

https://reviewboard.mozilla.org/r/203498/#review209288

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8932452 [details]
Bug 1419745 - part 3: CreateElementTransaction and SplitNodeTransaction should store DOM point with EditorDOMPoint rather than RangeBoundary

https://reviewboard.mozilla.org/r/203500/#review209290

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31082fdbf643
part 1: Make EditorDOMPointBase not a sub class of RangeBoundaryBase and duplicate methods of RangeBoundaryBase into EditorDOMPointBase r=m_kato
https://hg.mozilla.org/integration/autoland/rev/dc40e5160744
part 2: Make EditorDOMPointBase store child node at offset instead of previous sibling of child node at offset r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9319d599c3b0
part 3: CreateElementTransaction and SplitNodeTransaction should store DOM point with EditorDOMPoint rather than RangeBoundary r=m_kato

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/31082fdbf643
https://hg.mozilla.org/mozilla-central/rev/dc40e5160744
https://hg.mozilla.org/mozilla-central/rev/9319d599c3b0

Comment 21

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

Now, EditorDOMPointBase<A, B> has stopped inheriting RangeBoundaryBase<A, B>. So, we can back some patches out from RangeBoundary.h, that were added for EditorDOMPoint.

catalinb, do you think I should back them out from RangeBoundary.h to keep the implementation as far as simple? If so, I'll file a follow up bug and post a patch.

Comment 22

[Cătălin Badea (:catalinb)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #21)
> Now, EditorDOMPointBase<A, B> has stopped inheriting RangeBoundaryBase<A,
> B>. So, we can back some patches out from RangeBoundary.h, that were added
> for EditorDOMPoint.
> 
> catalinb, do you think I should back them out from RangeBoundary.h to keep
> the implementation as far as simple? If so, I'll file a follow up bug and
> post a patch.

Yes, please. No reason to keep them around if they aren't used anymore.