1453872 - HTMLEditRules::JoinNodesSmart() doesn't set initial result correctly

Status: RESOLVED FIXED

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

Description

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

This is regression of bug 1423835.

If HTMLEditRules::JoinNodesSmart() joins aLeftNode and aRightNode actually, children of aLeftNode are inserted before first child of aRightNode and returns the point of old first node of aRightNode as its result.

However, after fixing bug 1423835, it returns point at nth child of new aRightNode, nth is length of old right node children.

I'm not sure how to get actual symptom though.

Comment 1

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

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

Comment 2

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

Attachment: Bug 1453872 - Make HTMLEditRules::JoinNodesSmart() return { aRightNode - aLeftNode.Length() } by default

This is regression of bug 1423835.

When I fixed the bug, I accidentally changed the result of
HTMLEditRules::JoinNodesSmart() to use new API.  However, it was simple
misunderstand.  The original code sets the initial value of result to
{ aRightNode - aLeftNode.Length() } but I understood it as
{ aRightNode - aRightNode.Length() }.  Therefore, this patch backs out the
patch only for this line.

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

Comment 3

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

Here is the diff of bug 1423835:
https://searchfox.org/mozilla-central/diff/d95c27869decc6a8d7787d8245a23cc095e35956/editor/libeditor/HTMLEditRules.cpp#7614

Comment 5

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

Fortunately, we've already gotten an actual bug report with STR. I'll add automated test for this.

Comment 6

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

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

Comment 7

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

Comment on attachment 8967664 [details]
Bug 1453872 - Make HTMLEditRules::JoinNodesSmart() return { aRightNode - aLeftNode.Length() } by default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236394/diff/1-2/

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8967664 [details]
Bug 1453872 - Make HTMLEditRules::JoinNodesSmart() return { aRightNode - aLeftNode.Length() } by default

https://reviewboard.mozilla.org/r/236394/#review242494

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc6e8d505359
Make HTMLEditRules::JoinNodesSmart() return { aRightNode - aLeftNode.Length() } by default r=m_kato

Comment 10

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

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

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bc6e8d505359

Comment 13

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

Attachment: Patch for Beta (only changes manifest file of wpt)

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1423835.

[User impact if declined]:
User will see odd crate position when removing <br> element with Delete or Backspace.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Not yet, but new automated test passed on m-c.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This is one line backing out.

[String changes made/needed]:
No.

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8968139 [details] [diff] [review]
Patch for Beta (only changes manifest file of wpt)

editor regression fix, approved for 60.0b13

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/01e680ea0270