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

RESOLVED FIXED in Firefox 52

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: HP Korn, Assigned: mats)

Tracking

({regression, testcase})

Trunk
mozilla54
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(6 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
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
Comment hidden (obsolete)
(Reporter)

Updated

5 years ago
Version: 15 → 16
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Duplicate of this bug: 806235
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)

Updated

7 months ago
Flags: needinfo?(hape)
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 42

7 months ago
Created attachment 8827811 [details]
Simple HTML page with instructions showing the problem

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

Updated

7 months ago
Whiteboard: [STR comment 35]

Comment 43

7 months ago
I made *all* comments obsolete, since they are. All you need is the simple example that shows the bug in Firefox.

Comment 44

7 months ago
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)

Comment 45

7 months ago
(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)

Updated

7 months ago
Blocks: 612128
Keywords: regression

Comment 46

7 months ago
Alice, thank you so much. I'm impressed and surprised every time you help us. That helps a great deal.

Comment 47

7 months ago
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.

Comment 48

7 months ago
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)

Comment 49

7 months ago
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 50

7 months ago
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)

Comment 52

7 months ago
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.

Comment 55

7 months ago
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 ;-)

Comment 56

7 months ago
Sorry, comment #55 was in answer to Boris' question from comment #53.

Comment 57

7 months ago
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.

Comment 59

7 months ago
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>?

Comment 61

7 months ago
(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.

Comment 62

7 months ago
Wires crossed again, now answering comment #60 ;-) Stand by.

Comment 63

7 months ago
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.

Updated

7 months ago
Flags: needinfo?(masayuki)
Created attachment 8828462 [details] [diff] [review]
Possible fix

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

Comment 65

7 months ago
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) ;-)

Comment 66

7 months ago
Also working in FF (as expected).

Comment 67

7 months ago
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)

Comment 68

7 months ago
Created attachment 8828692 [details] [diff] [review]
795418-paste-as-quotation.patch - fix by 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)

Comment 69

7 months ago
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)
(Assignee)

Comment 71

7 months ago
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-

Comment 72

7 months ago
> Flags: review?(mats@mozilla.com) → review-
Oh, what happened to a possible fix just now :-(
Where do we go from here?
(Assignee)

Comment 73

7 months ago
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)
(Assignee)

Comment 75

7 months ago
(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.

Comment 76

7 months ago
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 :-(
(Assignee)

Comment 77

7 months ago
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?

Comment 78

7 months ago
Created attachment 8829086 [details] [diff] [review]
795418-WipeContainingBlock.patch - fix by Mats

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

Comment 79

7 months ago
This try is 100% green. Please get the patch reviewed and I'll do a test.

Comment 80

7 months ago
Created attachment 8829140 [details] [diff] [review]
795418-paste-as-quotation-test.patch (v1).

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)
(Assignee)

Comment 81

7 months ago
Created attachment 8829183 [details] [diff] [review]
fix

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)
(Assignee)

Comment 82

7 months ago
Created attachment 8829184 [details] [diff] [review]
Jorg's testcase with some minor editorial corrections
Attachment #8829140 - Attachment is obsolete: true
Attachment #8829140 - Flags: review?(mats)
Attachment #8829184 - Flags: review+
(Assignee)

Comment 83

7 months ago
Created attachment 8829189 [details] [diff] [review]
More tests that hits other branches in WipeContainingBlock

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

Comment 84

7 months ago
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.)
(Assignee)

Comment 85

7 months ago
Created attachment 8829320 [details] [diff] [review]
Alternative fix

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)
(Assignee)

Updated

7 months ago
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)
(Assignee)

Comment 87

7 months ago
(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+

Comment 89

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

Comment 90

7 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/045a0995bdec
Ensure that the inserted wrapper element has a frame to make it IsEditable.  r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea70ba06fa9
test. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66216e85022
More tests.
(Assignee)

Updated

7 months ago
Flags: in-testsuite+
(Assignee)

Updated

7 months ago
Flags: needinfo?(mats)

Comment 91

7 months ago
bugherder
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
Last Resolved: 5 years ago7 months 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)
(Assignee)

Comment 93

7 months ago
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)

Comment 94

7 months ago
(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.

Comment 95

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

Comment 96

7 months ago
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 97

7 months ago
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 98

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/385b4f5d1c49
https://hg.mozilla.org/releases/mozilla-aurora/rev/cfff8883bbed
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a28269883ab
status-firefox53: affected → fixed
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+

Comment 100

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/86dede1ab4b3
https://hg.mozilla.org/releases/mozilla-beta/rev/8b37cad9ed3b
https://hg.mozilla.org/releases/mozilla-beta/rev/2c8c1e192196
status-firefox52: affected → fixed
Backed out for bustage. Looks like this needs rebasing around bug 1328832.
https://treeherder.mozilla.org/logviewer.html#?job_id=75609652&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/25ec6b657dce
status-firefox52: fixed → affected
Flags: needinfo?(mats)
(Assignee)

Comment 102

7 months ago
Created attachment 8835802 [details] [diff] [review]
part 1 for beta

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)

Comment 103

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4bd450fe3834
https://hg.mozilla.org/releases/mozilla-beta/rev/8369d14b575e
https://hg.mozilla.org/releases/mozilla-beta/rev/91cb8bf2804e
status-firefox52: affected → fixed
Flags: needinfo?(ryanvm)

Comment 104

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/4bd450fe3834
https://hg.mozilla.org/releases/mozilla-esr52/rev/8369d14b575e
https://hg.mozilla.org/releases/mozilla-esr52/rev/91cb8bf2804e
status-firefox-esr52: --- → fixed
You need to log in before you can comment on or make changes to this bug.