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)
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+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
past
:
review+
lmandel
:
approval-mozilla-aurora+
|
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)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Another test case is http://beta.etherpad.org/ (see bug 1117007).
I have a fix.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Assignee | ||
Comment 3•10 years ago
|
||
[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.
status-firefox37:
--- → affected
status-firefox38:
--- → affected
tracking-firefox37:
--- → ?
tracking-firefox38:
--- → ?
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8550781 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
This probably fixes a whole bunch of edge cases where content uses
elements other than div.
Attachment #8550782 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8550783 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
(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)
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
After the patches in this bug, we only inject a newline at block element
boundaries.
Attachment #8551276 -
Flags: review?(n.nethercote)
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
(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?
Assignee | ||
Comment 17•10 years ago
|
||
(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. :-)
Assignee | ||
Comment 18•10 years ago
|
||
(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)
Updated•10 years ago
|
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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 21•10 years ago
|
||
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-
Assignee | ||
Comment 22•10 years ago
|
||
(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)
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Comment 24•10 years ago
|
||
> 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)
Comment 25•10 years ago
|
||
(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)
Assignee | ||
Comment 26•10 years ago
|
||
(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)
Assignee | ||
Comment 27•10 years ago
|
||
(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)
Assignee | ||
Comment 28•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Comment 29•10 years ago
|
||
> 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)
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Assignee | ||
Comment 31•10 years ago
|
||
(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•10 years ago
|
||
> Yes, I think so.
OK. What would go wrong if we just dropped the OutputRaw in nsCopySupport.cpp instead?
Assignee | ||
Comment 33•10 years ago
|
||
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.
Assignee | ||
Comment 34•10 years ago
|
||
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
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8553486 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8550783 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Updated•10 years ago
|
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 36•10 years ago
|
||
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+
Assignee | ||
Comment 37•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 38•10 years ago
|
||
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
Assignee | ||
Comment 39•10 years ago
|
||
This probably fixes a whole bunch of edge cases where content uses
elements other than div.
Attachment #8553899 -
Flags: review?(bzbarsky)
Comment 40•10 years ago
|
||
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+
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8554206 -
Flags: review?(bzbarsky)
Comment 42•10 years ago
|
||
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+
Comment 43•10 years ago
|
||
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)
Assignee | ||
Comment 44•10 years ago
|
||
With Brian's patch, the entire patch set is green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b816819253
Comment 45•10 years ago
|
||
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+
Comment 46•10 years ago
|
||
(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•10 years ago
|
||
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 48•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8554643 -
Attachment is obsolete: true
Assignee | ||
Comment 49•10 years ago
|
||
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
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
Flags: needinfo?(ehsan)
Assignee | ||
Comment 51•10 years ago
|
||
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
Flags: needinfo?(ehsan)
Comment 52•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 53•10 years ago
|
||
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)
Comment 54•10 years ago
|
||
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 56•10 years ago
|
||
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 57•10 years ago
|
||
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 58•10 years ago
|
||
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 59•10 years ago
|
||
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 60•10 years ago
|
||
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 61•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8553486 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8551276 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8554206 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8555339 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 62•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•