Closed Bug 1119503 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Serializers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed

People

(Reporter: BenWa, Assigned: ehsan.akhgari)

References

Details

Attachments

(6 files, 5 obsolete files)

(deleted), patch
n.nethercote
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
(deleted), patch
past
: review+
Details | Diff | Splinter Review
Bug from discussion with ehsan. As an example: <div style="white-space: pre;"> <div>foo</div> <div> bar</div> </div>
Flags: needinfo?(ehsan)
Another test case is http://beta.etherpad.org/ (see bug 1117007). I have a fix.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Blocks: 116083
[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.
Attachment #8550781 - Flags: review?(bzbarsky)
This probably fixes a whole bunch of edge cases where content uses elements other than div.
Attachment #8550782 - Flags: review?(bzbarsky)
(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! :-)
Flags: needinfo?(pbrosset)
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?
Flags: needinfo?(pbrosset)
(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/.
Flags: needinfo?(past)
After the patches in this bug, we only inject a newline at block element boundaries.
Attachment #8551276 - Flags: review?(n.nethercote)
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
Flags: needinfo?(past) → needinfo?(bgrinstead)
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 :)
Attachment #8551276 - Flags: review?(n.nethercote) → review+
(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.
Attached patch console-message-pre-WIP.patch (obsolete) (deleted) — Splinter Review
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?
Flags: needinfo?(bgrinstead) → needinfo?(ehsan)
(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?
(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. :-)
(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.
Flags: needinfo?(ehsan)
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.
Attachment #8550781 - Flags: review?(bzbarsky) → review+
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
Attachment #8550782 - Flags: review?(bzbarsky) → review+
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?
Flags: needinfo?(ehsan)
Attachment #8550783 - Flags: review?(bzbarsky) → review-
(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...
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(bgrinstead)
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?
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
(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.
(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.
Depends on: 611103
> Yes, I think so. OK. What would go wrong if we just dropped the OutputRaw in nsCopySupport.cpp instead?
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.
Blocks: 1123062
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.
Attachment #8550781 - Attachment is obsolete: true
Attachment #8550783 - Attachment is obsolete: true
Attachment #8550782 - Attachment description: Part 2: Determine whether an element is a block element based on the style, not the tag → Part 1: Determine whether an element is a block element based on the style, not the tag
Attachment #8551276 - Attachment description: Part 4: Change test_aboutmemory.xul in order to not expect a newline at the very end of the copied text → Part 3: Change test_aboutmemory.xul in order to not expect a newline at the very end of the copied text
Comment on attachment 8553486 [details] [diff] [review] Part 2: Insert a line break between preformatted block boundaries when creating raw output r=me
Attachment #8553486 - Flags: review?(bzbarsky) → review+
Attached file devtools test failures (deleted) —
These are the devtools test failures with the latest version of the patches in this bug. Brian, can you please suggest a fix? Thanks!
Flags: needinfo?(bgrinstead)
Flags: needinfo?(bgrinstead)
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.
Attachment #8550782 - Attachment is obsolete: true
This probably fixes a whole bunch of edge cases where content uses elements other than div.
Attachment #8553899 - Flags: review?(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
Attachment #8553899 - Flags: review?(bzbarsky) → review+
No longer blocks: 1123062
Comment on attachment 8554206 [details] [diff] [review] Part 4: Add a test for serialization of block elements without style information r=me
Attachment #8554206 - Flags: review?(bzbarsky) → review+
Attached patch console-message-pre.patch (obsolete) (deleted) — Splinter Review
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
Attachment #8551909 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8554643 - Flags: review?(past)
With Brian's patch, the entire patch set is green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b816819253
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.
Attachment #8554643 - Flags: review?(past) → review+
(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.
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.
Attachment #8555339 - Flags: review?(past)
Comment on attachment 8555339 [details] [diff] [review] console-message-pre-and-wrap-simple-message.patch Review of attachment 8555339 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8555339 - Flags: review?(past) → review+
Attachment #8554643 - Attachment is obsolete: true
Depends on: 1113238
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?
Flags: needinfo?(bzbarsky)
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 54 seems reasonable to me.
Flags: needinfo?(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 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
Attachment #8553899 - Flags: approval-mozilla-aurora?
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
Attachment #8553486 - Flags: approval-mozilla-aurora?
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
Attachment #8551276 - Flags: approval-mozilla-aurora?
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
Attachment #8554206 - Flags: approval-mozilla-aurora?
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
Attachment #8555339 - Flags: approval-mozilla-aurora?
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+
Attachment #8553899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8553486 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8551276 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8554206 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8555339 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: