Last Comment Bug 787432 - After keyup in contenteditable, Firefox 15 replaces full node of inner text selection
: After keyup in contenteditable, Firefox 15 replaces full node of inner text s...
Status: RESOLVED FIXED
[qa-]
: regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 15 Branch
: x86 Mac OS X
: -- normal with 6 votes (vote)
: mozilla18
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
:
Mentors:
Depends on:
Blocks: 590640
  Show dependency treegraph
 
Reported: 2012-08-31 08:18 PDT by Matthew DeLambo
Modified: 2012-10-16 16:12 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
Reporter's testcase (1.06 KB, text/html)
2012-09-24 00:26 PDT, Loic
no flags Details
Range Test Case (1.03 KB, text/html)
2012-09-24 05:41 PDT, Matthew DeLambo
no flags Details
Patch (27.89 KB, patch)
2012-09-27 05:44 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Matthew DeLambo 2012-08-31 08:18:39 PDT
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).
Comment 1 Matthias Versen [:Matti] 2012-09-22 16:08:47 PDT
Please describe what I have to do at your testcase to reproduce the bug, thanks
Comment 2 Matthew DeLambo 2012-09-23 18:00:15 PDT
(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/
Comment 3 Loic 2012-09-24 00:26:22 PDT
Created attachment 663936 [details]
Reporter's testcase

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.
Comment 4 Matthew DeLambo 2012-09-24 05:41:20 PDT
Created attachment 664030 [details]
Range Test Case

Attached...
Comment 5 Loic 2012-09-24 06:19:37 PDT
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.
Comment 6 Alice0775 White 2012-09-24 07:24:41 PDT
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
Comment 7 Loic 2012-09-24 07:59:22 PDT
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
Comment 8 :Ehsan Akhgari 2012-09-24 09:05:43 PDT
Aryeh, can you look into this, please?
Comment 9 Aryeh Gregor (:ayg) (away until October 25) 2012-09-27 04:51:46 PDT
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.
Comment 10 Aryeh Gregor (:ayg) (away until October 25) 2012-09-27 05:44:31 PDT
Created attachment 665378 [details] [diff] [review]
Patch

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
Comment 11 :Ehsan Akhgari 2012-09-27 10:55:49 PDT
Comment on attachment 665378 [details] [diff] [review]
Patch

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

Looks great!
Comment 13 :Ehsan Akhgari 2012-09-27 10:58:22 PDT
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.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:15:02 PDT
https://hg.mozilla.org/mozilla-central/rev/5ef5ee5e312c
Comment 15 Alex Keybl [:akeybl] 2012-09-28 10:34:56 PDT
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.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-16 16:12:55 PDT
Actually, changing verifyme to [qa-] since we have a landed testcase in-testsuite.

Note You need to log in before you can comment on or make changes to this bug.