(not currently active) Ted Mielczarek Reporter
	Description • 20 years ago

There's a seamonkey bug, bug 16909 , that has a patch to enable wrapping of
plain text documents viewed in the browser window.  I think this should be
ported to Firefox.  It adds a "View -> Word Wrap" menu item, which would toggle
the wrapping status of the current document.  I don't think the patch there is
perfect, since that code has the menuitem toggling a pref, which doesn't make
much sense to me.  Also, the patch doesn't disable the menuitem on non-plain
text pages, which it ought to.  Otherwise I think this is a useful feature.

I'll port that patch in a little bit.  This obviously isn't going to make FF1.0,
so no hurry.

	Daniel Hyde
	Comment 1 • 19 years ago

The view-source window has this feature, so as a nasty workaround you can read a
plain-text file wrapped by viewing its "source" and enabling wrapping on that.

It should be in the main interface though.  Opening a view-source window just to
wrap text is nasty.

In the view-source window, it looks like View -> Wrap Long Lines.

	Olli Männistö
	Comment 2 • 18 years ago

Very active bug, this. 

I just started wondering why my fancy IE-killing browser shows text files on my HDD in such a retarded way. I immediately went to "view" to look for word wrap, which was not present, of course. 

If this 2 year old suggestion is implemented, IMO the toggle should be for ALL text files, not just the current one (mouse clicks eat your life)

	Frédéric Buclin
	Comment 3 • 18 years ago

so Ted, where is this patch? :)

	scientus
	Comment 4 • 15 years ago

I dont think it needs a toggle, I think the completely sensible setting is to always word-wrap text/plain, but soft wrap of course so that copy-paste gets the original. As it is already in a proportionally spaced font this wont break the only thing i could think of which would be ASCII art.

	Ian Stirling.
	Comment 6 • 15 years ago

Always wordwrap can be a bad thing.
Consider a very wide text table - say 200 chars - wrapping may break stuff.

	Yet Ding
	Comment 7 • 15 years ago

Workaround: in userContent.css
  body > pre:first-child:last-child {
    white-space: pre-wrap;
  }

How other browsers do it:
IE, Opera: no wrap
Safari, Chrome: wrap

IMO it should only be on by default if there's a switch to disable it. There should also be a visual hint for continued lines (background color?).

	Travis Evans
	Comment 8 • 12 years ago

The lack of this feature is even worse now that the View Source option is apparently *disabled* for text documents.  The workaround in Comment 7 did not work for me in Firefox 10, but the bookmarklet described in Bug 507822 works beautifully (and can easily be placed on the Bookmarks toolbar for convenience).

	Peter Ford
	Comment 10 • 12 years ago

The "Page Source" trick still works for me in Firefox 10.0.2.  What made you suspect it was disabled?

	Travis Evans
	Comment 11 • 12 years ago

Whoops, my mistake.  It was disabled on a page generated by a locally running piece of software that I mistook for a plain text page but appears to actually be served with the application/json type.  Seems to work fine with plain/text.  Though I think I like the bookmarklet better now--faster, more convenient, plus it works in both instances. :-)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 12 • 11 years ago

We should do this for text/plain documents, and I don't think we need to include a menuitem or option for this. An about:config pref is a fine compromise for those wanting to turn this off.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 13 • 11 years ago

This patch implements word-wrap for text/plain, text/javascript, text/x-javascript, and text/css. It adds a stylesheet to the page if the content type matches one of the aforementioned types. The stylesheet is disabled if the browser.plainText.wordWrap pref is false (the pref is enabled by default).

Requesting review from Felipe since all of the code changes are under /browser.

I'd also like a review/spec pass from Ms2ger since http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-text shows that it was recently edited by Ms2ger.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 14 • 11 years ago

I forgot to mention something in my previous comment. Because the stylesheet is added to the document, the application of the stylesheet can be toggled using the preexisting View->Page Style menu. Toggling this menuitem however will not persist the choice of the user (the about:config pref will need to be flipped if the user wants the change to be persisted).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 15 • 11 years ago

You may want to re-use mimeTypeIsTextBased for this.

	:Felipe Gomes (needinfo for replies!)
	Comment 16 • 11 years ago

The View Source window has a better string for this, IMO: "Wrap Long Lines". Probably not worth to try to use the string from there but I'd just reword this one to be the same.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 17 • 11 years ago

Thanks for the feedback Gavin and Felipe. I've addressed both recommendations in this new patch.

	:Felipe Gomes (needinfo for replies!)
	Comment 18 • 11 years ago

Comment on attachment 716940 [details] [diff] [review]
Patch v1.1

Review of attachment 716940 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4417,5 @@
> +      } else if (!onContentRSChangeRegistered) {
> +        content.document.addEventListener("readystatechange", onContentRSChange);
> +        onContentRSChangeRegistered = true;
> +      }
> +

This block seems to basically be repeating the same logic as above. Can't you invert the block above to let the "if (content.document.rs == 'interactive'..." be the outer condition and simplify things? (hopefully it won't the need this new boolean to track if the listener was registered)

The same comment about "// Don't need to re-enable/disable find commands for same-document location changes" applies to applyWordWrap so that seems doable

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 19 • 11 years ago

This patch combines the two code blocks and reduces some of the duplication in the previous patch. It is tricky to make sure that the logic still flows in the correct paths, but I believe that this refactoring has not changed any of the code paths for disabling fast-find.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 20 • 11 years ago

Comment on attachment 721041 [details] [diff] [review]
Patch v1.2

Boris, can you please review this change to verify that I am not breaking the spec here? My interpretation of http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-text is that this change is acceptable, but it would be good if you could verify it.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

This is fine per spec.  But the behavior here will be that the wrap rules don't take effect until the document is fully loaded, no?

That seems pretty suboptimal, especially for large documents.  Seems like it would be better to do this on the Gecko level so we can just inject the link when we create the <head>...  Alternately, to check for the <head> earlier and if it's not there watch for it with a mutation observer.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 22 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #21)
> This is fine per spec.  But the behavior here will be that the wrap rules
> don't take effect until the document is fully loaded, no?

This is true, since it is waiting for "interactive" which is analogous to DOMContentLoaded.

> That seems pretty suboptimal, especially for large documents.  Seems like it
> would be better to do this on the Gecko level so we can just inject the link
> when we create the <head>...  Alternately, to check for the <head> earlier
> and if it's not there watch for it with a mutation observer.

I originally approached this patch by editing /parser/htmlparser/src/CNavDTD.cpp but that change seems less straightforward and maybe only affect text/plain documents? If that's not the right place in Gecko, where should I look to implement this?

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 11 years ago

I think the right place nowadays is nsHtml5TreeBuilder::StartPlainText.  See how nsHtml5TreeBuilder::StartPlainTextViewSource add a <link>; you'll want something like that.  Note that you'll also need to stop calling StartPlainText from StartPlainTextViewSource (maybe move its current contents to a new method called from both places?).

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 11 years ago

Comment on attachment 722003 [details] [diff] [review]
Patch v2

Looks good for the C++ bits.  For the all.js change, would it make sense to default this to false and only set true for apps that want to opt in (e.g. Firefox in this case)?

r=me

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 26 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #25)
> Comment on attachment 722003 [details] [diff] [review]
> Patch v2
> 
> Looks good for the C++ bits.  For the all.js change, would it make sense to
> default this to false and only set true for apps that want to opt in (e.g.
> Firefox in this case)?
> 
> r=me

Yeah, that probably is the better way to do it. I'll make the change before landing.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae59f51150c9

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ae59f51150c9

	Alex Keybl [:akeybl]
	Comment 29 • 11 years ago

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email release-mgmt@mozilla.com asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/