1175418 - Contenteditable: Typing beside a text node creates a new text node

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[david]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150511103303

Steps to reproduce:

Type at any cursor position that is adjacent to, but outside of, a text node. For example:

1. Open the following data URI page: data:text/html,<h1 align=center contenteditable><img src='%23'/>foo<img src='%23' /></h1>

2. Click before the first img (or after the second)

3. Cursor across the img to beside the 'foo' text

4. Press 'x'


Actual results:

The typed text is put into a separate new text node. In the example above, the h1's childNodes are: [ img, #text, #text, img ]


Expected results:

The typed text should become part of the 'foo' text node; i.e. the h1's childNodes should be [ img, #text, img ]

Comment 1

[david]

cf. https://code.google.com/p/chromium/issues/detail?id=501206 - similar behaviour in Chromium (but only immediately before the text node, not immediately after)

Comment 2

[Abe - QA (:Abe_LV)]

Hey David,

Are you still having this issue in the latest version?  Thanks

Comment 3

[david]

Hi Abe, I still get the issue on Firefox 42 and Firefox 45.0a (on Ubuntu 15.10).

Comment 4

[Abe - QA (:Abe_LV)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Nightly 45.0a1 Build ID:20151211030207
and Firefox 42.0


I am able to reproduce this.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 1175418 - Don't create unnecessary text nodes in editor;

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

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Oops, needs tests.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

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

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Probably there should be more tests, but the basic issue is very simple, so maybe it's not necessary.

Comment 11

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

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

https://reviewboard.mozilla.org/r/124780/#review127470

::: editor/libeditor/EditorBase.cpp:2446
(Diff revision 2)
> +  if (!node->IsNodeOfType(nsINode::eTEXT)) {
> +    if (offset && node->GetChildAt(offset - 1)->IsNodeOfType(nsINode::eTEXT)) {
> +      node = node->GetChildAt(offset - 1);
> +      offset = node->Length();
> +    } else if (offset < static_cast<int32_t>(node->Length()) &&
> +               node->GetChildAt(offset) ->IsNodeOfType(nsINode::eTEXT)) {

nit: unnecessary " " before "->".

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f66c724a9d8
Don't create unnecessary text nodes in editor; r=masayuki

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Makoto Kato [:m_kato]]

Aryeh, could you uplift this to 54?  bug 1327917 (regression by bug 1100966) is fixed by this.

Comment 17

[Nathan Froyd [:froydnj]]

Putting the affected flag here rather than in bug 1327917, since this bug actually has the fix.  Thanks for fixing this!

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1100966 (landed in 39).
[User impact if declined]: Bug 1327917.  I don't know the full scope of the bug, but it causes unexpected behavior in the editor in some cases.  The specific case reported sounds like it's only annoying -- it seems you can just click in the editor and continue as normal.
[Is this code covered by automated tests?]: Our test coverage for this type of editing code is pretty good.
[Has the fix been verified in Nightly?]: It landed on central over three weeks ago.
[Needs manual test from QE? If yes, steps to reproduce]: No manual testing needed, I don't think.
[List of other uplifts needed for the feature/fix]: N/a
[Is the change risky?]: More risky than adding a missing null check, but it doesn't seem very risky in absolute terms.  I would be surprised if it broke anything.  I'll defer to Masayuki if he disagrees.
[Why is the change risky/not risky?]: The change is only a few lines, and all it does is condense adjacent text nodes into one in some cases.  It seems unlikely that any code works correctly with multiple adjacent text nodes but breaks on a single text node, since a single text node is the overwhelmingly common case, and also a simpler case.  It seems likely that it only fixes bugs (like bug 1327917, which got fixed by mistake by this patch).  But I can't make any guarantees, because editor code is extremely complicated.
[String changes made/needed]: None.

Comment 19

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

I don't disagree with it. Our editor still has a lot of bad issues of compatibility with other browsers and UX. So, fixing bad UX caused by regressions should be better than keeping bad behavior since anyway most regressions will be reported in late Beta or after release.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

Redirecting uplift request in comment 18.

Comment 21

[Gerry Chang [:gchang]]

Hi Florin,
could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 22

[Andrei Vaida [:avaida]]

(In reply to Gerry Chang [:gchang] from comment #21)
> Hi Florin,
> could you help find someone to verify if this issue was fixed as expected on
> a latest Nightly build? Thanks!

Forwarding this to Brindusa, as she's usually taking care of these kind of requests on the Nightly channel.

Comment 23

[ovidiu boca[:Ovidiu]]

I tested this issue on FF Nightly 55.0a1(2017-04-25) with Ubuntu 16.04, Mac OS X 10.10 and Windows 10 x64 and I can't reproduce it. Please note that I tried to test this to see if I can reproduce the bug, with a Nightly build from 2017-03-22, but I couldn't. 
Based on the fact that this issue is no longer reproducible with the latest FF 55.0a1 Nightly build (2017-04-25) I will mark this as a verified fix.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

Fix a regression related to editor and was verified. Beta54+. Should be in 54 beta 3.

Comment 25

[ovidiu boca[:Ovidiu]]

David, can you please try to verify this issue on your end? To do that, please install the Nightly build from here: https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly

Comment 26

[david]

Yes, I can confirm it's working correctly for me on the current Nightly. Thanks!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e65cc09cdf87

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

See comment 18.

Comment 29

[Paul Silaghi, QA [:pauly]]

Reproduced on Fx 53.0.
Verified fixed on Fx 54b3, Win 10.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

This is a pretty old regression. ESR release uplift bar includes security fixes, and fixes to severe recent regressions. I don't believe this one meets the bar.