22022 - no wrapping in view source

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement, P3)

Description

[unapersson]

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.

Comment 1

[Michael Lowe]

One for rickg?

Comment 2

[Paul MacQuiddy]

qa assigning to sairuh

Comment 3

[rickg]

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

Comment 4

[cbegle]

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

Comment 5

[sairuh (rarely reading bugmail)]

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.

Comment 6

[Pierre Saslawsky]

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

Comment 7

[rickg]

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.

Comment 8

[Blake Ross]

> 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.

Comment 9

[sairuh (rarely reading bugmail)]

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
[e-mail].

i'm also setting this as an enhancement request. (a very useful one, imho. :)

Comment 10

[Jesse Ruderman]

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

Comment 11

[timeless]

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").

Comment 12

[Akkana Peck]

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.

Comment 13

[Alexey Chernyak]

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

depends on bug 63026

Comment 14

[Doron Rosenberg (IBM)]

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

Comment 15

[jkng]

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. 

Comment 16

[rbs]

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;
 }

Comment 17

[rbs]

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

Comment 18

[Hixie (not reading bugmail)]

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

Comment 19

[timeless]

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

Comment 20

[Matthew Cline]

> 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;
}

Comment 21

[Doron Rosenberg (IBM)]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[rbs]

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.

Comment 25

[rbs]

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.

Comment 26

[rbs]

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

Comment 27

[Doron Rosenberg (IBM)]

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

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 29

[rbs]

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>. 

Comment 30

[Doron Rosenberg (IBM)]

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

Comment 31

[Doron Rosenberg (IBM)]

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?

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 33

[rbs]

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);

Comment 34

[rbs]

Attachment: proper diff (tested)

Comment 35

[rbs]

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.)

Comment 36

[Boris Zbarsky [:bzbarsky]]

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.

Comment 37

[rbs]

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...

Comment 38

[Boris Zbarsky [:bzbarsky]]

r=bzbarsky

ccing vidur and jst for sr

Comment 39

[Johnny Stenback (:jst)]

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?

Comment 40

[rbs]

Attachment: updated diff to cut one class per jst's comments

Comment 41

[rbs]

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

Comment 42

[rbs]

Attachment: final patch

Comment 43

[rbs]

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.

Comment 44

[rbs]

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;
}

Comment 45

[Johnny Stenback (:jst)]

sr=jst

Comment 46

[Boris Zbarsky [:bzbarsky]]

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

Comment 47

[Doron Rosenberg (IBM)]

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)

Comment 48

[Boris Zbarsky [:bzbarsky]]

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.

Comment 49

[Akkana Peck]

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

Comment 50

[rbs]

Back-end patch landed. 

Comment 51

[rbs]

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]

Comment 52

[Doron Rosenberg (IBM)]

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

Comment 53

[Doron Rosenberg (IBM)]

Attachment: proposed patch for frontend

Comment 54

[Boris Zbarsky [:bzbarsky]]

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

Comment 55

[rbs]

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.

Comment 56

[Doron Rosenberg (IBM)]

Attachment: new front end patch

Comment 57

[Doron Rosenberg (IBM)]

new patch, fixes all mentioned issues.

Comment 58

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 59

[rbs]

sr?

Comment 60

[Doron Rosenberg (IBM)]

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

Comment 61

[Alec Flett]

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

sr=alecf

Comment 62

[Boris Zbarsky [:bzbarsky]]

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

Comment 63

[Boris Zbarsky [:bzbarsky]]

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

Comment 64

[Doron Rosenberg (IBM)]

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

Comment 65

[rbs]

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...

Comment 66

[Boris Zbarsky [:bzbarsky]]

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.

Comment 67

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to always load the stylesheet

Comment 68

[Boris Zbarsky [:bzbarsky]]

rbs, would you review?

Comment 69

[Doron Rosenberg (IBM)]

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

Comment 70

[Boris Zbarsky [:bzbarsky]]

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.

Comment 71

[Doron Rosenberg (IBM)]

r=doron, trivial change

Comment 72

[rbs]

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

r=rbs as well

Comment 73

[Doron Rosenberg (IBM)]

alecf: could you sr the new patch to fix?

Comment 74

[Alec Flett]

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

sr=alecf

Comment 75

[Boris Zbarsky [:bzbarsky]]

checked in

Comment 76

[sairuh (rarely reading bugmail)]

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!

Comment 77

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 78

[sairuh (rarely reading bugmail)]

thanks for the info, Christopher!

finally vrfy'ing as fixed.

Comment 79

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/b493b7529231
[fenix] For https://github.com/mozilla-mobile/fenix/issues/22022 - Update SelectableChip button background colors
https://hg.mozilla.org/releases/mozilla-beta/rev/dbbb6e7c4c60
[fenix] For https://github.com/mozilla-mobile/fenix/issues/22022 - Composify Customize Home Button and update the background color