Closed Bug 787432 Opened 12 years ago Closed 12 years ago

After keyup in contenteditable, Firefox 15 replaces full node of inner text selection

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
15 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
firefox17 + fixed
firefox18 + fixed

People

(Reporter: delambo, Assigned: ayg)

References

Details

(Keywords: regression, testcase, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

Range Test Case
1.03 KB, text/html
Details
Patch
27.89 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta-
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.57 Safari/537.1

Steps to reproduce:

Please reference this jsfiddle:

http://jsfiddle.net/rFuxj/2/

In a contenteditable, in a keypress event handler, I use the current selection/range to insert a new node (span) with one character ("%"). Following that, I set the range to select the new character - startContainer and endContainer both equal new node, startPos and endPos equal 0 and 1, respectively.


Actual results:

The newly inserted node gets deleted on character insertion by the firefox browser.


Expected results:

Before FF 15 and still in Webkit, the browser should delete the selection and insert the newly typed character in place, without deleting the container (the newly inserted node).
Please describe what I have to do at your testcase to reproduce the bug, thanks
(In reply to Matthias Versen (Matti) from comment #1)
> Please describe what I have to do at your testcase to reproduce the bug,
> thanks

The result pane (bottom right pane) is a contenteditable region. Click in the editable and type a character. The new character should be green (wrapped in a <span.insert>) - this is the case for Firefox before version 15, IE9+, and Webkit.

I updated the fiddle with better comments at the following version:

http://jsfiddle.net/rFuxj/3/
Attached file Reporter's testcase (obsolete) — 
Could you attach a simple html testcase instead of jsfiddle testcase, please.
Sometimes the result is different between jsfiddle and a local html file open in the same Firefox version.

It looks like it's the case if I transform the jsfiddle testcase into a local file.
Attached file Range Test Case — 
Attached...
Attachment #664030 - Attachment mime type: text/plain → text/html
Attachment #663936 - Attachment is obsolete: true
Regression range:

m-c
good=2012-05-18
bad=2012-09-19
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f

There are some bugs about core:editor (Ms2ger & Aryeh Gregor) in the changelog, surely one of them is the culprit. Bisection needed.
Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
Keywords: regression, regressionwindow-wanted, testcase
Product: Firefox → Core
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/3b86eaa02d91
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120518080514
Bad:
http://hg.mozilla.org/mozilla-central/rev/17422a2d0c70
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120518104452
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3b86eaa02d91&tochange=17422a2d0c70


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/11780e80c8c3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517220915
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e8ebc8f1825e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517232316
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=11780e80c8c3&tochange=e8ebc8f1825e
Thanks. I bet my 2 cents on
Aryeh Gregor — Bug 590640 part 6 - Don't create empty style tags unless we're about to insert text in them; r=ehsan :D
Aryeh, can you look into this, please?
Assignee: nobody → ayg
More minimal test-case (uses insertText, so probably only works in Gecko/WebKit):

data:text/html,<!doctype html>
<div contenteditable><span class="insert">%</span><br></div>
<script>
getSelection().collapse(document.querySelector("span"), 0);
getSelection().extend(document.querySelector("span"), 1);
document.execCommand("inserttext", false, "x");
document.body.textContent = document.body.firstChild.innerHTML;
</script>

This should output

  <span class="insert">x</span><br>

but in current Gecko it outputs

  x<br>

Removing the <br> fixes the problem, oddly!

Bug 590640 is probably the culprit, yeah.  I'll investigate.
Status: NEW → ASSIGNED
Attached patch Patch — — Splinter Review 
One-line fix.  This has the negative side effect that it might leave empty wrappers around in some corner cases, namely if the text doesn't actually wind up being inserted for some reason, but that doesn't seem worth worrying about.  After writing a test, I found this was already tested by runtest.html anyway (yay for exhaustive test suites!).

Try, Linux64 opt only (since I don't anticipate platform-specific issues): https://tbpl.mozilla.org/?tree=Try&rev=3834a7a9488f
Attachment #665378 - Flags: review?(ehsan)
Blocks: 590640
Keywords: regressionwindow-wanted
Comment on attachment 665378 [details] [diff] [review]
Patch

Review of attachment 665378 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #665378 - Flags: review?(ehsan) → review+
Comment on attachment 665378 [details] [diff] [review]
Patch

This is a very simple fix for a regression which we shipped in 15.  I would like us to get it fixed in 16 and above.
Attachment #665378 - Flags: approval-mozilla-beta?
Attachment #665378 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5ef5ee5e312c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 665378 [details] [diff] [review]
Patch

[Triage Comment]
We have a full handle on this regression as it occurred in FF15, and we can't take non-critical bug fixes in our final beta. Approving for Aurora 17.
Attachment #665378 - Flags: approval-mozilla-beta?
Attachment #665378 - Flags: approval-mozilla-beta-
Attachment #665378 - Flags: approval-mozilla-aurora?
Attachment #665378 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Actually, changing verifyme to [qa-] since we have a landed testcase in-testsuite.
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: