Closed Bug 795418 Opened 12 years ago Closed 7 years ago

Paste it as quotation doesn't work when pasting into <font> tag

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: hape, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: regression, testcase)

Attachments

(6 files, 6 obsolete files)

Simple HTML page with instructions showing the problem
457 bytes, text/html
Details
fix
7.50 KB, patch
Details | Diff | Splinter Review
Jorg's testcase with some minor editorial corrections
2.72 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
More tests that hits other branches in WipeContainingBlock
12.20 KB, patch
Details | Diff | Splinter Review
Alternative fix
4.32 KB, patch
masayuki
: review+
ritu
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
part 1 for beta
4.44 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

since my last update to Thunderbird 15.0.1 in the compose window this is not working: 

copy a text (or part of) in this compose-window
paste this text as quotation

The only possible way is, to paste this text first into the Win-texteditor / Wordpad or a Word-doc, then copy it from there again and then "paste as quotation"


Actual results:

Doing this:

copy a text (or part of) in this compose-window
paste this text as quotation

nothing happens


Expected results:

the copied & pasted text should be here as a quoted text
Summary: copy part of mailtext and paste it as quotation doesn't work → copy part of mailtext and paste it as quotation sometimes doesn't work
Version: 15 → 16
Flags: needinfo?(hape)
This problem can be reproduced in Firefox, no Thunderbird needed. Open the page and follow the instructions.
Attachment #8604880 - Attachment is obsolete: true
Attachment #8604881 - Attachment is obsolete: true
Whiteboard: [STR comment 35]
I made *all* comments obsolete, since they are. All you need is the simple example that shows the bug in Firefox.
Alice, the reporter suggests that this broke in TB 15.0.1. I don't know whether finding the regression here would help with fixing it. If you have some time, could you look in a regression in Firefox. Please use the attachment which also gives the STR in Firefox.
Flags: needinfo?(alice0775)
(In reply to Jorg K (GMT+1) from comment #44)
> Alice, the reporter suggests that this broke in TB 15.0.1. I don't know
> whether finding the regression here would help with fixing it. If you have
> some time, could you look in a regression in Firefox. Please use the
> attachment which also gives the STR in Firefox.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5e8a0a47dc5&tochange=36c7d7fdc2ea

Suspect: 838fb33405ba	Ehsan Akhgari — Bug 612128 - Prevent the editor from modifying nodes which are not under an editing host; r=roc,bzbarsky
Flags: needinfo?(alice0775)
Blocks: 612128
Keywords: regression
Alice, thank you so much. I'm impressed and surprised every time you help us. That helps a great deal.
Problem is here:
https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/editor/libeditor/HTMLEditorDataTransfer.cpp#234
IsEditable(targetNode) returns false here, and no pasting takes place.
I'm sure that's a consequence of the bug Alice pointed out.
The decision that the target isn't editable is taken here:
https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/editor/libeditor/EditorBase.cpp#3459

And the stack is this:
xul.dll!mozilla::IsElementVisible(mozilla::dom::Element * aElement) Line 3459	C++
xul.dll!mozilla::EditorBase::IsEditable(nsINode * aNode) Line 3505	C++
xul.dll!mozilla::HTMLEditor::IsEditable(nsINode * aNode) Line 5255	C++
xul.dll!mozilla::EditorBase::IsEditable(nsIDOMNode * aNode) Line 3490	C++
xul.dll!mozilla::HTMLEditor::DoInsertHTMLWithContext(...) Line 234	C++ <===
xul.dll!mozilla::HTMLEditor::InsertFromTransferable(...) Line 1186	C++
xul.dll!mozilla::HTMLEditor::Paste(int aSelectionType) Line 1468	C++
xul.dll!mozilla::HTMLEditor::PasteAsCitedQuotation(...) Line 1664	C++
xul.dll!mozilla::HTMLEditor::PasteAsQuotation(int aSelectionType) Line 1626	C++

IsEditable() being called where indicated in HTMLEditorDataTransfer.cpp#234

Who's the best person to ask about this?
Boris added that "lazy bit stuff":
https://hg.mozilla.org/mozilla-central/rev/efb577526489

Ehsan landed the regressing bug where nsHTMLEditor::IsEditable() was added, now called HTMLEditor::IsEditable(), which calls into nsPlaintextEditor::IsEditable(aNode), now called EditorBase::IsEditable().
https://hg.mozilla.org/mozilla-central/rev/838fb33405ba#l14.37
EditorBase::IsEditable() calls IsElementVisible() that decides that something isn't visible.
Boris also signed off on Ehsan's bug.

An in the end, all this landed in the lap of Masayuki.

Does IsElementVisible() have a bug or shouldn't it be called?

I don't know how many people are using "quote as quotation" in webmail using FF, but I could imagine that being a useful feature worth fixing for FF.

Help, hints, suggestions welcome.
Flags: needinfo?(masayuki)
Flags: needinfo?(ehsan)
Flags: needinfo?(bzbarsky)
I forgot to mention: "Paste as quotation" only fails when you try to paste a HTML fragment. If you fill your clipboard with plain text, say from Notepad++, it works since this is a different code path.
Comment 39 and comment 49 together describing when pasting quotation works and when not.
works:
- pasting plain text clipboard anywhere
- pasting html text clipboard directly into body node of the e-mail

works not:
- pasting html text clipboard into font sub node of body node of the e-mail
What element is passed to IsElementVisible() here?
Flags: needinfo?(ehsan) → needinfo?(jorgk)
Flags: needinfo?(bzbarsky)
Hi, long time, no see ;-) Thanks for the quick feedback.

