Closed Bug 235223 Opened 21 years ago Closed 18 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: 18 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: