1119503 - We should inject a new line when we encouter a block boundary when copying pre-formated text

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Benoit Girard (:BenWa)]

Bug from discussion with ehsan.

As an example:

<div style="white-space: pre;">
  <div>foo</div>
  <div>  bar</div>
</div>

Comment 2

[(no longer active)]

Another test case is http://beta.etherpad.org/ (see bug 1117007).

I have a fix.

Comment 3

[(no longer active)]

[Tracking Requested - why for this release]:
Part of the fix here fixes a regression from bug 116083, which was fixed in Firefox 37 which breaks websites such as http://beta.etherpad.org/.  We should backport the fix here.

Comment 4

[(no longer active)]

Attachment: Part 1: Fix the editor code to recognize a br element with no siblings as visible

Comment 5

[(no longer active)]

Attachment: Part 1: Determine whether an element is a block element based on the style, not the tag

This probably fixes a whole bunch of edge cases where content uses
elements other than div.

Comment 6

[(no longer active)]

Attachment: Part 3: Insert a line break between preformatted block boundaries when creating raw output

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b710d5fbdf6

These patches change what gets copies from the Web Console by inserting a newline between the message and the filename.  This happens because the message is in a span that is display:block, and the file name is in a display:flex element after it, so we consider that a block boundary.  I think given that markup, that is the right thing to do in the general case, but in this case Web Console definitely wants something else.

Patrick, can you please help me find someone who can come up with an alternate markup with the same visuals but without putting the message in a preformatted block?  Thanks!  :-)

Comment 9

[Patrick Brosset <:pbro>]

Panos is probably the one who knows the console the best now, and Brian did a lot of css/html changes in many places of the toolbox, one of them will be able to help you.

What does cause the newline to be inserted? The fact that the span is display:block? or the preformatting?

Comment 10

