Closed Bug 235223 Opened 21 years ago Closed 19 years ago

when layout whitespace trimming is fixed, spaces don't show up in editor until next character typed

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7beta

People

(Reporter: mscott, Assigned: dbaron)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [patch])

Attachments

(1 file, 1 obsolete file)

updated patch
1.54 KB, patch
mrbkap
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Something broke over this past week in editor. IN mail compose if you type a space, we don't actually move the cursor past the space untily you type another character. As a result it looks like the spaces aren't showing up.
Severity: normal → critical
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird0.6
This is not specific to Thunderbird. I am seeing it also in Seamonkey on Linux. Perhaps the product should be MailNews and made to block 1.7b.
yeah...
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews
Target Milestone: Thunderbird0.6 → mozilla1.7beta
Version: unspecified → Trunk
If this regression is in the 1.7a tag (I am seeing it on a trunk build from Friday), then this is probably a 1.7a stopper as well. Not having the space bar work right in HTML compose is pretty severe.
Flags: blocking1.7a?
wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219 It´s buildID 2004021913 including talkback: http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/experimental/2004-02-19-14-1.7a/ This must have regressed later, but I can´t check, as there are no windows builds.
I can't reproduce any problems making spaces in the 1.7 Alpha builds. I've tested Message Compose, Composer, and text areas in the browser on windows, mac and linux and don't see this problem.
Flags: blocking1.7b?
Flags: blocking1.7a?
Flags: blocking1.7a-
Through trial and error I've narrowed this down to checkins between midnight of the 19th and midnight of the 20th. so a 24 hour window
I finally figured out what caused this regression. Looks like: Bug #132561 backing out the changes to nsLineLayout made white space start showing up in HTML compose again.
The problem is that in HTML a trailing whitespace like that should NOT show up by default. If you want it to show up, you need use a style that will preserve whitespace in display (eg "white-space: -moz-pre-wrap" or "white-space: pre").
ummm just so we are talinga bout the same thing, bring up compose window to type a new message.... type in some text like: "Hello World" Note that compose shows "HelloWorld" now. I wouldn't think I should have to add style rules to make editor respect white space between words. But maybe I misunderstood what you wer suggesting as the fix.
> Note that compose shows "HelloWorld" now. It shows "Hello World" over here on a trunk build I pulled last night. I do see the problem described in comment 0, and that's what comment 8 is about. I have no idea what you're seeing in comment 9....
I'd think this should be fixed in the caret-positioning code.
no I see comment #0, I mis spoke in comment #9.
Attached patch patch (obsolete) — — Splinter Review
For no obvious reason this was clamping the caret position to the width of the frame. I got rid of the IBMBIDI ifdefs since the ifdef-ed code here is clearly correct. I don't understand the XXX comment, but perhaps that means I should leave it rather than get rid of it...
We've had longstanding issue when trying to draw the caret outside of frames; if we do this, we end up with caret turds in various situations, because we can't guarantee that the caret is getting erased correctly. So if this patch makes it so that the caret can draw outside of frames, test carefully (on multiple platforms) for new caret turd issues.
Thanks a lot David! This patch is working for me as far as addressing the original problem. But I can't vouch against the concerns raised by Simon. I'd have to try it out for a while. One of the bugs Simon is referring to is probably the famous Bug #98564 which bothers a lot of folks. The patch in that bug caused just the regression cited by Simon here...we started seeing caret turds which is why it hasn't gone into the tree yet.
That code got added for bug 19265... I asume backing out this one part of that patch doesn't regress that bug?
I released a build with this patch in it so folks can use the builds and get around this regression. Apparently the patch has introduced another editor regression where we now show a space where there sometimes isn't. You can now end up with "end SPACE CURSOR" the cursor looks like there is a space after end. Hitting delete, deletes teh 'd' in end but the space still looks like it is there. At this point, the regression has been in the builds for almost a week (?). That's a really long time for something as bad as this behavior. The patch we have to fix it causes another issue with caret behavior in editor. I'd like to recommend that we back out the small layout change that caused this regression so mail compose becomes a bit more useable again. Once we have an editor patch that works right, then we can re-land Boris's small patch to nsLineLayout.
> You can now end up with "end SPACE CURSOR" How does one get into this situation? As far as that goes, has anyone tried setting the white-space rules I suggested on the editor in question?
Note that I just checked in DOM inspector, and setting "white-space: -moz-pre-wrap" on the <body> in the mail compose window's editing area seems like a better temporary workaround than rebreaking layout of whitespace befor <br> in general.... I've not been able to find any problems it causes (though I also didn't spend that long looking, to be frank). I'm not sure whether it's easy to hook up the mailcompose code to do this. I'm also not sure how hard the issues with David's patch are to resolve. More information on those issues would be nice.
*** Bug 233982 has been marked as a duplicate of this bug. ***
Ok, per Brendan, I backed out the regression that is causing this for now. Now we need to figure out what the right thing to do is to fix the problems this patch causes for editor. I just don't know enough about the implications of adding a style rule like white-space: -moz-pre-wrap to composer means. I think we need Simon and Daniel to decide what those implications are. Keep in mind this regression is with composer, mail is just one app that uses composer. So in theory you need to either add this kind of style rule to all consumers of an HTML editor or find a core place in editor to add such a rule.
I could be wrong, but I thought that -moz-pre-wrap implied monospace font (i.e. behave like <pre>) and may affect other space-collapsing behaviour (i.e. space runs).
(In reply to comment #21) > Keep in mind this regression is with composer, mail is > just one app that uses composer. So in theory you need to either add this kind > of style rule to all consumers of an HTML editor That's not feasible, since it would break display of actual HTML pages in editor. And that's not desirable, because we _don't_ want editor rendering differently from the browser. If I have a <div style="border: solid red">text <br>text</div> we don't want HTML editor rendering that space, in my opinion. So we do in fact want a very much mail-specific fix. (In reply to comment #22) > I could be wrong, but I thought that -moz-pre-wrap implied monospace font It does not. It just affects whitespace handling. > and may affect other space-collapsing behaviour (i.e. space runs). Space runs in editor end up with &nbsp; in place of all but one of the spaces. So the style will not have an effect on them at all. I _did_ test that before making comment 19.
If someone can point me to whatever stylesheet mail uses for the HTML smilys, I'll attach a patch for people to test.
Hmm, I don't think we have a style sheet that's specific to mail for editor (but I could be wrong). The smileys are in EditorContent.css. http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/EditorContent.css#78 FWIW...
Hrm. Like I said, we don't want this in editor in general, really... We could just have the mailcompose window's load handler set this style on the <body>. That'd have to be duplicated in tbird, I assume, and hence doesn't make me that happy. David's solution would be the way to go if caret painting weren't all confused...
well... if we don't have the editor internals, then we could do: 363 var editor = GetCurrentEditor(); 411 try { 412 editor.QueryInterface(nsIEditorStyleSheets); 413 414 // and extra styles for showing anchors, table borders, smileys, etc 415 editor.addOverrideStyleSheet(kNormalStyleSheet); replace kNormalStyleSheet with something interesting. Instead, i'd suggest using: nsIEditingSession.idl : htmlmail, the code could live in editor.xml for the moment. note that composer seems to know about htmlmail, so perhaps that's not too bad.
You say you don't want this in editor in general, but if you're writing a new web page, and you start typing some text, you do want the caret to appear after you press space... perhaps the fix is to make space insert an &nbsp; and correct it back to a space if you type another character? Also, do these invisible spaces get serialized by the plain text converter?
> Also, do these invisible spaces get serialized by the plain text converter? Yes.
*** Bug 236214 has been marked as a duplicate of this bug. ***
*** Bug 236370 has been marked as a duplicate of this bug. ***
*** Bug 236787 has been marked as a duplicate of this bug. ***
Bug 236787 is OS/2 -> All
OS: Windows XP → All
The more I think about it, the more I think that comment 28 is right and nsHTMLEditor's behavior on spacebar should be changed. Is there an editor person who's willing to try to give it a shot? Or at least provide some clear information on what places will need changing? Or will bug 132561 follow in the footsteps of bug 69070 and be forever a browser bug because editor makes some assumptions it should not be making?
Blocks: 132561
I think comment #28 is right too. But a second space should (or might) turn the first space to a &#x20; but then stay a &nbsp; itself, because otherwise it would become ignorable whitespace. Usually when a user types more then one space he/she wants it to stay.
OK, as long as when you type "Sentence 1. Sentence 2." that the correct space remains an &nsbp; - presumably the first one in that case. I originally typed "the right space is left" but decided it was confusing ;-)
no longer a 1.7 blocker.
Flags: blocking1.7b?
(In reply to comment #35) > Usually when a user types more then one space he/she wants it to stay. That depends. Often, when a user types more than one space after a period, it's from typing-school habit. I know in my case, since the extra space looks bad between sentences in a proportional font, I'd rather they were both compressed the way HTML normally compresses whitespace.
Hmm, I have never had typewriting lessons, so I wasn't even aware of the habit to put two spaces between sentences. I've been searching the web though and the consensus seems to be to put only one space between sentences. So, users should either unlearn inserting two spaces, or live with the consequence :) I read somewhere that MS Word has a user pref to convert 2 spaces into 1 between sentences, but I think this is not justifiable for mozilla to include...?
The editor should not do anything to alter the content that you type in. If you type two spaces, two spaces MUST show up in the document. Please don't detract from the issue here.
Simon, are you talking about not altering the _content_, or not altering the _expected_presentation_? Right now if I type two spaces the generated markup is " &nbsp;", as mentioned previously, which is most definitely alteration of the content.
not altering the expected presentation.
Right now the generated content is like: blah blah&nbsp; blah&nbsp; <br> Wouldn't it be easyer to make it like blah blah &nbsp;blah &nbsp;<br> I see two advantages. One: you only need a counter (boolean even), the first space is a regular space and later spaces are &nbsp;. Instead of altering previous spaces you just need to insert the right character depending on the counter/boolean. The second advantage is that the last space would not be ignored, as the user probably expects. This is visible when a border is drawn around the content.
> blah blah &nbsp;blah &nbsp;<br> So when it wraps you have all these spaces at the beginning of the next line? Right now wrapping acts sorta sane...
Hm, I guess you don't want the spaces at the beginning of the next line when wrapping. And the last space not showing up when a border is drawn around it is not that important to me, and even desired behaviour for some (comment #23). So I guess I'm okay with everything :)
Need some action here for 1.7b. mscott, are you the right owner? /be
Flags: blocking1.7b?
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.7?
Flags: blocking1.7? → blocking1.7+
Flags: blocking1.7a-
The bug that caused this regression was backed out, correct? If so, then this shouldn't be a 1.7 blocker.
Flags: blocking1.7+ → blocking1.7-
It'd really be nice to get some traction here so we can fix the layout bug that editor is depending on...
Keywords: helpwanted
Sorry, I'm not clear on what the issue here is. Why would the spaces not render in the editor? Are they no longer nbsp? What was the code that was backed out? Someone needs to clue me in a bit since most of the conversation is > 6 months old. This sounds like a caret issue more than editor or is it tied to the insertion of nbsp's and spaces? Was caret navigation also broken when this was broken?
As far as I can see, everything is okay now. Typing a 2nd space converts the 1st space to &nbsp;.
Is this issue resolved? It seems to still exists in TB 0.9 (build 20041103). The problem is - it is really hard to reproduce it reliably - it happens very often, but in a non-consistent way. Sometimes it is enough to save a message as draft and reopen it to cure the problem.
Product: MailNews → Core
I just experienced this problem in version 1.0.2 (20050317).
This patch and the one in bug 132561 should be able to go in once bug 287813 lands.
Depends on: 287813
Summary: Regression in editor, spaces not showing up → when layout whitespace trimming is fixed, spaces don't show up in editor until next character typed
(In reply to comment #53) > This patch and the one in bug 132561 should be able to go in once bug 287813 > lands. Bug 287813 is fixed now.
Assignee: mscott → dbaron
Status: ASSIGNED → NEW
Keywords: helpwanted
Whiteboard: [patch]
Attached patch updated patch — — Splinter Review
I've had this in my tree all this time.
Attachment #142193 - Attachment is obsolete: true
Attachment #218946 - Flags: superreview?(bzbarsky)
Attachment #218946 - Flags: review?(mrbkap)
Comment on attachment 218946 [details] [diff] [review] updated patch Looks good.
Attachment #218946 - Flags: review?(mrbkap) → review+
Comment on attachment 218946 [details] [diff] [review] updated patch mmmm.... tasty! ;)
Attachment #218946 - Flags: superreview?(bzbarsky) → superreview+
Checked in to trunk. I'm sure it'll break something.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
(In reply to comment #58) > Checked in to trunk. I'm sure it'll break something. If I have: sss and I delete the last s using the Delete key, the caret goes before the first s, and remains painted stationary, but then correctly returns to its position after the second s and blinks.
(In reply to comment #59) > > If I have: > > sss > > and I delete the last s using the Delete key, the caret goes before the first > s, and remains painted stationary, but then correctly returns to its position > after the second s and blinks. Sorry for both of these spam messages; that is bug 334608.
Depends on: 335560
Depends on: 336408
(In reply to comment #58) > Checked in to trunk. I'm sure it'll break something. Not exactly broken, but see bug 363624: still a bit of room for improvement. I think this did improve bug 87314 somewhat.
Depends on: 394157
Product: Core → MailNews Core
Depends on: 437679
(In reply to comment #58) > Checked in to trunk. I'm sure it'll break something. Looks like Bug 437679 cause of this fix, or at least this is only bug close to regression window changes I found.
URL: 437679
URL: 437679
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: