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

RESOLVED FIXED in mozilla1.7beta

Status

MailNews Core
Composition
--
critical
RESOLVED FIXED
14 years ago
4 years ago

People

(Reporter: Scott MacGregor, Assigned: dbaron)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla1.7beta
x86
All
Dependency tree / graph
Bug Flags:
blocking1.7b -
blocking1.7 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
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.
(Reporter)

Updated

14 years ago
Severity: normal → critical
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird0.6

Comment 1

14 years ago
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.
(Reporter)

Comment 2

14 years ago
yeah...
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews
Target Milestone: Thunderbird0.6 → mozilla1.7beta
Version: unspecified → Trunk
(Reporter)

Comment 3

14 years ago
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?

Comment 4

14 years ago
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.

Comment 5

14 years ago
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-
(Reporter)

Comment 6

14 years ago
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
(Reporter)

Comment 7

14 years ago
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").
(Reporter)

Comment 9

14 years ago
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....
(Assignee)

Comment 11

14 years ago
I'd think this should be fixed in the caret-positioning code.
(Reporter)

Comment 12

14 years ago
no I see comment #0, I mis spoke in comment #9.
(Assignee)

Comment 13

14 years ago
Created attachment 142193 [details] [diff] [review]
patch

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

Comment 14

14 years ago
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.
(Reporter)

Comment 15

14 years ago
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?
(Reporter)

Comment 17

14 years ago
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.

Comment 20

14 years ago
*** Bug 233982 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 21

14 years ago
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.

Comment 22

14 years ago
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.
(Reporter)

Comment 25

14 years ago
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...

Comment 27

14 years ago
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.

Comment 28

14 years ago
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.
(Reporter)

Comment 30

14 years ago
*** Bug 236214 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 31

14 years ago
*** Bug 236370 has been marked as a duplicate of this bug. ***

Comment 32

14 years ago
*** Bug 236787 has been marked as a duplicate of this bug. ***

Comment 33

14 years ago
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

Comment 35

14 years ago
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.

Comment 36

14 years ago
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 ;-)

Comment 37

14 years ago
no longer a 1.7 blocker.
Flags: blocking1.7b?

Comment 38

14 years ago
(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.

Comment 39

14 years ago
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...?

Comment 40

14 years ago
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.

Comment 42

14 years ago
not altering the expected presentation.

Comment 43

14 years ago
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...

Comment 45

14 years ago
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?

Updated

14 years ago
Flags: blocking1.7b?
Flags: blocking1.7b-
Flags: blocking1.7?

Updated

14 years ago
Flags: blocking1.7? → blocking1.7+

Updated

14 years ago
Flags: blocking1.7a-

Comment 47

14 years ago
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

Comment 49

14 years ago
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?

Comment 50

14 years ago
As far as I can see, everything is okay now. Typing a 2nd space converts the 1st
space to &nbsp;.

Comment 51

14 years ago
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

Comment 52

13 years ago
I just experienced this problem in version 1.0.2 (20050317).
(Assignee)

Comment 53

13 years ago
This patch and the one in bug 132561 should be able to go in once bug 287813 lands.
Depends on: 287813
(Assignee)

Updated

13 years ago
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)

Updated

12 years ago
Assignee: mscott → dbaron
Status: ASSIGNED → NEW
Keywords: helpwanted
Whiteboard: [patch]
(Assignee)

Comment 55

12 years ago
Created attachment 218946 [details] [diff] [review]
updated patch

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+
(Assignee)

Comment 58

12 years ago
Checked in to trunk.  I'm sure it'll break something.
Status: NEW → RESOLVED
Last Resolved: 12 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. 

Updated

12 years ago
Depends on: 335560

Updated

12 years ago
Depends on: 336408

Comment 61

11 years ago
(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.

Updated

11 years ago
Depends on: 394157
Product: Core → MailNews Core
Depends on: 437679

Comment 62

8 years ago
(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.

Updated

8 years ago
URL: 437679

Updated

8 years ago
URL: 437679
You need to log in before you can comment on or make changes to this bug.