Open Bug 1377836 Opened 8 years ago Updated 5 months ago

Tabs/tabulators should not be rendered as spaces

Categories

(Thunderbird :: Message Reader UI, defect)

Product:
Thunderbird
Component:
Message Reader UI
Version:
52 Branch
Platform:
x86_64
Windows 7
Type:
defect

Tracking

(Not tracked)

People

(Reporter: robert.pollak, Unassigned)

Details

Attachments

(2 files, 7 obsolete files)

1377836-new-encoder-flag.patch (v1)
2.72 KB, patch
Details | Diff | Splinter Review
1377836-render-tabs.patch (v6).
4.87 KB, patch
Details | Diff | Splinter Review
When I receive a plaintext mail with tabulator characters, the corresponding whitespace is rendered with space characters. Here is a snippet from Tb's source window: ==== begin ==== User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.78 on 140.78.3.83 2 #680 Protokoll zum Besuch, Typprüfung für Zackenspur 2.5 6 #680 DoRefineCenter deduplizieren 7 ===== end ===== ^ Here is a single tab. And here is the snippet as copied from the normal message display: ==== begin ==== 2 #680 Protokoll zum Besuch, Typprüfung für Zackenspur 2.5 6 #680 DoRefineCenter deduplizieren 7 ===== end ===== ^^^^ Here are four spaces. I need the tabulators to copy tabular data into a spreadsheet. My workaround is obviously to copy the data from the source window instead.
Interesting request. I don't know where the tabs get lost when rendering the message onto the screen, maybe somewhere in TB when reading the message or perhaps somewhere in the Mozilla core rendering engine.
I looked at it a little further. I inspected the source window with the DOM Inspector. All the text in the source windows sits in a big <pre> tag, so we can understand that tabs work there. Tabs in HTML are generally not supported and treated as white-space characters. Try HTML page with tabs not in a <pre> tag. The tabs are displayed as single space. Here on BMO, all comments are displayed in a <pre> tag, so the tab works in your comment #0. TB knows a little better and substitutes tabs with nbsp here: https://dxr.mozilla.org/comm-central/rev/408fd2480e7e9e33d7a0f5f154553f2a297a2f16/mailnews/mime/src/mimetpfl.cpp#565 BTW: kSpacesForATab = 4; That leads to tabs showing up with a width of four spaces. Not ideal, but that's as good as we can do given the restrictions in HTML. Your solution would be to get the sender of the message to send you HTML mail with a <pre> tag, so: <pre> 2 #680 Protokoll zum Besuch, Typprüfung für Zackenspur 2.5 6 #680 DoRefineCenter deduplizieren 7 </pre> That should be displayed with the tab character for you to paste. Sorry, I can't give you a better answer.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Indeed, I've just tried it, HTML mail with <pre> gets you the tabs. <html> <head> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> </head> <body text="#000000" bgcolor="#FFFFFF"> <pre>1 tab 12 tab </pre> </body> </html>
I looked at it even further: If the original plain text message is sent "non-flowed", then it is rendered in TB as <pre> and the tabs are maintained. Since your message was sent "flowed", see: Content-Type: text/plain; charset=utf-8; format=flowed TB replaces the tabs with spaces and renders as <div: style="font-family: -moz-fixed; font-size: 14px;">. I'm not sure that couldn't be changed. Let me see.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: WONTFIX → ---
BTW, this looks like the ancient bug 144230, but I won't dupe since that bug contains a hotchpotch of problems. Let's focus on the "tab should be displayed as tab and not spaces here".
Attached patch 1377836-render-tabs.patch (obsolete) — — Splinter Review
OK, I played around with it a little. This patch renders plain text flowed messages in a <div style="white-space:pre-wrap; ...> and doesn't change tabs to spaces any more. So a plain text message with tabs gets displayed properly. <tab>5 5<tab>5 is displayed as 5 5 5 However, a HTML or plain text reply to such a message doesn't work at all since the two lines which are no longer separated by a <br> but by a CSS trick get concatenated to > 5 5 5 in a reply. So this approach doesn't fly at all. At least not so far and major rework would be necessary. So my initial WONTFIX came pretty close. Sadly.
Attached patch 1377836-render-tabs.patch (v2). (obsolete) — — Splinter Review
This works a little better. Tabs are rendered as such, and in replies, multiple lines are not longer collapsed into one. However, in a HTML and plain text reply tabs are collapsed to one space. It would need a whole lot of changes to get reply to do something useful with the tabs, I suspect it would need changes in the M-C serialiser and maybe QuotingOutputStreamListener in nsMsgCompose.cpp. So not an easy change and very regression prone. In HTML replies we'd have to change tabs to nbsp again, since tabs are rendered as one blank. For plain text replies we could move pass the tabs trough to the quote in the reply. Forward already works since it included the plain text message in a <pre>, so those tabs are honoured.
Attachment #8883325 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #7) > In HTML replies we'd have to change tabs to nbsp again, since tabs are > rendered as one blank. For plain text replies we could pass the tabs > trough to the quote in the reply. Looking at this further in QuotingOutputStreamListener::OnDataAvailable(): The tabs show up here and they are also put into a HTML reply, as mentioned above, rendered as a single space as noted in comment #2: "Tabs in HTML are generally not supported and treated as white-space characters." For plain text replies the tabs really get lost and are replaced by spaces. Bug 144230 comment #24 suggests that this happens in the plain text serialiser and I can confirm that. Debug before and after the call to ConvertBufToPlainText() in QuotingOutputStreamListener::ConvertToPlainText() shows === before |5<tab>huhu<br><tab>huhu<br><br></body> </html> </html>| === after |5 huhu huhu | where the tabs have been replaced by spaces or removed altogether at the beginning of the line. And about that the folks over in bug 144230 have been pulling their hair out since the serialiser is an unowned and unmaintained module.
Looking at this further, this is where the tab is replaced with a blank and is lost: https://dxr.mozilla.org/mozilla-central/rev/e6a7a778ba132b87f346a5458b0879c45a3061b7/dom/base/nsPlainTextSerializer.cpp#1791 So we need to find a way to preserve the tab for a plain text reply, perhaps be forcing mPreFormattedMail = true; which is set under different circumstances here: https://dxr.mozilla.org/mozilla-central/rev/e6a7a778ba132b87f346a5458b0879c45a3061b7/dom/base/nsPlainTextSerializer.cpp#571
Thank you for your investigations! In the meantime I have found another workaround: The receiver can set the preference mailnews.display.disable_format_flowed_support to true. I will test this.
Yes, setting mailnews.display.disable_format_flowed_support displays the tabs as tabs, as could be expected reading comment #4. But that will remove all support for flowed messages which I don't think you want in general. I'm still looking at improving the situation for flowed messages.
Comment on attachment 8883383 [details] [diff] [review] 1377836-render-tabs.patch (v2). While the patch displays tabs correctly in flowed messages, it destroys the flowing. The flowed text is rendered as word word word<space> word word word word<space> and showing that content with "pre-wrap" shows the lines like this, no flowing.
Attached patch 1377836-render-tabs.patch (v3). (obsolete) — — Splinter Review
Restore flowing.
Attachment #8883383 - Attachment is obsolete: true
Attached patch 1377836-render-tabs.patch (v4). (obsolete) — — Splinter Review
More tweaks. Idea is the same: Display with pre-wrap so that tabs show properly. Since we're using pre-wrap, we also don't need to change multiple spaces to nbsp any more. Next I need to tackle the reply problem which is harder.
Assignee: nobody → jorgk
Attachment #8883533 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8883567 [details] [diff] [review] 1377836-render-tabs.patch (v4). Alfred, here I have a patch that changes the way format=flowed messages are displays. I use pre-wrap, so I get the tabs to display correctly. What do you think? Which this change, replying to such a message is broken, that's what I need to fix next.
Attachment #8883567 - Flags: feedback?(infofrommozilla)
Attached patch 1377836-tabs-reply.patch (v1) (obsolete) — — Splinter Review
OK, this fixes plain text replies. I still need to fix HTML replies since there all whitespace is collapsed.
Attachment #8884067 - Flags: feedback?(infofrommozilla)
Attached patch 1377836-tabs-reply.patch (v2). (obsolete) — — Splinter Review
Oops, (v1) was the hacky patch that I didn't want to publish.
Attachment #8884067 - Attachment is obsolete: true
Attachment #8884067 - Flags: feedback?(infofrommozilla)
Attachment #8884068 - Flags: feedback?(infofrommozilla)
Masayuki-san, I'm trying to fix an ancient old bug 144230 from 2002. I need a new serialiser flag that will preserve whitespace although it's delivering "formatted" and not "preformatted" output. If you want, I can add a test to TestPlainTextSerializer.cpp. With the three patches attached so far, I can show tabs properly in format=flowed messages and do plain text replies to those messages. HTML replies still need more work.
Attachment #8884076 - Flags: feedback?(masayuki)
Comment on attachment 8884068 [details] [diff] [review] 1377836-tabs-reply.patch (v2). Sorry, I get: | nsMsgUtils.cpp | s:/Projects/mozi/comm-central/mailnews/base/util/nsMsgUtils.cpp(2086): error C2039: 'PreserveWhitespace': is not a member of 'nsIDocumentEncoder' | s:/Output/tb-obj/dist/include\nsIDocumentEncoder.h(115): note: see declaration of 'nsIDocumentEncoder' | s:/Projects/mozi/comm-central/mailnews/base/util/nsMsgUtils.cpp(2086): error C2065: 'PreserveWhitespace': undeclared identifier | s:/Projects/mozi/comm-central/mozilla/config/rules.mk:1040: recipe for target 'nsMsgUtils.obj' failed
Attachment #8884068 - Flags: feedback?(infofrommozilla) → feedback-
You need to apply the M-C patch to your /mozilla directory and do a |mozilla/mach build| since an IDL file has changed. Would I submit something that doesn't compile ;-)
Attached patch 1377836-fix-tabs.patch (v5). (obsolete) — — Splinter Review
I've merged the two C-C patches into one. I'll be asking for feedback again once I solved the HTML reply issue, won't be long now ;-)
Attachment #8883567 - Attachment is obsolete: true
Attachment #8884068 - Attachment is obsolete: true
Attachment #8883567 - Flags: feedback?(infofrommozilla)
Comment on attachment 8884076 [details] [diff] [review] 1377836-new-encoder-flag.patch (v1) Let's try a simpler solution that only renders format=flowed messages with pre-wrap. In replies, we use the old scheme.
Attachment #8884076 - Flags: feedback?(masayuki)
OK, this works without M-C changes. It's the small solution that only changes the display behaviour. Replies still use the old scheme of replacing tabs with spaces. That's necessary for HTML replies anyway. I found no way to communicate from the code in nsMsgCompose.cpp (which knows whether we're putting together a HTML or plain text reply) to the MIME code which kind of space treatment we need. So maybe it's not such a good idea to spend much time with this. Also, tabs in plain text quotes don't really work as expected. Say I quote <tab>5 which is rendered as 5 then the reply, if a tab were included, would look like > <tab>5 so effectively > 5 since the tab positions to column 9, regardless of the "> " that was prepended to the line. I'm open to opinions.
Attachment #8884365 - Attachment is obsolete: true
Attachment #8884427 - Flags: feedback?(infofrommozilla)
Attachment #8884427 - Flags: feedback?(acelists)
Comment on attachment 8884427 [details] [diff] [review] 1377836-render-tabs.patch (v6). (In reply to Jorg K (GMT+2) from comment #23) > Created attachment 8884427 [details] [diff] [review] > 1377836-render-tabs.patch (v6). > > OK, this works without M-C changes. It's the small solution that only > changes the display behaviour. Replies still use the old scheme of replacing > tabs with spaces. That's necessary for HTML replies anyway. If I see that correctly, it happens only if I answer with activated Format-Flowed on an F-F article. This does not happen with the three other combinations. > I found no way to communicate from the code in nsMsgCompose.cpp (which knows > whether we're putting together a HTML or plain text reply) to the MIME code > which kind of space treatment we need. So maybe it's not such a good idea to > spend much time with this. Agreed. I see even more problems which may not be worthwhile to be fixed. Just to mention them: What about line length? A tab counts as one char. But converted in multiple Spaces they may exceed the wrap-length. At F-F a tab that falls on a RAW line break is replaced by a space. > Also, tabs in plain text quotes don't really work as expected. > Say I quote > <tab>5 which is rendered as > 5 > then the reply, if a tab were included, would look like > > <tab>5 > so effectively > > 5 > since the tab positions to column 9, regardless of the "> " that was > prepended to the line. Formatting with tabs is a battle you can not win. At the latest in another client that uses a different tab-width, the design is broken anyway. > I'm open to opinions. The behavior should be as uniform as possible and independent of settings. Then we would have won a lot.
Attachment #8884427 - Flags: feedback?(infofrommozilla)
Comment on attachment 8884427 [details] [diff] [review] 1377836-render-tabs.patch (v6). I noticed something else. Copy & paste also does not work uniformly. When I do copy text with tabs with deactivated Format-Flowed from a F-F article, the tabs become spaces. Not for the other combinations.
Thanks for the feedback, but are you saying yea or nay? (In reply to Alfred Peters from comment #24) > What about line length? A tab counts as one char. But converted in multiple > Spaces they may exceed the wrap-length. Which wrap-length are you referring to in a F-F context? (F-F: format=flowed). > At F-F a tab that falls on a RAW line break is replaced by a space. Umm, not sure what you're saying here. Where is the code for that? > Formatting with tabs is a battle you can not win. At the latest in another > client that uses a different tab-width, the design is broken anyway. Actually, I don't want to win any battle. As stated, non-F-F displays tabs well so people can copy/paste their tables, I don't see why F-F should be different. Replacing tabs with NBSP/(normal) spaces seems wrong in any plain text environment. As explained, HTML replies are hopeless anyway and plain text replies are hard to do, but I could find a way to pass the required information from compose to MIME, maybe somehow though the stream listener. > The behavior should be as uniform as possible and independent of settings. > Then we would have won a lot. Well, that what I'm trying to do: Align non-F-F display with F-F display. The missing piece is the F-F reply. So are you saying: 1) Good so far, but do the reply as well. 2) Bad so far, you can't win, forget it. 3) Other: _______ P.S.: > When I do copy text with tabs with deactivated Format-Flowed from a F-F article, > the tabs become spaces. Not for the other combinations. Copying is different story. There M-C code prepares various clipboard flavours (text/html, text/plain) and it depends which one the paste operation uses. So can you be more specific. F-F message with tabs, mailnews.display.disable_format_flowed_support==true, copy tabs from F-F message, paste where/how? And with or without the patch, or doesn't it matter since with the pref set, everything should be treated as non-F-F. Which leads to the question why/if copying from a non-F-F message would be different.
With the patch applied I've done this: mailnews.display.disable_format_flowed_support==false (default) View F-F message with a tab in it. Copy text containing tab. Paste to Notepad++ or new plain text composition or new HTML composition: Result: Tab gone. I've looked at this with a clipboard viewer on Windows (FreeClipboardViewer) and see that the HTML flavour has the tab, the plain text flavour doesn't. So that explains the Notepad++ paste since that's using the text/plain flavour. Pasting into a composition will use text/html flavour and there apparently the tab gets tossed away in some other way which is understandable since HTML doesn't support tabs and (I assume) the editor doesn't know that we're pasting into a <pre-wrap> when pasting into a plain text composition. Anyway, that would need to be verified in FF alone. So maybe you can clarify your comment #25.
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
Attachment #8884427 - Flags: feedback?(acelists)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: