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

RESOLVED FIXED in mozilla22

Status

()

Core
Layout
--
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: ted, Assigned: jaws)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 22+)

Details

(Whiteboard: [parity-safari][parity-chrome])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Updated

12 years ago
Depends on: 16909

Comment 2

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

Comment 3

11 years ago
so Ted, where is this patch? :)

Updated

11 years ago
Whiteboard: [p-safari]
(Reporter)

Updated

11 years ago
Assignee: ted.mielczarek → nobody

Comment 4

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

Updated

8 years ago
Duplicate of this bug: 507822

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

6 years ago
Created attachment 602289 [details]
Some long lines of text.  (I'm specifying "text/plain".)

Comment 10

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

Comment 11

6 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. :-)
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.
Whiteboard: [p-safari] → [parity-safari][parity-chrome]
Created attachment 716387 [details] [diff] [review]
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.
Assignee: nobody → jAwS
Status: NEW → ASSIGNED
Attachment #716387 - Flags: review?(felipc)
Attachment #716387 - Flags: review?(Ms2ger)
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).
You may want to re-use mimeTypeIsTextBased for this.
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.
Created attachment 716940 [details] [diff] [review]
Patch v1.1

Thanks for the feedback Gavin and Felipe. I've addressed both recommendations in this new patch.
Attachment #716387 - Attachment is obsolete: true
Attachment #716387 - Flags: review?(felipc)
Attachment #716387 - Flags: review?(Ms2ger)
Attachment #716940 - Flags: review?(felipc)
Attachment #716940 - Flags: review?(Ms2ger)
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
Attachment #716940 - Flags: review?(felipc)
Created attachment 721041 [details] [diff] [review]
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.
Attachment #716940 - Attachment is obsolete: true
Attachment #716940 - Flags: review?(Ms2ger)
Attachment #721041 - Flags: review?(felipc)
Attachment #721041 - Flags: review?(Ms2ger)
Attachment #721041 - Flags: review?(felipc) → review+
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.
Attachment #721041 - Flags: review?(Ms2ger) → review?(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.
(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?
Flags: needinfo?(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?).
Flags: needinfo?(bzbarsky)
Created attachment 722003 [details] [diff] [review]
Patch v2
Attachment #721041 - Attachment is obsolete: true
Attachment #721041 - Flags: review?(bzbarsky)
Attachment #722003 - Flags: review?(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
Attachment #722003 - Flags: review?(bzbarsky) → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae59f51150c9
Blocks: 16909
No longer depends on: 16909
Summary: Add text/plain wrapping (port mozilla bug 16909) → Plain text (text/plain, text/javascript, text/css, etc) documents should word-wrap
relnote-firefox: --- → ?
https://hg.mozilla.org/mozilla-central/rev/ae59f51150c9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Depends on: 849373

Updated

5 years ago
Blocks: 849422

Updated

5 years ago
Blocks: 849568

Updated

5 years ago
Blocks: 849599
Component: General → Layout
Product: Firefox → Core
Target Milestone: Firefox 22 → mozilla22
Version: unspecified → Trunk
Depends on: 851230
Depends on: 851832
Blocks: 854425

Updated

4 years ago
relnote-firefox: ? → 22+
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/
Depends on: 965692
You need to log in before you can comment on or make changes to this bug.