composer changes font mid email

Bug #584632 reported by ricardisimo
32
This bug affects 7 people
Affects Status Importance Assigned to Milestone
Mozilla Thunderbird
Fix Released
High
thunderbird (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

Binary package hint: thunderbird

As I'm typing my emails in Thunderbird, I can see what appears to be a font size change on screen, normally in the second line of text. The second line appear smaller than the first. It's barely perceptible, so half them time I think I am imagining it.

Well, I've started Bccing to myself to check, and the emails I am receiving from myself are not only a different size, they're also a different font. Composer starts in some default serif, and by the second line is sans. I'd bee glad to email someone viz thunderbird, and also send along a screenshot of how it looks while I am typing.

Thanks.

Revision history for this message
In , Esther (esther) wrote :

Using trunk builds 20030428 onw winxp, macosx and linux if the user changes
their Mail composition default for font style, the prepopulated text used in
Replys "xyx wrote:" does not change to this default. If the user backspaces in
the first line all the way to the left, the newly typed text picks up the font
style of this pre populated text.

1. Launch app - set to reply above quoted text (default for commercial builds)
2. Change your mailnews preference for composition to another font style.
3. Click Reply to a message someone sent preferably with the default font style.
4. Start typing (note you will see your default style), backspace all the way
to the left margin, start typing again.

Result: the font is now of the style of the prepopulated text instead of your
default text.

Expected: to be able to backspace and still have my default font used.

Revision history for this message
In , Esther (esther) wrote :

nomintaing because this is a new feature and mistakenly backspacing in a reply
can look as though this feature doesn't work all the time.

Revision history for this message
In , Esther (esther) wrote :

Also happens when Forwarding Inline, the prepopulated hearder of the forwarded
message has the same affect at the prepopulated text in a reply.

Revision history for this message
In , Sspitzer (sspitzer) wrote :

over to shuehan

Revision history for this message
In , Samir-bugzilla (samir-bugzilla) wrote :

adt: nsbeta1-

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 251364 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 245581 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mcow (mcow) wrote :

See (from the dupe) bug 245581 comment 1.

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 273622 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 287013 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 353424 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mcow (mcow) wrote :

*** Bug 373904 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Rob Hasselbaum (rhasselbaum) wrote :

Created attachment 260245
Use HTML font preference for reply and forward headers

With this patch, Thunderbird uses the user's preferred HTML font face/size for reply and forward header text. I added logic in nsMsgCompose and the MIME routines to wrap header text in font markup if we are in HTML composition mode and a non-default font face or size was chosen in preferences.

Revision history for this message
In , Rob Hasselbaum (rhasselbaum) wrote :

Created attachment 260269
Updated patch that addresses TT special case

Upon further testing, I discovered that the "Fixed Width" meta-font requires use of the TT tag instead of FONT. Updated the patch to address this special case.

Revision history for this message
In , Mscott-mozilla (mscott-mozilla) wrote :

Comment on attachment 260269
Updated patch that addresses TT special case

some minor nits:

+ nsXPIDLString font_face;

font_face should be a nsString

This:

+ if(NS_LITERAL_STRING("tt").Equals(font_face))

should be
font_face.EqualsLiteral("tt")

and does it need to be a case insensitive compare?

Can this:

aText.Append(NS_LITERAL_STRING("</TT>"));

be aText.AppendLiteral(...)?

Use a nsString here:
+ nsXPIDLString font_size;

could this be a simple switch statement?

+ if(NS_LITERAL_STRING("x-small").Equals(font_size))
+ font_size.Assign(NS_LITERAL_STRING("-2"));
+ else if(NS_LITERAL_STRING("small").Equals(font_size))
+ font_size.Assign(NS_LITERAL_STRING("-1"));
+ else if(NS_LITERAL_STRING("large").Equals(font_size))
+ font_size.Assign(NS_LITERAL_STRING("+1"));
+ else if(NS_LITERAL_STRING("x-large").Equals(font_size))
+ font_size.Assign(NS_LITERAL_STRING("+2"));
+ else if(NS_LITERAL_STRING("xx-large").Equals(font_size))
+ font_size.Assign(NS_LITERAL_STRING("+3"));
+ else
+ font_size.Assign(NS_LITERAL_STRING(""));

if not, the comparison statements should be of the form:

if (font_size.EqualsLiteral("xx-large"))

* NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "msgcompose.font_size", NS_LITERAL_STRING(""), font_size);

why don't we just read the pref directly from the pref service instead of using this localized unichar preference routine. I don't think this pref is a localized pref.

I haven't looked at the mime part yet, I'll try to get to that soon! thanks for taking a look at this.

Revision history for this message
In , Notagain (notagain) wrote :

this still happens with tb 3.0b2pre nightlies. both the reply and forward header cases. any work done? will this be solved anytime soon (3.0 final perhaps)?

Revision history for this message
In , Mozilla-mawer (mozilla-mawer) wrote :

This would be a welcome fix in Thunderbird 3 -- using a sans serif font (eg. Arial) in Thunderbird at the moment looks very unprofessional, as whenever you reply you end up with a serif font for the "XYZ wrote" text...

Revision history for this message
In , Gjunk (gjunk) wrote :

I have same problem using 3.0b2 nightly as of 20090513.

Also - the font size selector is too coarse - medium or large - we really need to be able pick the font size we use.

thanks.

Revision history for this message
In , Rick (rick2910) wrote :

Agree fully with comment #17: please create a dropdown with font sizes to choose from instead of 'larger' and 'smaller' buttons.

Revision history for this message
In , Gjunk (gjunk) wrote :

Bug is still there with build as of 20090528 .. both bugs - the font not suddenly switching away from user specified default to something else and also the inability to choose a font size.

Revision history for this message
In , Gary-rumblingedge (gary-rumblingedge) wrote :

Comment on attachment 260269
Updated patch that addresses TT special case

Obsoleting due to rejected review.

Revision history for this message
In , Gjunk (gjunk) wrote :

 I dont understand comment #20 at all.

 I think the bug is this:

   Hit reply - start typing - your preference font/size is chosen - in mid type is switches to something else - different font different size.

    My default is sans-serif/large - it usually switches to variable/medium.

  End resault is reply contains new text 1/2 in preference compose font and 1/2 in thunderbird default font.

   WHy is this not a bug - this happens with this build:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090617 Lightning/1.0pre Shredder/3.0b3pre ID:20090617031506

  Which is hardly an obsoleted build.

Revision history for this message
In , Bugzilla-standard8 (bugzilla-standard8) wrote :

(In reply to comment #21)
> I dont understand comment #20 at all.
...
> Which is hardly an obsoleted build.

The comment didn't say the bug was obsolete, it said the *patch* was obsolete.

Revision history for this message
In , Gjunk (gjunk) wrote :

Ooooohhhh silly me ..

sorry

:-|

Revision history for this message
In , Ulysse (ulysse) wrote :

*** Bug 480815 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Josh Kinnison (kinnison-josh) wrote :

This bug occurs even if you just click an area with a non-default font. It is amazingly frustrating. It feels like playing Minesweeper when all I want to do is write an email.

27 comments hidden view all 230 comments
Revision history for this message
ricardisimo (ricardisimo) wrote :

Not one bite in over a month? Would no one even like to see an email from me?

Revision history for this message
bmaupin (bmaupin) wrote :

I had this same problem and it drove me nuts. My problem is that when I reply to an existing email, the font will be "Helvetica, Arial". This is what's listed when I go to Edit --> Preferences --> Composition --> General --> HTML --> Font. But if I click away from the body of the email, even if I click to another part of the composition window such as the subject box, as soon as I click back to the body, the font will change to some kind of serif font. It says "Variable Width" in the font box. I think it's whatever font is listed in Edit --> Preferences --> Display --> Formatting --> Fonts --> Default font. My configuration says the default font is "serif."

If I type a completely new message, I don't seem to have the problem as much.

My solution was to simply set the font settings in the locations I mentioned above to the same font. So I went to Edit --> Preferences --> Display --> Formatting --> Fonts --> Default font, set it to Arial, and it hasn't happened again, yet--it wasn't that long ago that I made the change, so we'll see how well it works.

27 comments hidden view all 230 comments
Revision history for this message
In , Mike001 (mike001) wrote :

*** Bug 536613 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Mike001 (mike001) wrote :

Bug 533613 was DUP'd to this bug because the reporter of Bug 533613 agreed that the symptoms and STR described in Bug 273622 matched what he was trying to report. It is not clear to me that this bug report (203810) actually describes those same symptoms, nor that the proposed fix will fix the problem noted in the Description in Bug 273622. Therefore, if the patch for this bug WILL NOT address the problem noted in Bug 273622, I respectfully request that the assignee of this bug:
a) Reopen Bug 273622; and,
b) Mark Bug 533613 as a duplicate of Bug 273622.
Doing so will ensure that the problem noted in Bug 273622 DOES get fixed.

Revision history for this message
In , N-hillyer (n-hillyer) wrote :

Can we assume that this is an HTML problem only? If so I have failed to find where it is clearly stated.

I am not sure if my remarks below will be of any help to those working on this bug or users who end up here.

I seldom compose with HTML and don't see this bug with TB 3.1.2 on a G4 with OS X 10.5.8.

However, I did have a difficult to track down font issue with an identical G4. I eventually discovered that most of the considerable fonts in
/Applications/Thunderbird.app/Contents/MacOS/greprefs/all.js were set to zero size. I removed many unwanted fonts and set all the others to reasonable sizes. This cured my problems with this G4. The other identical G4 has not had the problem - on checking I see that all its font sizes in greprefs are reasonable. Although the G4s are identical their TB update histories may be different ie fresh installs vs updates may have varied from time to time.

Revision history for this message
In , Sgtfool (sgtfool) wrote :

A variation on this is that it also loses the default font when I click on the subject line and then click back to my text.

Although I love thunderbird, about once a month I search for ways to get outlook to replicate my most beloved add-on's because it is so frustrating to use an e-mail program that will not let me control my font style (and size).

Revision history for this message
In , Gjunk (gjunk) wrote :

Bug is still here in 3.1.7 nightly as of last night ...

amazing this bug is over 7 years old ...

do the experts know what to fix is yet ?

