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

VERIFIED FIXED in Firefox 54

Status

()

Core
Editor
VERIFIED FIXED
3 years ago
7 months ago

People

(Reporter: david, Assigned: ayg)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla55
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52- wontfix, firefox53 wontfix, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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 ]
(Reporter)

Comment 1

3 years ago
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)

Updated

3 years ago
Flags: needinfo?(ehsan)

Comment 2

2 years ago
Hey David,

Are you still having this issue in the latest version?  Thanks
Component: Untriaged → Editor
Flags: needinfo?(david)
Product: Firefox → Core
See Also: → bug 1193517

Updated

2 years ago
See Also: → bug 237964
(Reporter)

Comment 3

2 years ago
Hi Abe, I still get the issue on Firefox 42 and Firefox 45.0a (on Ubuntu 15.10).
Flags: needinfo?(david)

Comment 4

2 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → bug 1350554

Updated

9 months ago
Flags: needinfo?(ehsan)
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Duplicate of this bug: 1350554
Blocks: 1341152
Comment hidden (mozreview-request)
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab8114b3e05ca18351162e8defb7447cd634e161
Oops, needs tests.
Comment hidden (mozreview-request)
Probably there should be more tests, but the basic issue is very simple, so maybe it's not necessary.

Comment 11

9 months ago
mozreview-review
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 "->".
Attachment #8852585 - Flags: review?(masayuki) → review+
Comment hidden (mozreview-request)

Comment 13

9 months ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/8f66c724a9d8
Don't create unnecessary text nodes in editor; r=masayuki

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f66c724a9d8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Aryeh, could you uplift this to 54?  bug 1327917 (regression by bug 1100966) is fixed by this.
Flags: needinfo?(ayg)
Duplicate of this bug: 1327917
Putting the affected flag here rather than in bug 1327917, since this bug actually has the fix.  Thanks for fixing this!
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Keywords: regression
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.
Flags: needinfo?(ayg) → needinfo?(masayuki)
Attachment #8852585 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(masayuki)
Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

Redirecting uplift request in comment 18.
Attachment #8852585 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Hi Florin,
could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
(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.
Flags: needinfo?(florin.mezei) → needinfo?(brindusa.tot)

Comment 23

8 months ago
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
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.
Attachment #8852585 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 25

8 months ago
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
Flags: needinfo?(brindusa.tot) → needinfo?(david)
(Reporter)

Comment 26

8 months ago
Yes, I can confirm it's working correctly for me on the current Nightly. Thanks!
Flags: needinfo?(david)

Comment 27

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e65cc09cdf87
status-firefox54: affected → fixed
Flags: in-testsuite+
tracking-firefox-esr52: --- → ?
Comment on attachment 8852585 [details]
Bug 1175418 - Don't create unnecessary text nodes in editor;

See comment 18.
Attachment #8852585 - Flags: approval-mozilla-esr52?
Reproduced on Fx 53.0.
Verified fixed on Fx 54b3, Win 10.
status-firefox54: fixed → verified
Flags: qe-verify+

Comment 30

7 months ago
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.
Attachment #8852585 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-

Updated

7 months ago
status-firefox-esr52: affected → wontfix
tracking-firefox-esr52: ? → -
You need to log in before you can comment on or make changes to this bug.