1420415 - TextEditor::CreateBRImpl() returns wrong offset if it splits a text node

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Bug 1420415 - TextEditor::CreateBRImpl() needs to make pointToInsertBrNode store mOffset before calling EditorBase::CreateNode()

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.

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

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8932023 [details]
Bug 1420415 - TextEditor::CreateBRImpl() needs to make pointToInsertBrNode store mOffset before calling EditorBase::CreateNode()

https://reviewboard.mozilla.org/r/203088/#review208372

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fc00183c31d
TextEditor::CreateBRImpl() needs to make pointToInsertBrNode store mOffset before calling EditorBase::CreateNode() r=m_kato

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8fc00183c31d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Does this need to be uplifted to Beta or can it ride the 59 train?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> Does this need to be uplifted to Beta or can it ride the 59 train?

Should be uplifted because it's really simple. But perhaps, I need to create patch for Beta because the code is changed after the last merge. I'll request approval after creating it (or confirming if it's graftable).

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Patch for beta (just renamed the variable from the patch for m-c)

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

[User impact if declined]:
*Might* break some web apps with this unexpected behavior change.

[Is this code covered by automated tests?]:
No because I've not found how to hit this behavior change without changing some other code yet.

[Has the fix been verified in Nightly?]:
Yes. I found this bug with fixing another bug which caused new orange. However, this patch fixes the new orange.

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

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

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Just making the local variable store offset which will be referred after changing DOM tree before changing the DOM tree.

[String changes made/needed]:
No.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8932335 [details] [diff] [review]
Patch for beta (just renamed the variable from the patch for m-c)

A low risk fix. Let's take it in Beta58.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3e16c8e2364b