no wrapping in view source

VERIFIED FIXED in mozilla0.9.5

Status

SeaMonkey
UI Design
P3
enhancement
VERIFIED FIXED
18 years ago
9 years ago

People

(Reporter: unapersson, Assigned: Doron Rosenberg (IBM))

Tracking

Trunk
mozilla0.9.5
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

18 years ago
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.

Updated

18 years ago
Assignee: don → rickg

Comment 1

18 years ago
One for rickg?

Updated

18 years ago
QA Contact: paulmac → sairuh

Comment 2

18 years ago
qa assigning to sairuh

Updated

18 years ago
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → LATER

Comment 3

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

Comment 4

18 years ago
*** 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.
Status: RESOLVED → REOPENED
Component: XP Apps → XP Apps: GUI Features
Keywords: helpwanted
Resolution: LATER → ---
Target Milestone: --- → Future

Comment 6

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

Comment 7

17 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → WONTFIX

Comment 8

17 years ago
> 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.
Status: RESOLVED → REOPENED
OS: other → All
Hardware: Other → All
Resolution: WONTFIX → ---
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. :)
Severity: normal → enhancement

Comment 10

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

Comment 11

17 years ago
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").
Keywords: mozilla1.0

Comment 12

17 years ago
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

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

depends on bug 63026
Depends on: 63026
(Assignee)

Comment 14

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

Comment 15

17 years ago
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

17 years ago
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

17 years ago
Actually, 'white-space: -moz-pre-wrap' gives a nicer output.
-moz-pre-wrap is the one you want, normal would also collapse whitespace.

Comment 19

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

Comment 20

17 years ago
> 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;
}
(Assignee)

Comment 21

17 years ago
what we would need is a pref we can turn on/off and add a checkbox menuitem to
the coming viewsource UI.  
Blocks: 79518
*** 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?

Comment 24

16 years ago
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

16 years ago
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

16 years ago
Also, the GUI code of mailnews for '[/] Wrap Long Lines' can be replicated here 
for the toggling GUI part.
(Assignee)

Comment 27

16 years ago
since the 0.9.4 freeze just happened. -> 0.9.5 and taking
Assignee: rickg → doronr
Status: REOPENED → NEW
Target Milestone: Future → mozilla0.9.5
*** Bug 97008 has been marked as a duplicate of this bug. ***

Comment 29

16 years ago
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>. 
(Assignee)

Comment 30

16 years ago
an id for the pre might be a good idea, as using getElementsByTagName is more
costly.
(Assignee)

Comment 31

16 years ago
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?
Status: NEW → ASSIGNED
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

16 years ago
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

16 years ago
Created attachment 47590 [details]
proper diff (tested)

Comment 35

16 years ago
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.

Comment 37

16 years ago
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
Keywords: approval, patch
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

16 years ago
Created attachment 47616 [details]
updated diff to cut one class per jst's comments

Comment 41

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

Comment 42

16 years ago
Created attachment 47619 [details]
final patch

Comment 43

16 years ago
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

16 years ago
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).
(Assignee)

Comment 47

16 years ago
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.

Comment 49

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

Comment 50

16 years ago
Back-end patch landed. 

Updated

16 years ago
Keywords: approval, helpwanted, mozilla1.0, patch

Comment 51

16 years ago
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]
(Assignee)

Comment 52

16 years ago
I showed it to bz yesterday, need to clean it up. I'll be attaching it shortly
(Assignee)

Comment 53

16 years ago
Created attachment 48691 [details] [diff] [review]
proposed patch for frontend
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
Attachment #48691 - Flags: review+

Comment 55

16 years ago
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.
(Assignee)

Comment 56

16 years ago
Created attachment 48705 [details] [diff] [review]
new front end patch
(Assignee)

Comment 57

16 years ago
new patch, fixes all mentioned issues.
Attachment #48691 - Attachment is obsolete: true
Comment on attachment 48705 [details] [diff] [review]
new front end patch

r=bzbarsky
Attachment #48705 - Flags: review+
Keywords: approval, patch

Comment 59

16 years ago
sr?
(Assignee)

Comment 60

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

Comment 61

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

sr=alecf
Attachment #48705 - Flags: superreview+
Fix checked in.  The lizard bless you for doing this, Doron and rbs
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED
*** Bug 99784 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 64

16 years ago
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

16 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 50457 [details] [diff] [review]
Patch to always load the stylesheet
rbs, would you review?
Keywords: approval → review
(Assignee)

Comment 69

16 years ago
can't we create a stylesheet for unhighlighted source and load that?
Keywords: review → approval
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.
(Assignee)

Comment 71

16 years ago
r=doron, trivial change

Comment 72

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

r=rbs as well
Attachment #50457 - Flags: review+
(Assignee)

Comment 73

16 years ago
alecf: could you sr the new patch to fix?

Comment 74

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

sr=alecf
Attachment #50457 - Flags: superreview+
checked in
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.