font flipping makes for ugly emails ;-( ...

30 comments hidden view all 230 comments
Revision history for this message
bmaupin (bmaupin) wrote :

Wow, looks like this bug just might be 8 years old. Guess I'd better get used to it...

https://bugzilla.mozilla.org/show_bug.cgi?id=203810

Changed in thunderbird:
importance: Unknown → Medium
status: Unknown → Confirmed
31 comments hidden view all 230 comments
Revision history for this message
In , Mike-cloaked (mike-cloaked) wrote :

Truly astounding as this bug comes up to almost 8 years old - I am using 3.1.10pre and this bug is still present - it bites me pretty much every week - sometimes on the next line when composing and sometime in the middle of a line and yes with html email.....

Surely someone could give this bug a little love and sort it out once and for all?

Revision history for this message
In , Gjunk (gjunk) wrote :

also bug is still in todays Miramar 3.3a4 pre ... bit sad really .. :-(

Bug is assigned to nobody presently ... there were some suggestions on how to fix this in 2007 ... may not help tho ..

Revision history for this message
McFly81 (christian-lange-81) wrote :

Still present TB 3.1.9 (Ubuntu 12.04 LTS). When I compose a Mail. the font-size and font-type keeps changig. Every few lines I have to manually mark my text and change font-size/type myself. Very annoying.
Additionally, when I copy-paste some text (e.g. a URL from my browser address bar) this text gets some formatting. Mosty some ugly one not in line with the remainder of the mail.

Revision history for this message
McFly81 (christian-lange-81) wrote :

Sgtfool wrote: "A variation on this is that it also loses the default font when I click on the subject line and then click back to my text."
I can confirm that.

Revision history for this message
In , N-hillyer (n-hillyer) wrote :

I doubt that this 8 year old bug will ever be fixed.

Perhaps users should use text rather than HTML.

Changed in thunderbird (Ubuntu):
status: New → Confirmed
Revision history for this message
In , 5u-mozilla (5u-mozilla) wrote :

I have come to the edge of abandoning Thunderbird due to this 9 1/2 year old bug. People reply to my replies and I see that the format of what I sent them makes it look like it was written by a 7-year old with crayons. It's a hodgepodge of two different fonts/sizes -- embarrassing and unprofessional.

Is there a work-around or a fix? (People working with TB 3 in 2010 had some success with setups involving the add-ons Quote & Reply and QuoteandComposeManager. But before I wade into those waters I would like a little assurance that it can be made to work.)

Changed in thunderbird:
status: Confirmed → Invalid
B Bobo (yout-bobo123)
affects: thunderbird → seamonkey
affects: seamonkey → thunderbird
bmaupin (bmaupin)
Changed in thunderbird:
importance: Medium → Unknown
status: Invalid → Unknown
Changed in thunderbird:
importance: Unknown → High
status: Unknown → Confirmed
150 comments hidden view all 230 comments
Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8582923
Easy fix for test due to new selection behaviour (move commands)

This is the last of the updated tests.

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8582931
Unified patch (code + test changes) ready for next push to try.

Pushed to try server (thanks guys for the support!):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1

Aryeh: Please help me review the results.

Actually, I followed what was suggested for the try:
try: -b do -p all -u all[x64] -t none

Just out of interest: Why build on all platforms and then just execute on Linux? I mean, why build it in the first place? Just to see that it compiles? If it already compiled once, and I only rerun tests, that doesn't make too much sense.

Further steps: If this try run goes well, the only thing missing is a test for the changes I made. Two scenarios are already covered by the existing tests which I had to change: the "moving around with the left arrow" and the "jumping to the end of the line with a key stroke". What's missing is the "clicking beyond the end of the line". The first try run in comment #71 was done with only the code to fix the clicking. No tests failed, so there wasn't a test for this, so it needs to be added now.

Revision history for this message
In , Aryeh Gregor (simetrical+launchpad) wrote :

(In reply to Jorg K from comment #106)
> Pushed to try server (thanks guys for the support!):
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1

Treeherder isn't loading for me right now, but someone else who looked said it seemed fine.

> Just out of interest: Why build on all platforms and then just execute on
> Linux? I mean, why build it in the first place? Just to see that it
> compiles? If it already compiled once, and I only rerun tests, that doesn't
> make too much sense.

Yes, to see that it compiles. Different platforms use different compilers, and maybe sometimes different compiler versions, and they treat different things as errors. Compiling the code on all platforms is usually a good resources-safety tradeoff. (Ideally all tests would be run on all platforms, but it overloads the try servers.)

> Further steps: If this try run goes well, the only thing missing is a test
> for the changes I made. Two scenarios are already covered by the existing
> tests which I had to change: the "moving around with the left arrow" and the
> "jumping to the end of the line with a key stroke". What's missing is the
> "clicking beyond the end of the line". The first try run in comment #71 was
> done with only the code to fix the clicking. No tests failed, so there
> wasn't a test for this, so it needs to be added now.

Sounds good!

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Is it worth running it on other systems as well since I found some - most likely unrelated - problems (see comment #97) when running it locally on Windows 7? Or are these known problems that always fail?

Revision history for this message
In , Aryeh Gregor (simetrical+launchpad) wrote :

Try running the tests locally without your patches if you want to be sure (hg qpop -a will get rid of them if you're using mq). If they fail even without your patches, don't worry about it. In theory all tests should work on all supported platforms and configurations, but in practice some small fraction will break on some systems for various reasons, and in practice we only require that they work on the try servers.

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8583259
Test case

Here is the test case. Forgive me if the JS is bad, I've never written any, just copied bits and pieces around.
Two questions:
Where in the mochitest.ini do I put my new test? I put it right at the front since I don't understand the syntax of "skip-if". Does that skip the next line? Perhaps you can suggest a line where it should go. Or say: After such and such.

In the test I use a <span>123</span> to get offset 3 when clicking behind it. Without the span, I get offset 4, which I find surprising, or clicking at the front gives offset 1 instead of 0. FF 36 does the same, so I guess it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why does the span make a difference?

Anyway, the tests are passed. Without my changes, the first test fails as expected.

As I said in comment #106: There are already tests for hitting the "end" key and navigating with the arrow keys, so this test should be the only one we need additionally.

Revision history for this message
In , Aryeh Gregor (simetrical+launchpad) wrote :

(In reply to Jorg K from comment #110)
> Where in the mochitest.ini do I put my new test? I put it right at the front
> since I don't understand the syntax of "skip-if". Does that skip the next
> line? Perhaps you can suggest a line where it should go. Or say: After such
> and such.

Just add a line that says
  [test_bug756984.html]
(assuming that's the file's name). Put it right before the next test line, like "[test_bug784410.html]" or whatever. The "skip-if", "support-files", etc. lines apply to the preceding test file, so don't put it before one of those lines. (This is confusing. I looked it up in the online docs: https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/build/buildsystem/test_manifests.html)

> In the test I use a <span>123</span> to get offset 3 when clicking behind
> it. Without the span, I get offset 4, which I find surprising, or clicking
> at the front gives offset 1 instead of 0. FF 36 does the same, so I guess
> it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why
> does the span make a difference?

You have a newline at the beginning of the div. That's the first character in the text node (although it doesn't normally render). If you did

  <div id='div1'>123<br>456<br></div>

all on one line, the offsets would be as you expected even without the span.

> Anyway, the tests are passed. Without my changes, the first test fails as
> expected.
>
> As I said in comment #106: There are already tests for hitting the "end" key
> and navigating with the arrow keys, so this test should be the only one we
> need additionally.

Sounds great!

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

I'll revise the test and post a new one in a second.

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8583354
Test case (revised)

If this is good to go, I'll submit another "unified" patch (code + all tests), if necessary.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :
Download full text (4.5 KiB)

Comment on attachment 8583354
Test case (revised)

Review of attachment 8583354:
-----------------------------------------------------------------

Looks like a great start! Did you intend to test the rest of the cases in a separate patch?

::: layout/generic/test/test_bug756984.html
@@ +13,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=756984">Mozilla Bug 756984</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +</div>

Since you mentioned you're new to writing tests, I'll add some bits about how this stuff works here. I hope that helps!

The above is basically boilerplate that we include in most mochitests. The only parts that are really required are the two script elements that load the SimpleTest and event synthesis APIs.

@@ +25,5 @@
> +
> + /** Test for Bug 756984 **/
> + /** We test that clicking beyond the end of a line terminated with <br> selects the preceding text, if any **/
> +
> + SimpleTest.waitForExplicitFinish();

This is used to tell the test harness to not stop the test once the page load finishes (which is the default.) This is required for all tests that can run past that point. For example, this test is asynchronous because of the waitForFocus call below.

@@ +27,5 @@
> + /** We test that clicking beyond the end of a line terminated with <br> selects the preceding text, if any **/
> +
> + SimpleTest.waitForExplicitFinish();
> +
> + SimpleTest.waitForFocus(function() {

This is required to ensure that the test window is raised to the top on the desktop, which is needed to ensure proper event delivery.

@@ +35,5 @@
> + var sel = window.getSelection();
> + var f = document.getElementById("div1");
> + theDiv.focus();
> + sel.collapse(theDiv, 0);
> + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);

The last argument of synthesizeMouse is the window object corresponding to the element that should receive the event. If omitted, the default is the current window object. Now, f is an HTMLDivElement, which doesn't have a contentWindow property, so f.contentWindow ends up as undefined. The reason this works is that from the perspective of the callee function, omitted arguments are undefined, so that's what this function checks for: <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#814>.

So please just remove this last argument here and elsewhere in the test.

@@ +36,5 @@
> + var f = document.getElementById("div1");
> + theDiv.focus();
> + sel.collapse(theDiv, 0);
> + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);
> + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);

Nit: You can just issue one synthesizeMouse as {type: "click"} and it will dispatch both of these events internally.

@@ +37,5 @@
> + theDiv.focus();
> + sel.collapse(theDiv, 0);
> + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);
> + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow);
> + var selObj = window.getSelection();

Nit: You already have |sel| ...

Read more...

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8584346
Unified patch (code + test changes + twice revised new test)

Thank you very much for the review and all the additional explanation.
I hope I could address your questions in additional comments in the test.

Sadly switching from "mousedown/up" to "click" as you had suggested makes the test fail with:
failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at chrome://specialpowers/content/specialpowersAPI.js:161

This is actually the only test I intended to submit ;-)

This is for the case where the user clicks beyond the end of the line.
There are two tests already which I had to adapt to the new selection behaviour which test the moving to the end
(editor/libeditor/tests/test_selection_move_commands.xul)
and moving from the subsequent line using left arrow
(layout/generic/test/test_movement_by_characters.html).

If you wanted to be really purist a test with a "div contenteditable" could be added where an empty line is first filled with some text, which is then removed so the range becomes empty. The code caters for this case.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to Jorg K from comment #115)
> Created attachment 8584346
> Unified patch (code + test changes + twice revised new test)
>
> Thank you very much for the review and all the additional explanation.
> I hope I could address your questions in additional comments in the test.
>
> Sadly switching from "mousedown/up" to "click" as you had suggested makes
> the test fail with:
> failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure
> code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at
> chrome://specialpowers/content/specialpowersAPI.js:161

Err, sorry, I misspoke. I should have said: "remove the type value" altogether (just pass an empty object `{}'). That should do the right thing: <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#272>

> This is actually the only test I intended to submit ;-)
>
> This is for the case where the user clicks beyond the end of the line.
> There are two tests already which I had to adapt to the new selection
> behaviour which test the moving to the end
> (editor/libeditor/tests/test_selection_move_commands.xul)
> and moving from the subsequent line using left arrow
> (layout/generic/test/test_movement_by_characters.html).
>
> If you wanted to be really purist a test with a "div contenteditable" could
> be added where an empty line is first filled with some text, which is then
> removed so the range becomes empty. The code caters for this case.

I think relying on the selection movement tests is sort of OK, but I really prefer to have much better testing on this, since this code is extremely fragile and I'm pretty sure that without extensive tests, we'll end up breaking it again in the future. That being said, I guess it's not fair for me to make you do this. :-)

But at the very least, we should have tests for clicking past the end of the line with the line terminating with a bunch of inline elements that have a text node inside them. That is essentially what this bug is really about. So please add some test along those lines.

Also, to reiterate, the second test in your test case is wrong...

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8584608
Unified patch (code + test changes + three times revised new test)

Here comes the updated test. This time with some additional inline elements.

I'm not sure why the second test should be wrong, I'll add another attachment in a second to convince you ;-)

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8584620
Here comes another manual test.

Here some HTML to run a test by hand.

If clicking behind any of the eight lines in this test, "DIV" will be returned in the current version of FF.

With the new behaviour clicking behind lines 1-6 and 8, "#text" will be returned, but for the empty line 7, DIV is still returned.

If the code were wrong, this line couldn't be clicked at all and the caret would move to the previous or subsequent line. I know, because at first my code didn't cater for this case.

IMHO my test is correct, if you don't think so, please be more specific.

Changing the subject:
I'd like to see this landed one day very soon. I'd also request to land it on mozilla38, so TB 38 can pick it up. I don't want to mention again that this is fixing a long-standing TB bug that people have been pulling their hair out for over 10 years now.

If you really want more tests to be written, I can do that, but *afterwards* ;-)

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to Jorg K from comment #118)
> Created attachment 8584620
> Here comes another manual test.
>
> Here some HTML to run a test by hand.
>
> If clicking behind any of the eight lines in this test, "DIV" will be
> returned in the current version of FF.
>
> With the new behaviour clicking behind lines 1-6 and 8, "#text" will be
> returned, but for the empty line 7, DIV is still returned.
>
> If the code were wrong, this line couldn't be clicked at all and the caret
> would move to the previous or subsequent line. I know, because at first my
> code didn't cater for this case.
>
> IMHO my test is correct, if you don't think so, please be more specific.

OK, I understand what you want to test now. So your test is actually correct, it was the comments where you mentioned clicking past the *second* line that confused me. :-)

> Changing the subject:
> I'd like to see this landed one day very soon. I'd also request to land it
> on mozilla38, so TB 38 can pick it up. I don't want to mention again that
> this is fixing a long-standing TB bug that people have been pulling their
> hair out for over 10 years now.

Sorry but this patch is not suitable for backporting into Aurora (which will soon become Beta) at this point, because it's quite risky in terms of the possibility to cause regressions. It needs to land on trunk and ride the trains. I understand that you'd like to see it fixed as soon as possible, but please be a bit more patient. :-)

> If you really want more tests to be written, I can do that, but *afterwards*
> ;-)

OK, that's fair. :-)

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 8584608
Unified patch (code + test changes + three times revised new test)

Review of attachment 8584608:
-----------------------------------------------------------------

Can you please revise the commit message to explain what the patch does? Something like: "Bug 756984 - Collapse the selection on the last text node on the line, skipping br and inline frames when clicking past the end of line; r=roc,ehsan".

Thanks again! Once you address these nits, this is ready to be checked in.

::: layout/generic/test/test_bug756984.html
@@ +43,5 @@
> + is(selRange.endContainer.nodeName, "#text", "selection should be in text node");
> + is(selRange.endOffset, 3, "offset should be 3");
> + }
> +
> + // click beyond the second line (100px to the left and 2px down), expect DIV.

Nit: please make this "first line".

@@ +47,5 @@
> + // click beyond the second line (100px to the left and 2px down), expect DIV.
> + // This is the previous behaviour which hasn't changed since the line is empty.
> + // If the processing were wrong, the selection would end up in the preceing non-empty line.
> + theDiv = document.getElementById("div4");
> + sel = window.getSelection();

Nit: this is not needed.

Revision history for this message
In , Charles (tanstaafl-libertytrek) wrote :

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #119)
> (In reply to Jorg K from comment #118)
>> Changing the subject:
>> I'd like to see this landed one day very soon. I'd also request to land it
>> on mozilla38, so TB 38 can pick it up. I don't want to mention again that
>> this is fixing a long-standing TB bug that people have been pulling their
>> hair out for over 10 years now.

> Sorry but this patch is not suitable for backporting into Aurora (which will
> soon become Beta) at this point, because it's quite risky in terms of the
> possibility to cause regressions. It needs to land on trunk and ride the
> trains. I understand that you'd like to see it fixed as soon as possible,
> but please be a bit more patient. :-)

Oh god, I hope I'm mis-reading this...

Are you seriously saying that this bugfix will NOT land in time for TB 38? Meaning, we will have to another full YEAR to see it fixed?

Please tell me it ain't so.

As for regressions - so you're saying it is OK for major regressions to happen for the entire lifecycle of TB 31 (see all of the Address auto-entry bugs that are only just now being fixed), but not for Firefox?

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

Created attachment 8584639
Unified patch (code + test changes + four times revised new test)

This should be good to go then ;-)

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

(In reply to Charles from comment #121)

> Are you seriously saying that this bugfix will NOT land in time for TB 38?
> Meaning, we will have to another full YEAR to see it fixed?

Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks, that's 10 months ;-)
However, everyone is free to compile their own version, which is what I'll do.

Revision history for this message
In , Charles (tanstaafl-libertytrek) wrote :

(In reply to Jorg K from comment #123)
> (In reply to Charles from comment #121)
>
> > Are you seriously saying that this bugfix will NOT land in time for TB 38?
> > Meaning, we will have to another full YEAR to see it fixed?
>
> Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks,
> that's 10 months ;-)
> However, everyone is free to compile their own version, which is what I'll
> do.

Fine for us, but what about companies using TBird...

<sigh> I just don't get this kind of reluctance... even if there were regressions, they could easily be fixed...

Oh well, I guess beggars cannot be choosers...

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Charles, this patch affects more than Thunderbird. I'm not sure what the usual practices are with regards to stuff that are regression prone in Thunderbird, but in Firefox, we are usually very conservative, and prefer to give things more time to bake.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

Comment on attachment 8584639
Unified patch (code + test changes + four times revised new test)

Review of attachment 8584639:
-----------------------------------------------------------------

Thanks, looks great!

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

(In reply to Charles from comment #124)

IMHO the problem is in the coupling between TB and FF. I think it's quite reasonable that the FF people are more conservative. Who would want to break a browser with a big market share.

TB on the other hand is client software. An nothing should stop client software to pick its foundation and additional patches. I've seen client software (KompoZer, or for example KDE with its so-called "build dependencies") where I couldn't believe my eyes how much they patch the underlying components.

This discussion, however, should be continued elsewhere.

Revision history for this message
In , Charles (tanstaafl-libertytrek) wrote :

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #125)
> Charles, this patch affects more than Thunderbird. I'm not sure what the
> usual practices are with regards to stuff that are regression prone in
> Thunderbird, but in Firefox, we are usually very conservative, and prefer to
> give things more time to bake.

Yes, but Firefox is primarily a web browser - just how bad could a regression in some code in the editor be?

That said, Jorg is correct that this isn't the right place for such a conversation...

So, all that is left is to say THANK YOU!!! to Jorg, Ehsan, and everyone else involved for finally getting this bug squashed!

Revision history for this message
In , Malix (malix0) wrote :

I'm really sad that this patch will not land in TB 38. But I want to make a big thank to Jorg that is working on the correction of this long standing bug. Also thanks to Ehsan that worked this in the past.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to Charles from comment #128)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #125)
> > Charles, this patch affects more than Thunderbird. I'm not sure what the
> > usual practices are with regards to stuff that are regression prone in
> > Thunderbird, but in Firefox, we are usually very conservative, and prefer to
> > give things more time to bake.
>
> Yes, but Firefox is primarily a web browser - just how bad could a
> regression in some code in the editor be?

Potentially very bad. :-) Any regression that can break lots of websites is very bad.

Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :
Revision history for this message
In , Aryeh Gregor (simetrical+launchpad) wrote :

For those who want to see this in Thunderbird 38 -- I suggest talking to the Thunderbird people and asking them if they can cherry-pick the patch for Thunderbird without affecting Firefox. If it's really a huge improvement for them, maybe they'll be willing to accept it despite lack of testing. Perhaps file a bug against the Thunderbird product, or get on IRC/e-mail/etc. with the appropriate people.

Revision history for this message
In , Philringnalda (philringnalda) wrote :
Revision history for this message
In , Charles (tanstaafl-libertytrek) wrote :

Ummm... ok, but... who are these 'Thunderbird people', and how do we contact them?

Revision history for this message
In , Fm-mozbugs (fm-mozbugs) wrote :

Personally, I never understood why this problem involved browsers at all.
It happens when COMPOSING email in THUNDERBIRD
and manifests when reading email in THUNDERBIRD.

Revision history for this message
In , Mozilla-jorgk (mozilla-jorgk) wrote :

(In reply to maybe-the-one from comment #135)
> Personally, I never understood why this problem involved browsers at all.

That you don't understand the facts doesn't change them ;-)

Let me explain: Thunderbird is client software that sits on top of Mozilla core software. The so-called "editor" that is used in Thunderbird when writing an e-mail messase is Mozilla core software. It is also used in Firefox: Please go to http://www-archive.mozilla.org/editor/midasdemo/ to check that even in Firefox you can "write".

The problem happens when losing the font goes unnoticed while writing. You will notice it when reading a reply that contains your original ill-formatted message.

The (In reply to Charles from comment #134)
> Ummm... ok, but... who are these 'Thunderbird people', and how do we contact them?

They are aware of the problem via this meta bug 1142879.

Revision history for this message
In , Fm-mozbugs (fm-mozbugs) wrote :

OK, thanks for explaining. I did not know that there was shared code.
And indeed, facts are facts even when one is ignorant of them.

Revision history for this message
In , Fm-mozbugs (fm-mozbugs) wrote :

Is there any way we can lobby for landing it in 38?

Changed in thunderbird:
status: Confirmed → Fix Released
Revision history for this message
In , Ehsan-mozilla (ehsan-mozilla) wrote :

(In reply to maybe-the-one from comment #138)
> Is there any way we can lobby for landing it in 38?

Please move that discussion to bug 1142879 or elsewhere. Thanks!

Revision history for this message
In , Mike-cloaked (mike-cloaked) wrote :

The progress on squashing this bug is excellent. One thought is that currently TB v39 is in DAILY channel (where this fix has now presumably landed) and is due to move to EARLYBIRD week of March 31 which is tomorrow. Presumably if users are on the Earlybird channel then they will get the fix once v39 moves to that channel in the next few days? Of course it would mean a longer delay for those running stable versions. Anyway congratulations on this superb work.

Revision history for this message
In , Fm-mozbugs (fm-mozbugs) wrote :

Release channel is at 31.5 as of this post.
Seems a long way to reach 39 assuming it moves +1 at a time

Revision history for this message
In , Mike-cloaked (mike-cloaked) wrote :

OK I guess the delay for one version is about two months if it does not get into v38?

Revision history for this message
In , Charles (tanstaafl-libertytrek) wrote :

No... if it doesn't get back-ported to 38, then it has to wait for version 45 - another, what, 10 months?

Revision history for this message
Paul White (paulw2u) wrote :

Upstream report was closed "RESOLVED FIXED" on 2015-03-29
Unable to reproduce with Thunderbird 60.6 in Ubuntu 18.04
No comments here or upstream for 4 years so closing as fixed

Changed in thunderbird (Ubuntu):
status: Confirmed → Fix Released
Displaying first 40 and last 40 comments. View all 230 comments or add a comment.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.