(In reply to :Ehsan Akhgari from comment #51)
> What element is passed to IsElementVisible() here?
That's a good question that I can answer easily ;-) (although debugging this is a little annoying since this is called an awful lot, so I need to assure to get the right calls triggered from HTMLEditorDataTransfer.cpp#234).

When pasting as quotation close to the XXX:
blockquote@000000000B3E7EA0 type="cite" state=[40000100000] flags=[01901208] ranges:1 primaryframe=0000000000000000 refcount=9<>
Not a surprise since the blockquote is inserted at HTMLEditorDataTransfer.cpp#1653 and then we paste into it. Surely it's empty and not particularly visible (in my layman's opinion). Maybe a fix would be to not check visibility of the element is empty?

In comparison, when pasting as quotation close to the YYY:
blockquote@00000000098021C0 type="cite" state=[40000100000] flags=[01901208] ranges:1 primaryframe=00000000144A6250 refcount=10<>

But this time, IsElementVisible() returns immediately on |if (aElement->GetPrimaryFrame()) {|

Oh, check the primary frame in the XXX case, it's null.

Do it depends on where you paste whether the blockquote is classified visible or not. If it was inserted into a <font> tag, it's not visible, otherwise it has a primary frame and is visible. Hmm.

Would you like me to dump out the ancestors of the blockquote, or at least the parent or do you already have an idea of what's going on here?
Flags: needinfo?(jorgk)
Which "return false" do we take from IsElementVisible, exactly?  Does the blockquote ever end up getting a frame?
OK so the question is why does the blockquote node not have the NEEDS_NEW_FRAME flag set on it.

Here is what I would expect to happen, please check to see where in this process we go into the weeds:

1. HTMLEditor::PasteAsCitedQuotation() calls DeleteSelectionAndCreateElement() which creates the blockquote and inserts it under its parent in CreateElementTransaction::DoTransaction().
2. The presshell gets notified about the insertion and tells the frame constructor about it: <http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/base/PresShell.cpp#4334>
3. The frame constructor calls MaybeConstructLazily here: <http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/base/nsCSSFrameConstructor.cpp#7390>
4. The NODE_NEEDS_FRAME flag is set there <http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/base/nsCSSFrameConstructor.cpp#7114>.

One possibility is that we're returning early from MaybeConstructLazily here <http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/base/nsCSSFrameConstructor.cpp#7047> because the child is editable (we disable lazy frame construction for editable elements.)  But if this happens the frame constructor must be creating a frame for the blockquote element which would make the future call to GetPrimaryFrame() not return null.  The element is somehow not getting a frame nor a NODE_NEEDS_FRAME flag which means something in this setup isn't working as expected.
I think I said this in comment #48. This one here:
https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/editor/libeditor/EditorBase.cpp#3459

I'll answer the second question as soon as I understand it, help me ;-)
Sorry, comment #55 was in answer to Boris' question from comment #53.
Ehsan, thanks for the input. Surely I can continue debugging, but perhaps it would be more efficient if you did, unless it's meant as an educational exercise for me ;-) I mean, writing those comments would take more time than looking at it in the debugger yourself, since you know what you're looking for.
> I think I said this in comment #48. This one here:

Ah.  And cur is the <font> element, I bet?

I expect what happens is this:

1)  We go to insert the blockquote element (a block).
2)  The insertion parent is <font> (an inline with no block kids yet).
3)  We get to nsCSSFrameConstructor::ContentRangeInserted.
4)  We build the frame construction item list.
5)  We do the checks in WipeContainingBlock and discover that IsInlineFrame(aFrame)
    is true (for the <font>), aItems.AreAllItemsInline() is false (because it
    contains the <blockquote>), IsFramePartOfIBSplit(aFrame) is false
    (because it has no block kids yet).  So we mark the nearest block ancestor
    of the <font> as needing frame reconstruction and bail out of the whole thing.

