253564 - Plain text (text/plain, text/javascript, text/css, etc) documents should word-wrap

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[Daniel Hyde]

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.

Comment 2

[Olli Männistö]

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)

Comment 3

[Frédéric Buclin]

so Ted, where is this patch? :)

Comment 4

[scientus]

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.

Comment 6

[Ian Stirling.]

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

Comment 7

[Yet Ding]

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

Comment 8

[Travis Evans]

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

Comment 9

[Peter Ford]

Attachment: Some long lines of text. (I'm specifying "text/plain".)

Comment 10

[Peter Ford]

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

Comment 11

[Travis Evans]

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

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 15

[:Gavin Sharp [email: [e-mail]]]

You may want to re-use mimeTypeIsTextBased for this.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

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

Comment 18

[:Felipe Gomes (needinfo for replies!)]

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 25

[Boris Zbarsky [:bzbarsky]]

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

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Alex Keybl [:akeybl]]

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

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