[(no longer active)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> Panos is probably the one who knows the console the best now, and Brian did
> a lot of css/html changes in many places of the toolbox, one of them will be
> able to help you.

Thanks!  Panos, can you please help with this?

> What does cause the newline to be inserted? The fact that the span is
> display:block? or the preformatting?

Both of them together.  The goal is for us to insert line endings between the divs in the test case in comment 0, for example, or in http://beta.etherpad.org/.

Comment 11

[(no longer active)]

Attachment: Part 3: Change test_aboutmemory.xul in order to not expect a newline at the very end of the copied text

After the patches in this bug, we only inject a newline at block element
boundaries.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

I'm swamped right now, but perhaps Brian can take a look?

The code in question is in createMessageNode:

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/webconsole.js#2505

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8551276 [details] [diff] [review]
Part 3: Change test_aboutmemory.xul in order to not expect a newline at the very end of the copied text

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

I'm surprised there are so few changes -- there are numerous tests in toolkit/components/aboutmemory/tests/ that have similar "End of ..." strings. But... r=me for whatever makes it pass the tests :)

Comment 14

[(no longer active)]

(In reply to Panos Astithas [:past] from comment #12)
> I'm swamped right now, but perhaps Brian can take a look?
> 
> The code in question is in createMessageNode:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
> webconsole.js#2505

Please note that this patch series fixes a regression in Firefox 37, so we are going to need to take these patches, preferably before next week while I'm still around.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: console-message-pre-WIP.patch

The message is in a preformatted block so that console.log("foo\nbar") will display as formatted.  I don't want to change that behavior, but I believe this fixes the problem and will maintain the display of output.

It will take more testing to confirm that there is never text directly added as a child of message-body, but before I do that I want to make sure something like this would be a possible solution to the problem.  What do you think?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #8)
> These patches change what gets copies from the Web Console by inserting a
> newline between the message and the filename.  This happens because the
> message is in a span that is display:block, and the file name is in a
> display:flex element after it, so we consider that a block boundary.  I
> think given that markup, that is the right thing to do in the general case,
> but in this case Web Console definitely wants something else.

Tangentially, it seems like this same problem happen with web content or other markup/css within the browser chrome.  In that case, it seems these patches will change the copy behavior of that content as well - is that a problem?

Comment 17

[(no longer active)]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> needinfo? me!) from comment #8)
> > These patches change what gets copies from the Web Console by inserting a
> > newline between the message and the filename.  This happens because the
> > message is in a span that is display:block, and the file name is in a
> > display:flex element after it, so we consider that a block boundary.  I
> > think given that markup, that is the right thing to do in the general case,
> > but in this case Web Console definitely wants something else.
> 
> Tangentially, it seems like this same problem happen with web content or
> other markup/css within the browser chrome.  In that case, it seems these
> patches will change the copy behavior of that content as well - is that a
> problem?

Yes, there is definitely a change in how Web content gets copied as a result of this, but I think in almost all cases the change will be desired.  It is the exact layout of the Web Console which puts a flex box item to the right edge that is confusing us.

I'll test your patch in a moment...  Thanks a lot for writing it.  :-)

Comment 18

[(no longer active)]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Created attachment 8551909 [details] [diff] [review]
> console-message-pre-WIP.patch
> 
> The message is in a preformatted block so that console.log("foo\nbar") will
> display as formatted.  I don't want to change that behavior, but I believe
> this fixes the problem and will maintain the display of output.
> 
> It will take more testing to confirm that there is never text directly added
> as a child of message-body, but before I do that I want to make sure
> something like this would be a possible solution to the problem.  What do
> you think?

Unfortunately, with your patch the following tests still fail:

browser/devtools/webconsole/test/browser_webconsole_bug_587617_output_copy.js
browser/devtools/webconsole/test/browser_webconsole_bug_613280_jsterm_copy.js

But this seems to have fixed this test failure: browser/devtools/webconsole/test/browser_webconsole_output_copy_newlines.js.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8550781 [details] [diff] [review]
Part 1: Fix the editor code to recognize a br element with no siblings as visible

Hmm.  This feels a bit fishy, but ok.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8550782 [details] [diff] [review]
Part 1: Determine whether an element is a block element based on the style, not the tag

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8550783 [details] [diff] [review]
Part 3: Insert a line break between preformatted block boundaries when creating raw output

I don't understand this patch.  OutputRaw is really supposed to mean "no formatting".  Why are we changing that?  What's the caller that passes that flag but doesn't mean it?

Comment 22

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> Comment on attachment 8550783 [details] [diff] [review]
> Part 3: Insert a line break between preformatted block boundaries when
> creating raw output
> 
> I don't understand this patch.  OutputRaw is really supposed to mean "no
> formatting".  Why are we changing that?  What's the caller that passes that
> flag but doesn't mean it?

This caller: <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsCopySupport.cpp#100>  This is one of the cases where it's not clear how OutputRaw and OutputPreformatted are supposed to work together...

Comment 23

[(no longer active)]

So I've been looking at the try test failures in dom/imptest/editing caused by part 1 here.  Looking deeply into this issue, the code that I wrote back in attachment 488704 [details] [diff] [review] which uses the editor to guess whether a br element is visible is completely wrong.  :(  This can cause sites such as beta etherpad to not count <p><br></p> as a line-break, which means if you copy "a\n\nb" from etherpad and paste it again you will get "a\nb".  And of course if you copied the original text from a non-editable document the behavior would be correct, which means we're inconsistent too.

Is there a better way to know if a br element causes a line break or is collapsed?  At the lack of that, we may want to fix bug 611103, but that will probably not be the ideal fix since we'd still be guessworking the visibility of the br node out of the DOM disregarding the layout information.

And of course such a thing would not be backportable, so we may have to live with the brokenness I described above for Firefox 37, which is terrible.

Boris, do you have any better ideas?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> This is one of the cases where it's not clear how OutputRaw and OutputPreformatted are
> supposed to work together

Oh, right, and we have nice XXX comments...

So is it the case that before your patches specifying both together was just the same as OutputRaw?  If not, then I'm OK with claiming that OutputPreformatted overrides OutputRaw.  OutputRaw is slightly weird anyway where the plaintext serializer is concerned.

> Is there a better way to know if a br element causes a line break or is collapsed?

Just based on the element, or are we allowed to look at the frame?  In the latter case I'd look at frame tree dumps to see what the collapsed vs not BRFrames look like.  I tried to poke about the line layout and block code to see how we handle collapsing of consecutive <br> and whatnot, and got totally lost.

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #18)
> (In reply to Brian Grinstead [:bgrins] from comment #15)
> > Created attachment 8551909 [details] [diff] [review]
> > console-message-pre-WIP.patch
> > 
> > The message is in a preformatted block so that console.log("foo\nbar") will
> > display as formatted.  I don't want to change that behavior, but I believe
> > this fixes the problem and will maintain the display of output.
> > 
> > It will take more testing to confirm that there is never text directly added
> > as a child of message-body, but before I do that I want to make sure
> > something like this would be a possible solution to the problem.  What do
> > you think?
> 
> Unfortunately, with your patch the following tests still fail:
> 
> browser/devtools/webconsole/test/browser_webconsole_bug_587617_output_copy.js
> browser/devtools/webconsole/test/browser_webconsole_bug_613280_jsterm_copy.js
> 
> But this seems to have fixed this test failure:
> browser/devtools/webconsole/test/browser_webconsole_output_copy_newlines.js.

So in browser_webconsole_bug_587617_output_copy.js, it looks like the call to win.getSelection() is returning with a newline with that new CSS applied:

"Hello world! bug587617"
browser_webconsole_bug_587617_output_copy.js:41:2

Although the clipboard text returns as expected:

"Hello world! bug587617" browser_webconsole_bug_587617_output_copy.js:41:2

Interestingly, with my new CSS applied but none of the other patches from this bug included, this test passes (the win.getSelection() does not include the newline).  Is it expected that these patches would modify the behavior of getSelection in this way?

Comment 26

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #24)
> > This is one of the cases where it's not clear how OutputRaw and OutputPreformatted are
> > supposed to work together
> 
> Oh, right, and we have nice XXX comments...
> 
> So is it the case that before your patches specifying both together was just
> the same as OutputRaw?

No, why do you think that?  I _think_ specifying both OutputRaw and OutputFormatted is the same as just OutputRaw.

> If not, then I'm OK with claiming that
> OutputPreformatted overrides OutputRaw.  OutputRaw is slightly weird anyway
> where the plaintext serializer is concerned.
> 
> > Is there a better way to know if a br element causes a line break or is collapsed?
> 
> Just based on the element, or are we allowed to look at the frame?  In the
> latter case I'd look at frame tree dumps to see what the collapsed vs not
> BRFrames look like.  I tried to poke about the line layout and block code to
> see how we handle collapsing of consecutive <br> and whatnot, and got
> totally lost.

I asked dholbert on IRC about this today, and he suggested looking at the BSize() of the frame, and see if it's non-zero.  Based on what I can claim to understand from the reflow code for the br frame, that seems sensible.  I'll give it a try.

Comment 27

[(no longer active)]

(In reply to Brian Grinstead (unavailable through 1-26) [:bgrins] from comment #25)
> (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> needinfo? me!) from comment #18)
> > (In reply to Brian Grinstead [:bgrins] from comment #15)
> > > Created attachment 8551909 [details] [diff] [review]
> > > console-message-pre-WIP.patch
> > > 
> > > The message is in a preformatted block so that console.log("foo\nbar") will
> > > display as formatted.  I don't want to change that behavior, but I believe
> > > this fixes the problem and will maintain the display of output.
> > > 
> > > It will take more testing to confirm that there is never text directly added
> > > as a child of message-body, but before I do that I want to make sure
> > > something like this would be a possible solution to the problem.  What do
> > > you think?
> > 
> > Unfortunately, with your patch the following tests still fail:
> > 
> > browser/devtools/webconsole/test/browser_webconsole_bug_587617_output_copy.js
> > browser/devtools/webconsole/test/browser_webconsole_bug_613280_jsterm_copy.js
> > 
> > But this seems to have fixed this test failure:
> > browser/devtools/webconsole/test/browser_webconsole_output_copy_newlines.js.
> 
> So in browser_webconsole_bug_587617_output_copy.js, it looks like the call
> to win.getSelection() is returning with a newline with that new CSS applied:
> 
> "Hello world! bug587617"
> browser_webconsole_bug_587617_output_copy.js:41:2
> 
> Although the clipboard text returns as expected:
> 
> "Hello world! bug587617" browser_webconsole_bug_587617_output_copy.js:41:2
> 
> Interestingly, with my new CSS applied but none of the other patches from
> this bug included, this test passes (the win.getSelection() does not include
> the newline).  Is it expected that these patches would modify the behavior
> of getSelection in this way?

Nothing is modifying the return value of getSelection() in this patch.  What happens is that we expect the same thing to be copied to the clipboard as what getSelection() returns, and that's what the first argument to the waitForClipboard function in both of those tests does, and since my patch changes what gets copied to the clipboard, that assumption no longer holds, so waitForClipboard ends up waiting for the expected string to be copied forever and the test times out.

Does that help?

Comment 28

[(no longer active)]

Boris, in case the first part of comment 26 doesn't make sense, can you please suggest how you'd like me to fix this?

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I _think_ specifying both OutputRaw and OutputFormatted is the same as just OutputRaw.

Isn't that what I said?

So we would be changing this to no longer be true, right?

Comment 30

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> > I _think_ specifying both OutputRaw and OutputFormatted is the same as just OutputRaw.
> 
> Isn't that what I said?
> 
> So we would be changing this to no longer be true, right?

Yes, I think so.

Comment 31

[(no longer active)]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #27)
> (In reply to Brian Grinstead (unavailable through 1-26) [:bgrins] from
> comment #25)
> > (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> > needinfo? me!) from comment #18)
> > > (In reply to Brian Grinstead [:bgrins] from comment #15)
> > > > Created attachment 8551909 [details] [diff] [review]
> > > > console-message-pre-WIP.patch
> > > > 
> > > > The message is in a preformatted block so that console.log("foo\nbar") will
> > > > display as formatted.  I don't want to change that behavior, but I believe
> > > > this fixes the problem and will maintain the display of output.
> > > > 
> > > > It will take more testing to confirm that there is never text directly added
> > > > as a child of message-body, but before I do that I want to make sure
> > > > something like this would be a possible solution to the problem.  What do
> > > > you think?
> > > 
> > > Unfortunately, with your patch the following tests still fail:
> > > 
> > > browser/devtools/webconsole/test/browser_webconsole_bug_587617_output_copy.js
> > > browser/devtools/webconsole/test/browser_webconsole_bug_613280_jsterm_copy.js
> > > 
> > > But this seems to have fixed this test failure:
> > > browser/devtools/webconsole/test/browser_webconsole_output_copy_newlines.js.
> > 
> > So in browser_webconsole_bug_587617_output_copy.js, it looks like the call
> > to win.getSelection() is returning with a newline with that new CSS applied:
> > 
> > "Hello world! bug587617"
> > browser_webconsole_bug_587617_output_copy.js:41:2
> > 
> > Although the clipboard text returns as expected:
> > 
> > "Hello world! bug587617" browser_webconsole_bug_587617_output_copy.js:41:2
> > 
> > Interestingly, with my new CSS applied but none of the other patches from
> > this bug included, this test passes (the win.getSelection() does not include
> > the newline).  Is it expected that these patches would modify the behavior
> > of getSelection in this way?
> 
> Nothing is modifying the return value of getSelection() in this patch.  What
> happens is that we expect the same thing to be copied to the clipboard as
> what getSelection() returns, and that's what the first argument to the
> waitForClipboard function in both of those tests does, and since my patch
> changes what gets copied to the clipboard, that assumption no longer holds,
> so waitForClipboard ends up waiting for the expected string to be copied
> forever and the test times out.
> 
> Does that help?

OK, so I think I finally came up with a proper fix for bug 611103, which makes part 1 of this bug unnecessary, and changes the behavior so that those two tests don't fail any more.  Let me test this stuff on the try server and get back to you.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Yes, I think so.

OK.  What would go wrong if we just dropped the OutputRaw in nsCopySupport.cpp instead?

Comment 33

[(no longer active)]

We discussed this on IRC.  I am going to try to add a new flag called OutputForPlainTextClipboardCopy and make that do what I need instead of changing the meaning of OutputRaw.

Comment 34

[(no longer active)]

Comment on attachment 8550781 [details] [diff] [review]
Part 1: Fix the editor code to recognize a br element with no siblings as visible

This is not needed any more.

Comment 35

[(no longer active)]

Attachment: Part 2: Insert a line break between preformatted block boundaries when creating raw output

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553486 [details] [diff] [review]
Part 2: Insert a line break between preformatted block boundaries when creating raw output

r=me

Comment 37

[(no longer active)]

Attachment: devtools test failures

These are the devtools test failures with the latest version of the patches in this bug.  Brian, can you please suggest a fix?  Thanks!

Comment 38

[(no longer active)]

Comment on attachment 8550782 [details] [diff] [review]
Part 1: Determine whether an element is a block element based on the style, not the tag

Investigating the test_AddonRepository.js failure last night, we need to fall back to just look at the tag for the cases where there is no style information available at all, such as the nsHTMLFormatConverter::Convert consumer.

Comment 39

[(no longer active)]

Attachment: Part 1: Determine whether an element is a block element based on the style, not the tag

This probably fixes a whole bunch of edge cases where content uses
elements other than div.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553899 [details] [diff] [review]
Part 1: Determine whether an element is a block element based on the style, not the tag

r=me

Comment 41

[(no longer active)]

Attachment: Part 4: Add a test for serialization of block elements without style information

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554206 [details] [diff] [review]
Part 4: Add a test for serialization of block elements without style information

r=me

Comment 43

[Brian Grinstead [:bgrins]]

Attachment: console-message-pre.patch

Panos, this is changing the preformatted text to be applied to children of the message-body instead of the element itself.  Testing locally, this doesn't seem to be a problem since all output seems to be wrapped into children elements of message-body.  But are you aware of any case where text is directly added to this element, or any other reasons this may be a bad idea?

The couple of test changes are due to HUD.iframeWindow.getSelection() including a new line, but the clipboard now does not include the new line (see Comment 25 and 27).

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b22e643658

Comment 44

[(no longer active)]

With Brian's patch, the entire patch set is green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b816819253

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8554643 [details] [diff] [review]
console-message-pre.patch

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

I can't think of any case where this would be a bad idea. console.dir() is one weird case, but it seems this patch improves it (or maybe it has regressed between the try build in this bug that I've tried and fx-team tip).

To verify, type console.dir(document.body) on this page and see how the embedded variables view overflows its container on fx-team tip or nightly.

Comment 46

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #45)
> Comment on attachment 8554643 [details] [diff] [review]
> console-message-pre.patch
> 
> Review of attachment 8554643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't think of any case where this would be a bad idea. console.dir() is
> one weird case, but it seems this patch improves it (or maybe it has
> regressed between the try build in this bug that I've tried and fx-team tip).
> 

So yes, console.dir does inject the line above the variables view as a text node under message-body.  In this case, it probably is no problem that it doesn't have the white-space: pre-wrap and word-wrap: break-work set.  However, I did find one more case where text nodes are set directly under the message body element - the input line.  So if you run a multiline input:

var i = 1;
var j = 1;
i+j;

With the patch applied all of these lines get crunched into a single one in the console output, while they will show the new lines on fx-team.  I'll check to see how hard it is to just wrap that into an element.

Comment 47

[Brian Grinstead [:bgrins]]

Attachment: console-message-pre-and-wrap-simple-message.patch

This is the same as the last patch, but also wraps the Messages.Simple class rendering inside of an additional span element so that multiline input shows up correctly in the output.  I don't think this will cause any side effects since it's just wrapping the output inside of another inline child - but flagging r? just to get another set of eyes on it.

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8555339 [details] [diff] [review]
console-message-pre-and-wrap-simple-message.patch

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

LGTM

Comment 49

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af9f4a6a7b78
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9012ee428a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2129ff8897
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccef8ecf319
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ee3ae06643

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/mozilla-central/rev/b2b10231606b for Win7 PGO-only m-oth failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=5994823&repo=mozilla-inbound

Comment 51

[(no longer active)]

Ah, let's just compare the trimmed version of the strings in the about:memory tests in order to avoid this kind of dance:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d76c99657ff2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf4e1018997
https://hg.mozilla.org/integration/mozilla-inbound/rev/996ff2931452
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcaf823a0a0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0a0cf68cf0

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d76c99657ff2
https://hg.mozilla.org/mozilla-central/rev/5bf4e1018997
https://hg.mozilla.org/mozilla-central/rev/996ff2931452
https://hg.mozilla.org/mozilla-central/rev/dcaf823a0a0a
https://hg.mozilla.org/mozilla-central/rev/7f0a0cf68cf0

Comment 53

[Lawrence Mandel [:lmandel] (use needinfo)]

bz - Ehsan is away. In comment 3 he said that this impacts 37. As a reviewer, do you have concerns about uplifting this fix to Aurora 37?

Comment 54

[Andrew Overholt [:overholt]]

Ehsan asked me to ensure we uplift this to Aurora.  He said it has 3 prerequisites: bug 611103 and bug 1113238 and bug 1123062.  Once that's green on try and we have uplift approval, we'll have to land them in that order because otherwise tests can fail.

Ehsan also told me to use this as the risk analysis:

"The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment 54 seems reasonable to me.

Comment 56

[Andrew Overholt [:overholt]]

Comment on attachment 8553899 [details] [diff] [review]
Part 1: Determine whether an element is a block element based on the style, not the tag

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: copy/pasting pre-formatted text will be broken
[Describe test coverage new/current, TreeHerder]: for this patch, just try/time on m-c
[Risks and why]: ehsan says:
"The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none

Comment 57

[Andrew Overholt [:overholt]]

Comment on attachment 8553486 [details] [diff] [review]
Part 2: Insert a line break between preformatted block boundaries when creating raw output

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: weaker plain text (pre-formatted) copy/pasting
[Describe test coverage new/current, TreeHerder]: improved tests in this patch + try/time on m-c
[Risks and why]: "The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none

Comment 58

[Andrew Overholt [:overholt]]

Comment on attachment 8551276 [details] [diff] [review]
Part 3: Change test_aboutmemory.xul in order to not expect a newline at the very end of the copied text

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: none but we'll have a test failure because of other changes here (to improve copy/pasting pre-formatted text)
[Describe test coverage new/current, TreeHerder]: this is just fixing a test
[Risks and why]: "The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none

Comment 59

[Andrew Overholt [:overholt]]

Comment on attachment 8554206 [details] [diff] [review]
Part 4: Add a test for serialization of block elements without style information

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: reduced test coverage for the changes here
[Describe test coverage new/current, TreeHerder]: this is adding a test \o/
[Risks and why]: "The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none

Comment 60

[Andrew Overholt [:overholt]]

Comment on attachment 8555339 [details] [diff] [review]
console-message-pre-and-wrap-simple-message.patch

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: incorrect line breaks in copied/pasted text (in the console)
[Describe test coverage new/current, TreeHerder]: fixed test here but otherwise try + time on m-c
[Risks and why]: on the broader Gecko changes, Ehsan said "The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none

Comment 61

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8553899 [details] [diff] [review]
Part 1: Determine whether an element is a block element based on the style, not the tag

I'm accepting Ehsan's request to uplift the set of patches on this bug to Aurora. My understanding is that the copy/paste issue that this fixes is quite irritating for some users. Although there are 5 patches, the changesets are small, not very scary, and the patches include test related changes as well.

Note for sheriff: The prereqs for this bug, bug 611103, bug 1113238 and bug 1123062, need to land first and then the patches in this bug need to land in numerical order or there will be test failures.

Aurora+

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aec2b73d9038
https://hg.mozilla.org/releases/mozilla-aurora/rev/8af1c37a245c
https://hg.mozilla.org/releases/mozilla-aurora/rev/5225cddf3c3a
https://hg.mozilla.org/releases/mozilla-aurora/rev/26f84760fadf
https://hg.mozilla.org/releases/mozilla-aurora/rev/21bb2a633e6e