Currently the view source window uses preformatted text. When a lot of HTML appears on a single line you often have to scroll a long way to see it all. I have two suggestions to "fix" this, either: 1) set "white-space: normal" by default so the pre-formatted text can be wrapped (I couldn't find anything associated with this hidden in XUL so I assume it's set elsewhere). 2) add an item on the "view" menu to set word wrap on and off. The no wrapping seems traditional, i.e. I think previous versions of navigator/communicator do the same, which has been a long standing annoyance due the inordinate amount of left-right scrolling you have to do.

One for rickg?

qa assigning to sairuh

I'll think about feature requests once I come up for air.

*** Bug 26740 has been marked as a duplicate of this bug. ***

with the the Future milestone, i'm not sure what to do with Resolved Later bugs... reopen, and set the milestone to Future? that way it won't get ignored. also adding helpwanted kw, if there are any [external or otherwise] takers out there.

*** Bug 35582 has been marked as a duplicate of this bug. ***

The traditional answer for netscape viewsource is to not wrap. Given the date, and relatively low priority of this, I'm marking won't fix.

> traditional answer for netscape viewsource is to not wrap Traditional isn't always good. (in this case, for example, it isn't). > Given the date That's what milestones are for (specifically, Future) > relatively low priority of this That's what the Priority field is for. reopening this, although I suspect it's a dup.

agreed. if your plate is too full to fix this in the future (which is understandable), feel free to reassign to someone else, or even nobody@mozilla.org. i'm also setting this as an enhancement request. (a very useful one, imho. :)

cc timeless, who's working on some other view-source stuff.

rickg: speaking of wrapping, I need a way to insert newlines into datasource:text/plain,<text>. Otherwise I certainly can't wrap. Do you know how to do that? I need this for psuedo wysiwyg implementation. Rickg is technically correct about general wrapping. What should happen is that the text control should be able to wrap text on its own (probably determined by a context menu "[x]wrap text").

It would be easy to set style on the view source window to moz-pre-wrap to make everything wrap, and it certainly would make it easier to read most pages since lots of pro sites don't seem to bother to wrap their html ... I'm not sure that's a good idea by default, however, since then you won't be viewing the actual source of the page. Having a menu under View letting you toggle back and forth (preferably remembering the setting between invocations) would be ideal.

indeed, I think, View -> Word Wrap is how it should be done. depends on bug 63026

viewsource backend was just rewritten, I will take a lookg at this.

If possible, I think it would be useful to force wrap entire code segments. (e.g.,the entire code segment <a href="http://www.blah.com/blah.html"> would be forced to the next line when the view source window is resized instead of having it break at "<a " which IE does.

The fix for this is actually pretty "simple" (see patch below). Indeed, changing the white-space property causes the viewsouce window to wrap. But it is another matter to make this a clickable option... A reminder of the viewsource GUI which would require updating the viewsouce.css on the fly... Index: viewsource.css =================================================================== RCS file: /cvsroot/mozilla/layout/html/document/src/viewsource.css,v retrieving revision 1.6 diff -u -5 -r1.6 viewsource.css --- viewsource.css 2001/05/02 07:40:37 1.6 +++ viewsource.css 2001/05/04 05:22:28 @@ -28,11 +28,11 @@ .viewsource { font-family: -moz-fixed; font-weight: normal; font-size: normal; color: black; - white-space: pre; + white-space: normal; } .start-tag { color: purple; font-weight: bold; }

Actually, 'white-space: -moz-pre-wrap' gives a nicer output.

-moz-pre-wrap is the one you want, normal would also collapse whitespace.

collapsing whitespace might be useful (certainly not as a default option). Can user style sheets affect view source?

> Can user style sheets affect view source? Yep. Putting this into my userContent.css file added wrapping to view source: .viewsource { white-space: -moz-pre-wrap !important; }

what we would need is a pref we can turn on/off and add a checkbox menuitem to the coming viewsource UI.

*** Bug 90287 has been marked as a duplicate of this bug. ***

Doron? Should you and I take a shot at this? Do we want the setting to persist between sessions/invocations? Or do we want it to default back to not wrapped every time one opens view source?

Now that the "View" menu is back, adding a toggle "View -> [/] Wrap" to dynamically insert/remove the "white-space: -moz-pre-wrap !important;" rule in the viewsource window will be great.

Proposed fix (entirely with JS -- someone could pick the torch...) 1. Add the following rule in viewsource.css pre[wrap="on"] { white-space: -moz-pre-wrap !important; } 2. When toggling "View -> [/] Wrap", use {JS+DOM}.setAttribute("wrap", "on/off") on the <pre> element that surrounds the viewsource content, et voila.

Also, the GUI code of mailnews for '[/] Wrap Long Lines' can be replicated here for the toggling GUI part.

since the 0.9.4 freeze just happened. -> 0.9.5 and taking

*** Bug 97008 has been marked as a duplicate of this bug. ***

How are things coming along on this one? If you need it, note that it is possible to set an id on the <pre> element in the back-end so that the JS side can getElementById() and go straight to that <pre>.

an id for the pre might be a good idea, as using getElementsByTagName is more costly.

I have it working now, I put it as the last item in the view menu. What code do i have to change to make the first pre have an ID?

Doron: this might be what you're looking for. Seems a good starting point anyway. http://lxr.mozilla.org/mozilla/source/htmlparser/src/nsViewSourceHTML.cpp#470 http://lxr.mozilla.org/mozilla/source/htmlparser/src/nsViewSourceHTML.cpp#564

It just occurred to me that since "wrap" isn't a valid attribute of <pre> (is it?), it might be best to use two classes instead. Also, one might want to persist the state via the pref system for example. Below is a pseudo-code (with the usual disclaimer of a dubious hand-made pseudo-diff...) 92 static const char* kPreClass = "viewsource"; + static const char* kPreClassWrap = "viewsourceWrap"; [...] 325 mSyntaxHighlight = PR_FALSE; + mWrapLongLines = PR_FALSE; 326 // This determines the value of the boolean syntax_highlight preference. 327 nsCOMPtr<nsIPref> thePrefsService(do_GetService(NS_PREF_CONTRACTID)); 328 if (thePrefsService) { 329 thePrefsService->GetBoolPref("view_source.syntax_highlight", &mSyntaxHighlight); + thePrefsService->GetBoolPref("view_source.wrap_long_lines", &mWrapLongLines); } [...] 564 tag.Assign(NS_LITERAL_STRING("PRE")); 565 CStartToken* theToken=NS_STATIC_CAST(CStartToken*, theAllocator->CreateTokenOfType(eToken_start,eHTMLTag_pre,tag)); 566 567 if(theToken) { 568 CAttributeToken *theAttr=nsnull; 569 570 nsCParserNode theNode(theToken,0,theAllocator); 571 //XXX Need to have set 'kPreId' to something sensible earlier, e.g., //XXX 'kPreId=viewsource'? + theAttr=(CAttributeToken*) + theAllocator->CreateTokenOfType(eToken_attribute, + eHTMLTag_unknown,NS_ConvertASCIItoUCS2(kPreId)); + theAttr->SetKey(NS_LITERAL_STRING("id")); + theNode.AddAttribute(theAttr); + const char* preClass = (mWrapLongLines) ? kPreClassWrap : kPreClass; 572 theAttr=(CAttributeToken*) theAllocator->CreateTokenOfType(eToken_attribute, -/+ eHTMLTag_unknown,NS_ConvertASCIItoUCS2(preClass)); 573 theAttr->SetKey(NS_LITERAL_STRING("class")); 574 theNode.AddAttribute(theAttr);

Seeking some hot r/sr for the additional patch on the back-end so that doronr can finish off the font-end. (Setting class="wrap-disabled | wrap-disabled" on the viewsourcePre works OK -- but resizing is slow with wrap-enabled and this confirms that viewsource remains a good test bed and challenger of the Gecko system.)

Back end changes look pretty good to me. One question: + const char* preClass = (mWrapLongLines) ? kPreClassWrapEnabled : kPreClassWrapDisabled; There is no real need for preClass other than stylistic considerations, right? It may make more sense to not create that temporary. Either way is fine with me -- this is not a performance-critical part of the code. r=bzbarsky for the back end changes if you add some comments to either the css or the c++ (or both) giving a brief summary of what's going on with the wrapping.

OK, added the following comments: >>>>>> // bug 22022 - these are used to support 'Wrap Long Lines' on the viewsource // window by selectively toggling between the following two classes defined in // viewsource.css; the setting is remembered between invocations using a pref. static const char* kPreId = "viewsourcePre"; static const char* kPreClassWrapEnabled = "wrap-enabled"; static const char* kPreClassWrapDisabled = "wrap-disabled"; <<<<<< Yep, indeed the temp is just for readability. I didn't want to twist NS_ConvertASCIItoUCS2(kPreClassXXX) and break that long line :-) It is just a pointer and is used only once on that single <pre> that viewsource has, so...

r=bzbarsky ccing vidur and jst for sr

Why do we need to set the class of the pre to 'wrap-disabled', can't we just let the default be what it is today and simply set the class to 'wrap' when we do want long lines to wrap. That would be one less stylerule, one less class name to worry about?

Maybe I could just call the class wrap?! "pre.wrap"...

I corrected a leftover that I had, and simply used 'wrap' and 'viewsource' as the id -- This way, people can easily override with their #viewsource { rules } in their userContent.css as before.

Another picky iteration :-) I changed the rules for things to be a little bit discoverable (without going at the source) that overriding is still possible... #viewsource { font-family: -moz-fixed; font-weight: normal; font-size: normal; color: black; white-space: pre; } #viewsource.wrap { white-space: -moz-pre-wrap; }

sr=jst

r=bzbarsky for that last patch if the CSS uses #viewsource (good idea there).

should the ui change the pref value, or just change the wrapping for this session? (and if people want it always on, to change their own prefs.js)

This seems to me like a setting that should persist between view source invocations in one session if nothing else... I think it would make most sense as a persisted pref. Just my $0.02.

Another vote for persistent pref. It would be maddening to have to change it every time.

Back-end patch landed.

doronr, mind letting us have a loot at the hooks that you had for the UI? Since there is no harm in persisting the state, and it looks like it is what most people would expect, it seems OK to do so. [Another side-benefit is that the slowness (when resizing in wrap mode) that will be exposed could perhaps drive interested people to try to make viewsource faster. Make viewsouce fast and Gecko will be lightning fast. There is no style resloution when re-sizing the window, hence the blame cannot just be put on style resolution. Spin-offs to watch: bug 84707 - view source w/ syntax coloring very slow bug 97229 - Frame construction time explodes for elements with many childs]

I showed it to bz yesterday, need to clean it up. I'll be attaching it shortly

Comment on attachment 48691 [details] [diff] [review] proposed patch for frontend >+ var menuitem = document.getElementById('menu_wrapLongLines'); >+ menuitem.setAttribute("checked", "true"); Is that temp var just for clarity? I would eliminate it.... the line is not that long >+// function that enables/disables long-line wraping depending on the view_source.wrap_long_lines >+// pref. This comments seems misleading... we actually enable/disable based on the current state, no? Other than that, r=bzbarsky

Looks good, and yep, the comment can simply read: +//function to toggle long-line wrapping and set the view_source.wrap_long_lines +//pref to persist the last state. Also, the first fetching of 'myWrap' in onLoadViewSource() can be removed since nobody seems to be using it.

new patch, fixes all mentioned issues.

Comment on attachment 48705 [details] [diff] [review] new front end patch r=bzbarsky

sr?

cc: alecf for sr=, unless blake is generous enough

Comment on attachment 48705 [details] [diff] [review] new front end patch sr=alecf

Fix checked in. The lizard bless you for doing this, Doron and rbs

*** Bug 99784 has been marked as a duplicate of this bug. ***

ok, win32 2001091703, wrap long lines fails if syntax highlight is off. We do add the <pre> in front without the colouring, right?

Err... with syntax highlight off, the <link href="viewsource.css"> is not added so as not to pick the coloring style rules: http://lxr.mozilla.org/mozilla/source/htmlparser/src/nsViewSourceHTML.cpp#529 Note: the id="viewsource" and class="wrap" are still added but they are of no use without the associated CSS...

Right.... reopening to handle that. The not loading stylesheet was a perf optimization. We also don't create the highlighting spans, so loading the sheet should not cause color to appear or anything... patch coming up.

rbs, would you review?

can't we create a stylesheet for unhighlighted source and load that?

We could.... but that seems like un-needed duplication of style rules, leading to sync problems. And the perf difference between two different sheets would be minimal.

r=doron, trivial change

Comment on attachment 50457 [details] [diff] [review] Patch to always load the stylesheet r=rbs as well

alecf: could you sr the new patch to fix?

Comment on attachment 50457 [details] [diff] [review] Patch to always load the stylesheet sr=alecf

checked in

coolness! just a quick question, though, before i verify this: i went to http://mozilla.org, opened the view source window, then selected View > Wrap Long Lines. [the source window size is 700x535 pixels.] what i see is that *most* of the lines fit in the window --however, there are some lines that still stretch beyond, and i need to scroll more towards the right to view. for example: <a HREF="http://www.mozilla.org/webtools/bonsai/cvslog.cgi?file=mozilla-org//html/index.html&amp;rev=&amp;root=/cvsroot/">Document History</a>. i'm wondering: is this because such lines contain no whitespace in the "right places", and thus cannot be wrapped within the confines of the window? just wanted to double-check --thx!

sairuh, that's because of the way that the `-moz-pre-wrap' attribute works. I filed bug 99457 to create a new attribute `-moz-pre-force-wrap' which will force long lines such as the one you entered to wrap.

thanks for the info, Christopher! finally vrfy'ing as fixed.