Open Bug 117009 Opened 23 years ago Updated 4 years ago

nsPlaintextSerializer: Remove trailing spaces around structured text

Categories

(Core :: DOM: Serializers, enhancement, P5)

Product:
Core
Component:
DOM: Serializers
Type:
enhancement

Tracking

()

People

(Reporter: BenB, Unassigned)

References

Details

Reproduction: 1. Open Compose windows 2. Enter text "hello you there" 3. Mark " you " (not the spaces) and make it bold 4. Send it as plaintext 5. Recieve Actual result: "hello* you *there" Expected result: "hello *you* there" Comment #4 in bug 116842 from bienvenu@netscape.com: <quote src="http://bugzilla.mozilla.org/show_bug.cgi?id=116842#c4"> Afte thrashing around for a while trying to figure out if I've broken anything, I've found out that converting html to plain text is broken in mozilla, and has nothing to do with my changes. If you use the html compose window to make some text bold, and send it as plain text, it can get sent incorrectly, or displayed incorrectly (not sure which - if you make " test " bold, it will get sent and displayed as "* test *", which is broken. Perhaps the struct code doesn't handle this case correctly, because of the leading and trailing spaces. Is there a bug on this? If that's the bug, it seems like this code is pretty seriously flawed. </quote> Why do you think so? The actual behaviour is not optimal, but why "seriously flawed"? I thought about it, and IIRC, it would've been quite hard to implement (don't remember why), and I didn't consider it worth it.
Blocks: 116842
Nonsnense, not mozTXTToHTMLConv, but nsPlaintextSerializer (TXT->HTML). This bug is in the generator, not the recognizer.
Assignee: neeti → harishd
Component: Networking → DOM to Text Conversion
QA Contact: benc → sujay
Summary: mozTXTToHTMLConv: Remove trailing spaces around structured text → nsPlaintextSerializer: Remove trailing spaces around structured text
> (not the spaces) s/not/note/ Sorry. I don't think that this code is "seriously flawed", because we (er, I) merely fail to fixup the suboptimal formatting. I now remember the reason why I didn't fix it when I wrote the code: It was next to impossible to fix it in the bold-processing code, because I didn't have a DON and at the time </b> was processed, the last trailing space was already written out to the result. We'd have had to cache the whole content between <b> and </b> and process it differently - and it could contain more formatting/structure. Since we moved to the DOM conversion (in contrast to the old stream conversion), this bug is probably easier to fix, because we could modify the DOM first and "fixup" all markup before we convert. I'd remove DoOpen/CloseContainer and move their content into Open/CloseContainer <http://lxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer.cpp#385>. Then, we'd have the DOM node in the tag-specific processing code, which would (presumably) enable us to modify it. We could just take the first #text childnode and remove lealding spaces and then take the last #text subnode and remove the trailing spaces and be done.
If I double click on a word in the html compose window and turn it bold, incorrect plain text will get generated. It seems like a serious flaw that we generate structured text that we can't display with just three clicks of the mouse. This isn't some pathological edge case, unless I'm misunderstanding something. I know that you'll violently resist this suggestion, but I don't think we should do this structure stuff on conversion from html to plain text if we can't handle such a simple case. To the user, it just looks like we're throwing asterix around.
Well, in that case, composer generates HTML "hello<b> you </b>there!". You can argue that this is just as flawed as "hello* you *there". Anyways, this RFE doesn't seem hard to fix.
How is this an RFE? Taking html text from the editor and converting it to structured text is just plain broken. If this isn't supported, we should just turn it off until the RFE is done.
Because we insert the stars exactly where the <b> tags are. It might be braindead, but it's not a bug either. FWIW, it's turned off in Netscape 6 anyways, I think.
I can't look into this bug atm, because my hd broke and is off for exchange.
I guess I'm confused. I thought the whole point of inserting the '*''s is so that we would display the text as bold when displaying the message as plain text, not to tell the user where the original html had its <b> tags. Is there some sort of spec/internet standard for the way "structured text" should be formatted/displayed? And I don't understand why it's OK to mess up mozilla user's messages but not Netscape users.
> I thought the whole point of inserting the '*''s is so > that we would display the text as bold when displaying the message as plain > text, not to tell the user where the original html had its <b> tags. The reason for the code is both of that. > Is there some sort of spec/internet standard for the way "structured text" > should be formatted/displayed? Yes <http://info.astrian.net/jargon/How_Jargon_Works/Hacker_Writing_Style.html>, but admittedly, it requires the stars to be around words. I'll maybe look into this bug when I have a chance.
--> tanu
Assignee: harishd → tmutreja
Unable to reproduce the bug. Tried with the latest build(Jan16) and don't see any additional space around structured text.
Tried this and it works for me too.
I can reproduce it with a relatively current trunk build. Make sure to mark the spaces around "you", i.e. mark " you ", not just "you". You are getting starts at all, don't you? However, the bug is less obvious by now, because now, Composer marks only "you" (without spaces) when doubleclicking on a word.
Ben, your comments look confusing to me. I think this is not what you mentioned in the original bug ("not the spaces"). Anyway, while marking, if we include spaces around "you" the expected behaviour should be the same as we are actually getting here i.e. "* you *" should be the right thing.
> I think this is not what you mentioned in the original bug ("not the spaces") Please read comment 3. > "* you *" should be the right thing. This doesn't follow the convention, which is to add the stars around the word. (The fact that our TXT->HTML recognizer and probably that of other Unix software doesn't recognize it is a result of that.)
Ben, I understand the problem now. I need to confirm the exact expected behaviour before starting working on it. If I select " you " and make it bold (including spaces around), should it be sent as "*you*" (which in turn would mean that "hello you there" would be sent as "hello*you*there" or should it be sent as " *you* "(spaces around stars instead of spaces). Second option looks better but that would actually mean that we are not displying the correct DOM Node, because in the actual DOM Node spaces are part of <b> tag. Is there any specification for this? I understand your comment#2 except for "I'd remove DoOpen/CloseContainer and move their content into Open/CloseContainer <http://lxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer. cpp#385>. Then, we'd have the DOM node in the tag-specific processing code, " I feel even in Open/CloseContainer of nsPlainTextSerilaizer we do not have access to the DOM Node. The solution that I can think atm is to set a flag with the opening <b> and handle the Text children of <B> based on that flag.
Status: NEW → ASSIGNED
> Second option looks better but that would actually mean that we are not > displying the correct DOM Node, because in the actual DOM Node spaces are part > of <b> tag. The second option is correct, even though it doesn't exactly match the DOM. > Is there any specification for this? see comment 9. > I feel even in Open/CloseContainer of nsPlainTextSerilaizer we do not have > access to the DOM Node. Bummer. I think you have access to it in AppendElement*(), but not sure, if/when these are called.
Assignee: t_mutreja → nobody
QA Contact: sujay → dom-to-text
This is a mass change. Every comment has "assigned-to-new" in it. I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Status: ASSIGNED → NEW

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.

Severity: normal → S4
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.