The default bug view has changed. See this FAQ.

Revert broken execCommand("insertparagraph") support

RESOLVED FIXED in mozilla16

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({regression})

Trunk
mozilla16
regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15+ disabled)

Details

Attachments

(2 attachments)

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.
Flags: in-testsuite-
Created attachment 630513 [details] [diff] [review]
Patch v1
Attachment #630513 - Flags: review?(ehsan)
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.
Attachment #630513 - Flags: approval-mozilla-aurora?

Updated

5 years ago
tracking-firefox15: ? → +

Updated

5 years ago
Attachment #630513 - Flags: review?(ehsan) → review+
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?
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.
Attachment #630580 - Flags: approval-mozilla-aurora?
Attachment #630513 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ec24d61858
Target Milestone: --- → mozilla16
Comment on attachment 630580 [details] [diff] [review]
Backout patch for Aurora

r+, in case we require that for branch landings these days.
Attachment #630580 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d6ec24d61858
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 8

5 years ago
(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?
(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!
(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!

Updated

5 years ago
Attachment #630580 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/8a3042764de7
status-firefox15: --- → disabled
You need to log in before you can comment on or make changes to this bug.