Last Comment Bug 761993 - Revert broken execCommand("insertparagraph") support
: Revert broken execCommand("insertparagraph") support
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 748307
  Show dependency treegraph
 
Reported: 2012-06-06 04:57 PDT by :Aryeh Gregor (away until August 15)
Modified: 2012-06-12 21:28 PDT (History)
2 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
disabled


Attachments
Patch v1 (293.90 KB, patch)
2012-06-06 05:02 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Backout patch for Aurora (730.76 KB, patch)
2012-06-06 08:33 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Aryeh Gregor (away until August 15) 2012-06-06 04:57:50 PDT
The test-case in bug 761861 happened to alert me to the fact that document.execCommand("insertparagraph") actually works identically to document.execCommand("inserttext") after bug 748307 landed, due to a logic error.  We did support insertparagraph before, albeit with different functionality, so this is a regression in Firefox 15.  It's kind of sad that I didn't notice this before submitting the patch, but oh well . . .

The patch to make it work right would be nontrivial, as it turns out, because insertText("\n") actually inserts a <br> -- it doesn't behave like hitting Enter.  I think I'll treat that as part of bug 748308.
Comment 1 :Aryeh Gregor (away until August 15) 2012-06-06 05:02:42 PDT
Created attachment 630513 [details] [diff] [review]
Patch v1
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-06 05:08:50 PDT
Comment on attachment 630513 [details] [diff] [review]
Patch v1

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e9f54147eb76

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 748307.
User impact if declined: The command document.execCommand("insertparagraph") will work incorrectly.  Firefox behavior in version < 15 did not match other browsers or the spec, but had been around for many years.  In a future version, perhaps 16, we hope to change the behavior to match WebKit and the spec.  The current behavior in 15 is incorrect, matches no one, and makes no sense.  It's probably not going to actually break many sites, because of the lack of interoperability here.  Nevertheless, it's still an accidental, clearly incorrect behavior change, and it would be undesirable to have this behavior stick around for a release before being reverted -- that would only decrease interop of insertParagraph still further.  Thus we should revert the behavior in 15 back to that of 14 and earlier.
Testing completed (on m-c, etc.): The patch reverts to the exact same code as we've been using for years -- it's a partial backout of bug 748307 part 5.
Risk to taking this patch (and alternatives if risky): Essentially none.  This just reverts to the same behavior we've had since (AFAIK) execCommand() was first implemented several years ago.
String or UUID changes made by this patch: None.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-06 08:17:48 PDT
I think we should back out bug 748307 on Aurora, and not attempt to fix this there.  Can you please attach a patch for the backout here and request approval on it?
Comment 4 :Aryeh Gregor (away until August 15) 2012-06-06 08:33:18 PDT
Created attachment 630580 [details] [diff] [review]
Backout patch for Aurora

https://tbpl.mozilla.org/?tree=Try&rev=eaa511f3e7b3

See comment 2 for reasoning.  This backs out the offending changeset completely for Aurora instead of partially, as requested by Ehsan.
Comment 5 :Aryeh Gregor (away until August 15) 2012-06-06 10:34:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ec24d61858
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-06 10:37:36 PDT
Comment on attachment 630580 [details] [diff] [review]
Backout patch for Aurora

r+, in case we require that for branch landings these days.
Comment 7 Ed Morley [:emorley] 2012-06-07 05:52:29 PDT
https://hg.mozilla.org/mozilla-central/rev/d6ec24d61858
Comment 8 Alex Keybl [:akeybl] 2012-06-11 13:01:53 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> I think we should back out bug 748307 on Aurora, and not attempt to fix this
> there.  Can you please attach a patch for the backout here and request
> approval on it?

Aryeh/Ehsan - Is the current patch still nominated for approval? My understanding is it's a partial backout of bug 748307, but Ehsan suggests we do a full backout of bug 748307. What's the benefit to leaving any part of bug 748307 in FF15?
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-11 13:44:55 PDT
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > I think we should back out bug 748307 on Aurora, and not attempt to fix this
> > there.  Can you please attach a patch for the backout here and request
> > approval on it?
> 
> Aryeh/Ehsan - Is the current patch still nominated for approval? My
> understanding is it's a partial backout of bug 748307, but Ehsan suggests we
> do a full backout of bug 748307. What's the benefit to leaving any part of
> bug 748307 in FF15?

Yes this patch is still up for approval.  The rest of the parts in bug 748307 are just code cleanup, and do not change the semantics of the program, so we don't need to back out those parts in the interest of touching as few lines of code as possible.  If you don't agree, please let us know.  Thanks!
Comment 10 Alex Keybl [:akeybl] 2012-06-11 14:28:49 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Yes this patch is still up for approval.  The rest of the parts in bug
> 748307 are just code cleanup, and do not change the semantics of the
> program, so we don't need to back out those parts in the interest of
> touching as few lines of code as possible.  If you don't agree, please let
> us know.  Thanks!

Sounds like a reasonable method of risk mitigation - just wanted to make sure we're all on the same page. Thanks!
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-12 21:28:21 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/8a3042764de7

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