So now we have a situation in which the <font> has a frame, the <blockquote> has no frame, there is a pending "reconstruct this whole subtree" on the nearest block ancestor of the <font> that _will_ create a frame for the <blockquote>, and the code in IsElementVisible() is relying on invariants that just don't hold.
In reply to Ehsan's comment #54.

Hmm, I have a breakpoint on nsCSSFrameConstructor::ContentAppended().

Pasting the HTML fragment next to the XXX doesn't break there, pasting next to the YYY does.
Pasting "plain text" next to the XXX also breaks there.

So when pasting doesn't work, nsCSSFrameConstructor::ContentAppended() is not called.

Next, I'll answer Boris' question ;-)
If you're going to be answering my question anyway, check whether we end up reaching the "break" statement at http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/base/nsCSSFrameConstructor.cpp#12538 with aFrame->mContent being the <font>?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #58)
> Ah.  And cur is the <font> element, I bet?
Nope, the body itself:

    if (cur->GetPrimaryFrame()) {
      if (!haveLazyBitOnChild) {
        // Our ancestor directly under |cur| doesn't have lazy bits;
        // that means we won't get a frame
        cur->List();
        aElement->List();
        return false;
      }

body@0000000009384A20 state=[2040000080004] flags=[00100008] primaryframe=00000000109275A0 refcount=18<
  Text@000000000C2A98F0 flags=[00000000] primaryframe=0000000010987B50 refcount=6<\u000a>
  div@000000000C2A9980 style="background-color:#eee;border:1px solid #000;padding:10px; height:200px;" contenteditable="" state=2040000100006] flags=[01900200] primaryframe=0000000000000000 refcount=666<
[... stuff deleted ...]
>
blockquote@0000000008F561C0 type="cite" state=[40000100000] flags=[01901208] ranges:1 primaryframe=0000000000000000 refcount=9<>

You really lost me with 1) to 5) from your comment #58 ;-(

Please let me know if I can assist further, but I think I don't have the ghost of a chance to fix this myself. Clearing NI for Masayuki for now.
Wires crossed again, now answering comment #60 ;-) Stand by.
Yes, the break gets hit with this stack:

