Last Comment Bug 664788 - console.log(message) doesn't show leading whitespace in message
: console.log(message) doesn't show leading whitespace in message
Status: VERIFIED FIXED
[fixed-in-devtools]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: 664789
  Show dependency treegraph
 
Reported: 2011-06-16 11:46 PDT by Ian Bicking (:ianb)
Modified: 2011-06-28 07:02 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (2.87 KB, patch)
2011-06-17 06:56 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v2 (1.63 KB, patch)
2011-06-20 05:43 PDT, Panos Astithas [:past]
mihai.sucan: review+
dao+bmo: review+
Details | Diff | Splinter Review
Patch v3 (597 bytes, patch)
2011-06-21 03:03 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review

Description Ian Bicking (:ianb) 2011-06-16 11:46:56 PDT
I sometimes want to do:

  console.log('Some stuff:');
  console.log('  example:', foo);
  console.log('  also:', foo2);

But the leading whitespace is not displayed.  I wouldn't put those spaces there if I didn't want them to be there ;)
Comment 1 Philip Chee 2011-06-16 21:14:14 PDT
IMHO, I think this is an extreme edge case. Most people won't care.
Comment 2 Panos Astithas [:past] 2011-06-17 06:56:09 PDT
Created attachment 540050 [details] [diff] [review]
Patch v1

This has been bugging me as well, so I cooked up this patch that seems to fix it. It breaks lots of tests though that need fixing as well. Mihai do you have any less invasive ideas?
Comment 3 Teddy Ni 2011-06-18 23:53:48 PDT
In my opinion, preserving spaces in the debug console is the right thing to do. Easily formatted debug strings can make output more useful and palatable. Sure, it's not really important, but I can't think of any down side.

Along the lines of Panos's patch, would it be easier just to change

    aBody = aBody instanceof Ci.nsIDOMNode ?
            aBody : aDocument.createTextNode(aBody);

to

    aBody = aBody instanceof Ci.nsIDOMNode ?
            aBody : aDocument.createTextNode(aBody.replace(/ /g, "\xa0"));

In my inexperience, I have no idea how this would affect any tests.
Comment 4 Mihai Sucan [:msucan] 2011-06-20 05:14:24 PDT
Comment on attachment 540050 [details] [diff] [review]
Patch v1

There must be a styling-based approach. Maybe:

https://developer.mozilla.org/en/CSS/white-space

Try white-space: pre-wrap to see if it works.
Comment 5 Panos Astithas [:past] 2011-06-20 05:43:46 PDT
Created attachment 540440 [details] [diff] [review]
Patch v2

Bingo! Works as advertised, thanks!
Comment 6 Mihai Sucan [:msucan] 2011-06-20 09:54:08 PDT
Comment on attachment 540440 [details] [diff] [review]
Patch v2

Great to see it works!
Comment 7 Panos Astithas [:past] 2011-06-20 11:37:49 PDT
Comment on attachment 540440 [details] [diff] [review]
Patch v2

Thanks Mihai! Requesting peer review from dao.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-20 11:47:45 PDT
It's somewhat arguable whether that kind of rule belongs in the theme. Does the webConsole have a content stylesheet?
Comment 9 Dão Gottwald [:dao] 2011-06-20 16:10:51 PDT
Comment on attachment 540440 [details] [diff] [review]
Patch v2

This belongs in content CSS. Unfortunately, the webconsole is mingled in the browser window's DOM rather than being a separate iframe.
Comment 10 Panos Astithas [:past] 2011-06-21 03:03:51 PDT
Created attachment 540707 [details] [diff] [review]
Patch v3

I wasn't aware of the distinction, thanks. I guess that browser.css is the proper place, then?
Comment 11 Dão Gottwald [:dao] 2011-06-21 03:14:17 PDT
Well, no, ideally there would be a webConsole.css somewhere in toolkit/components/console/, and ideally that stylesheet would be loaded in an iframe rather than browser.xul.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-21 13:53:34 PDT
Comment on attachment 540707 [details] [diff] [review]
Patch v3

Indeed, this isn't really the proper place, but there are several things about the current web console code that aren't "proper". You could add a note to bug 635359, and also reference it in a comment here, but maybe it'd be cleaner to just add it to the theme for now and fix everything in bug 635359 (rather than expanding the set of things that need cleaning up to include browser.css).
Comment 13 Dão Gottwald [:dao] 2011-06-22 00:14:55 PDT
Comment on attachment 540440 [details] [diff] [review]
Patch v2

I'm still waiting for cleanup from the early days of the "heads up display" :/
Comment 14 Panos Astithas [:past] 2011-06-22 00:43:31 PDT
Thank you both. I added a comment to bug 635359 referencing comment 11 here.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-06-22 13:29:12 PDT
Comment on attachment 540440 [details] [diff] [review]
Patch v2

http://hg.mozilla.org/projects/devtools/rev/fb02b664d02d
Comment 16 Rob Campbell [:rc] (:robcee) 2011-06-24 05:59:05 PDT
backed out to try to clear some orange:

http://hg.mozilla.org/projects/devtools/rev/9d2571f091aa

should be able to reland.
Comment 18 George Carstoiu 2011-06-28 07:02:08 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110627 Firefox/7.0a1

Verified issue on Ubuntu 11.04, Mac OS X 10.6, WinXP, Win7 using the steps to reproduce from Comment 0.

Setting status to Verified Fixed.

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