xul.dll!nsCSSFrameConstructor::WipeContainingBlock() Line 12538	C++
xul.dll!nsCSSFrameConstructor::ContentRangeInserted() Line 8103	C++
xul.dll!nsCSSFrameConstructor::ContentInserted() Line 7670	C++
xul.dll!mozilla::PresShell::ContentInserted() Line 4362	C++
xul.dll!nsNodeUtils::ContentInserted() Line 201	C++
xul.dll!nsINode::doInsertChildAt() Line 1637	C++
xul.dll!mozilla::dom::FragmentOrElement::InsertChildAt() Line 1149	C++
xul.dll!nsINode::ReplaceOrInsertBefore() Line 2518	C++
xul.dll!nsINode::InsertBefore() Line 1804	C++
xul.dll!mozilla::CreateElementTransaction::DoTransaction() Line 89	C++
xul.dll!nsTransactionItem::DoTransaction() Line 156	C++
xul.dll!nsTransactionManager::BeginTransaction() Line 661	C++
xul.dll!nsTransactionManager::DoTransaction() Line 74	C++
xul.dll!mozilla::EditorBase::DoTransaction(nsITransaction * aTxn) Line 734	C++
xul.dll!mozilla::EditorBase::CreateNode() Line 1390	C++
xul.dll!mozilla::EditorBase::DeleteSelectionAndCreateElement() Line 4081	C++
xul.dll!mozilla::HTMLEditor::PasteAsCitedQuotation() Line 1653	C++
xul.dll!mozilla::HTMLEditor::PasteAsQuotation() Line 1626	C++

Would you like to take this to IRC? I have another 30 minutes before I need to go to a meeting.
Flags: needinfo?(masayuki)
Attached patch Possible fix (obsolete) — — Splinter Review 
If Boris' theory is correct (which it looks to be) then I think this patch fixes the bug.  Unfortunately I can't test it right now cause I'm on Mac without a mouse with a middle click button, and I don't have the time to convert it into a landable format (with tests and everything), but hopefully this is enough as a starting point for whoever ends up taking this.
Attachment #8828462 - Attachment is patch: true
Attachment #8828462 - Attachment mime type: text/x-patch → text/plain
Thanks Ehsan, this works, briefly tested with TB.

What sort of test would you like? A Mochitest that really tests the paste as quotation into a <font> tag as reported? Or something else?

What's your feeling, will the patch break other things? And who's a good reviewer, Masayuki?

With those three answers I'll leave you alone (I hope) ;-)
Also working in FF (as expected).
Ehsan, please kindly answer my questions from comment #65:
Please describe the test you want in a few words and tell me who should review it. Perhaps Boris for your code patch and you or Masayuki for the test? Could you also please give me a good commit message or is the one I chose OK?

Meanwhile I started a limited try run to see whether the code change affects anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e212b172d0becff386ef6d14aa2600570f0f11
Flags: needinfo?(ehsan)
I'll write a test as soon as Ehsan lets me know what he has in mind.
Attachment #8828462 - Attachment is obsolete: true
Attachment #8828692 - Flags: review?(bzbarsky)
Try 100% green, so with a test according to Ehsan's specifications, that could land.
(In reply to Jorg K (GMT+1) from comment #65)
> Thanks Ehsan, this works, briefly tested with TB.

Sweet!  The credit, FWIW, goes to Boris.  :-)

> What sort of test would you like? A Mochitest that really tests the paste as
> quotation into a <font> tag as reported? Or something else?

I think a mochitest that pastes as quotation in a <span> would be ideal (since this shouldn't be specific to <font>, any inline element would be similarly affected.)  In order to not have to muck with Ctrl+middle click, I suggest just running the cmd_pasteQuote command using SpecialPowers.doCommand().

> What's your feeling, will the patch break other things? And who's a good
> reviewer, Masayuki?

I don't actually think the patch is super risky, it just makes us adhere to a more correct invariant.  For this one I think someone who knows layout would be the best.  Since Boris effectively suggested the patch, try :mats.
Flags: needinfo?(ehsan)
Attachment #8828692 - Flags: review?(bzbarsky) → review?(mats)
Comment on attachment 8828692 [details] [diff] [review]
795418-paste-as-quotation.patch - fix by Ehsan

Hmm, isn't this code supposed to check that the node has a frame, or will have
a frame in the future?
I don't see why the added bit checks would guarantee that.
IIUC, they only say that there is a pending style change:
http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/base/Element.h#86-95
so I'm guessing these bits are also set for say a change in 'color', which doesn't
imply frame construction.
Attachment #8828692 - Flags: review?(mats) → review-
> Flags: review?(mats@mozilla.com) → review-
Oh, what happened to a possible fix just now :-(
Where do we go from here?
Perhaps the editor should just flush pending style changes and then check for
the presence of frames instead?
It seems fragile to try to do editor operations with pending style changes.

Alternatively, maybe we could pass aAsyncInsert = false to RecreateFramesForContent
from WipeContainingBlock when the insertion parent is editable?
Flags: needinfo?(ehsan)
Here RecreateFramesForContent() is called with REMOVE_FOR_RECONSTRUCTION.  This causes us to set reconstruct to true here <http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/base/nsCSSFrameConstructor.cpp#9743> which causes us to post a restyle event here <http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/base/nsCSSFrameConstructor.cpp#9756>.  This is where the flag setting happens: <http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/base/RestyleTracker.h#335>.  And Mats is right that this can mean either a restyle or a reframe.  Perhaps we can also set a special flag there for the nsChangeHint_ReconstructFrame.  This should help Jorg or someone else to write a better fix.

(In reply to Mats Palmgren (:mats) from comment #73)
> Perhaps the editor should just flush pending style changes and then check for
> the presence of frames instead?
> It seems fragile to try to do editor operations with pending style changes.

The IsEditable() function is too popular unfortunately so I think doing that will have performance implications.  Most of the editor operations shouldn't depend on the result of reflows, and in the ideal case we finish the editing without flushing and letting only one flush to happen for the next repaint.  (Also the editor code has been historically quite unsafe in the face of unexpected flushes.)

> Alternatively, maybe we could pass aAsyncInsert = false to
> RecreateFramesForContent
> from WipeContainingBlock when the insertion parent is editable?

Doesn't that force us into the eager frame construction codepath always?  That also seems undesirable from a performance perspective...
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #74)
> Perhaps we can also set a special flag there for the nsChangeHint_ReconstructFrame.

I don't think a pending ReconstructFrame on an ancestor node actually implies
that all nodes under it will have frames.

> (In reply to Mats Palmgren (:mats) from comment #73)
> > Perhaps the editor should just flush pending style changes and then check for
> > the presence of frames instead?
> 
> The IsEditable() function is too popular unfortunately so I think doing that
> will have performance implications.

I meant in one of the callers higher up in the stack in comment 48, e.g.
InsertFromTransferable or Paste.  This strategy assumes that there are
some editor methods that can be considered API entry points where we can
insert flush calls.

> Most of the editor operations shouldn't
> depend on the result of reflows, and in the ideal case we finish the editing
> without flushing and letting only one flush to happen for the next repaint. 

I meant Flush_Frames (a.k.a Flush_Style), not Flush_Layout.
So Reflow shouldn't be necessary.

> > Alternatively, maybe we could pass aAsyncInsert = false to
> > RecreateFramesForContent
> > from WipeContainingBlock when the insertion parent is editable?
> 
> Doesn't that force us into the eager frame construction codepath always? 

Yes, but only for editable nodes.

> That also seems undesirable from a performance perspective...

I'd be surprised if either flushing frames or eager frame construction
have a noticeable performance impact on the editing operations we're
discussing.  And it shouldn't affect any non-editable node code paths
at all.
Thanks for the comments. How about you suggest a patch? I'm happy to write the test Ehsan described in comment #70, or any other test for that matter.

(In reply to :Ehsan Akhgari from comment #74)
> This should help Jorg or someone else to write a better fix.
I'm really out of my depth here :-(
A mochitest would be much appreciated either way.  There are likely tests under
editor/libeditor/tests/ that you can crib / extend.

I would probably try the "eager frame construction" alternative and see how
that performs.  Try passing !aFrame->GetContent()->IsEditable() instead
of 'true' in the call to RecreateFramesForContent at the end of
WipeContainingBlock and see if that fixes this bug?
This works, too. In this case I'm not even able to come up with a commit message since I don't know what you're doing here. If the try works out, can you get a review for it, please. I'll do the test.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0a58d19eefe9f81e0be45c0288e9a36f9f367c2
This try is 100% green. Please get the patch reviewed and I'll do a test.
So here is a test, which fails without the code patch applied and passes with the code patch applied.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8829140 - Flags: review?(mats)
Attached patch fix — — Splinter Review 
There's an analysis of the issue in comment 58, in case you forgot.
Attachment #8828692 - Attachment is obsolete: true
Attachment #8829086 - Attachment is obsolete: true
Attachment #8829183 - Flags: review?(bzbarsky)

        Attachment #8829140 -
        Attachment is obsolete: true

        Attachment #8829140 -
        Flags: review?(mats)

        Attachment #8829184 -
        Flags: review+

    
    

    
  


  
I found one more case that triggers an error in editor: the display:table
branch in WipeContainingBlock.  I've included tests for display:grid and
display:ruby branches as well, but those didn't trigger any editor error.

(Also stumbled into an editor crash which I'm fixing separately: bug 1332876)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfb738292dcb211a599827ef60f6ad63f57ec285

    
    

    
  
Hey Mats, thanks a million for all your work here!
(It must sound terribly ungrateful mentioning that a trailing space slipped into your code patch.)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Alternative fix
        — 
          — Splinter Review
      

    


  
It looks to me that this problem is limited to the editor methods
that insert a wrapper element and then paste into that.  So I think
I prefer this fix instead.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f736be4ef4ea4cdb75bc1357700ce91a2982efc

        Attachment #8829320 -
        Flags: review?(ehsan)

    
  

        Attachment #8829183 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8829320 [details] [diff] [review]
Alternative fix

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

The patch looks fine to me but I don't review editor code any more.

::: editor/libeditor/HTMLEditorDataTransfer.cpp
@@ +1668,5 @@
>    rv = selection->Collapse(newNode, 0);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Ensure that the inserted <blockquote> has a frame to make it IsEditable.
> +  FlushFrames();

Perhaps you want to do this at the end of DeleteSelectionAndCreateElement() instead as opposed to in all of the callers?

        Attachment #8829320 -
        Flags: review?(ehsan) → review?(masayuki)

    
    

    
  
(In reply to :Ehsan Akhgari from comment #86)
> Perhaps you want to do this at the end of DeleteSelectionAndCreateElement()
> instead as opposed to in all of the callers?

It's actually not DeleteSelectionAndCreateElement that needs the frames;
it's the code that follows those calls that adds children into the newly added
element that requires a frame for an IsEditable check.  So from a correctness
POV, the flush should be in the caller.  (A hypothetical new caller might not
need that flush.)

That said, these three calls are the only ones in the entire tree, so I guess
we could make DeleteSelectionAndCreateElement a private convenience method on
HTMLEditorDataTransfer instead, and then it might make sense to make it
do the flush for convenience.

I don't have a strong opinion either way though, whatever Masayuki-san
prefers is fine with me. :-)

    
    

    
  
Comment on attachment 8829320 [details] [diff] [review]
Alternative fix

Although, this is pretty hard to review to me.

Looks okay, but perhaps, this must not be a perfect solution since this patch won't work if there is a script blocker. But probably, this won't cause new regression for existing add-ons because in such case, FlushFrames() does nothing actually (so, following methods work as current Nightly).

        Attachment #8829320 -
        Flags: review?(masayuki) → review+

    
    

    
  
You're going to land this soon ;-)
Flags: needinfo?(mats)

    
  
Flags: in-testsuite+

    
  
Flags: needinfo?(mats)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/045a0995bdec
https://hg.mozilla.org/mozilla-central/rev/2ea70ba06fa9
https://hg.mozilla.org/mozilla-central/rev/c66216e85022
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 7 years ago

          status-firefox54:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

    
    

    
  
Looks like a pretty straightforward fix with tests. I'm betting the TB folks would like to get this into 52 for their next release, so let's proceed with Aurora/Beta approval requests unless there's a good reason for not doing so.
Assignee: jorgk → mats

          status-firefox51:
          --- → wontfix

          status-firefox52:
          --- → affected

          status-firefox53:
          --- → affected
Flags: needinfo?(mats)

    
    

    
  
It's a reasonable low-risk patch, so Aurora uplift seems fine to me.

I'm less sure about Beta because there isn't much benefit in this fix for
Firefox.  If I weigh the risk vs benefit for Firefox only, I wouldn't take
it on Beta.  OTOH, it does seem worth it for TB though.
I'll leave the decision on Beta uplift to others.
Flags: needinfo?(mats)

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM] from comment #92)
> Looks like a pretty straightforward fix with tests. I'm betting the TB folks
> would like to get this into 52 for their next release, so let's proceed with
> Aurora/Beta approval requests unless there's a good reason for not doing so.
Thanks for keeping us in mind. Yes, we'd like this. The fact is, as we did for TB 38 ESR and TB 45 ESR, we'll have a special Thunderbird release branch on mozilla-esr52 and we'll include the patch there. But if it goes onto "main" mozilla-esr52, that would save us a lot of work. Who's filling in the uplift request.

    
    

    
  
Sorry, that was a question: Who's filling in the uplift request?

    
    

    
  
Comment on attachment 8829320 [details] [diff] [review]
Alternative fix

Approval Request Comment
[Feature/Bug causing the regression]: Bug 612128 (mozilla15 from 2012)
[User impact if declined]: Paste as quotations doesn't work. Impact for Thunderbird users, but also not working for FF users editing webmail.
[Is this code covered by automated tests?]: Yes, plenty of them.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: See below.
[Why is the change risky/not risky?]: See below.
[String changes made/needed]: None.

For the risk assessment, let's read the patch author's comment #93 (quote):
  It's a reasonable low-risk patch, so Aurora uplift seems fine to me.
  I'm less sure about Beta because there isn't much benefit in this fix for
  Firefox.  If I weigh the risk vs benefit for Firefox only, I wouldn't take
  it on Beta.  OTOH, it does seem worth it for TB though.
  I'll leave the decision on Beta uplift to others.

Indeed there is a good benefit for Thunderbird users. We're early in the cycle, should this "low-risk" patch cause any problems, it can always be backed out.

TB will include this fix in TB 52 ESR by using a branch on mozilla-esr52, but of course an uplift to mozilla52 would save the TB release manager (me) a whole lot of work ;-)

        Attachment #8829320 -
        Flags: approval-mozilla-beta?

        Attachment #8829320 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8829320 [details] [diff] [review]
Alternative fix

This fix has been on Nightly54 for a few days, seems safe to uplift to Aurora53

        Attachment #8829320 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Comment on attachment 8829320 [details] [diff] [review]
Alternative fix

This has survived a week in aurora, let's get it in beta now for TB's benefit

        Attachment #8829320 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 1 for beta
        — 
          — Splinter Review
      

    


  
Here you go.  I tried to land it but I got:
abort: push creates new remote head 7831e71bd146 on branch 'COMM2000_20110114_RELBRANCH'!

So I'll leave that to you :-)
Flags: needinfo?(mats) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: