User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5 Steps to reproduce: Wrote a paragraph in the compose window. Selected the paragraph and clicked on the icon to increase text size. Actual results: The selected paragraph enlarged in sections to different font sizes - with some text ultra large, some text smaller and some text medium. Expected results: All the text should have changed to the same larger font.

The specific build tested is Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120622 Thunderbird/15.0a2 ID:20120622042001

This image shows the result of selecting a paragraph during compose and then highlighting the entered text before clicking on the letter "A" with the yellow slightly down pointing triangle arrowhead (i.e. second "A" to the right of the font box - which should have enlarged the text uniformly by one font size to x-large. Different parts of the text have been enlarged differently which should not happen.

Note that in the attachment at comment #2 above if the icon for the letter "A" with a slightly up pointing yellow arrowhead (i.e. the first icon to the right of the font box) is clicked to reduce the fonts back to the original as written, then the text returns uniformly to the original font size - if immediately after this the top menu item Format->Size are selected, and a specific size such as x-large is selected then the text does indeed change to the selected size perfectly uniformly and correctly. So it is the action of the font change icon when clicked that is buggy.

Correction in my description for the A icon for font increasing/reducing - I realised that the yellow triangle I referred to was in fact pointing "up" for increasing font size and pointing "down" for reducing font size - it is easy to mis-interpet the triangle as a right-pointing triangle angled slightly down or up! Anyway the remainder of this report will hopefully be clear enough that others can check my finding and confirm the problem - hopefully will be fixed. I will need to check whether this occurs for both composing new email as well as replying to incoming email but certainly in the reply case the problem occurs for me.

I changed the severity of this report as the problem makes the compose window largely unusable. I have had to revert to 14b3 to get a usable mail client again.

Can you get some better steps to repeat for this, like when composing the paragraphs are you changing styles as well, or clicking back and forth, and maybe copy and paste? I couldn't reproduce with just a simple write a paragraph or two and change the size.

Given the window of when this occured, I suspect it is a regression from bug 590640.

In the example that I took a screenshot for all I did was to reply to an incoming email and start typing the reply - then highlighted my entire reply section and hit the icon to enlarge the fontsize. That gave the screenshot I attached to this report. The mail I replied to was an html email and I was set to default compose as html with dejavu sans large font. Selecting to increase by one size should have uniformly changed the text to x-large but different sections of the text changed to different sizes as you saw. After that I tried various composes and thunderbird froze up when I tried to change font using the same method. So I reverted to 14b3 which is completely stable for me. I am not sure if writing a new email will reproduce the same bug or not - but I can send you the original email that I replied to (privately) if you want to try to write a reply to that email to confirm the problem?

It would be great if we can see the email you were replying to. That will help us reproduce and fix this problem.

Ehsan - I have emailed you privately with a forwarded email and some additional information on the problem.

I probably didn't account for increase/decreaseFontSize properly. They're not in the spec, so I don't think about them. :) You can work around it by setting a specific font size using the drop-down, correct? This would be much easier for me to deal with if you could reproduce it in Firefox, since I've never used Thunderbird in my life. Unfortunately, none of the rich-text editors I've looked at support the increase/decreaseFontSize command. I notice that Gmail doesn't have any corresponding button either, and I don't see any obvious way to do it in LibreOffice either. According to my notes, only Gecko and Opera support these for execCommand(). I don't suppose we could just drop support for the commands? Do we really need a separate feature for "increase/decrease font size", rather than just allowing users to pick the font size from a list? Who would be a good Thunderbird person to talk to about something like this? Alternatively, perhaps we could make "document.execCommand('increasefontsize')" shorthand for "document.execCommand('fontsize', false, Number(document.queryCommandValue('fontsize')) + 1)" so they don't go through their own magic codepaths. Anyway, I can reproduce *something* being messed up with increaseFontSize pretty easily, although I don't know if it's the exact same issue: data:text/html,<!doctype html> <div contenteditable>foo<b>bar</b>baz</div> <script> getSelection().selectAllChildren(document.body.firstChild); document.execCommand("increasefontsize", false, ""); </script> In Firefox 16 this only makes "baz" bigger. In Firefox 13 it makes the whole run bigger. I could probably fix this pretty easily, if we want to keep the feature at all.

Yes you are perfectly correct that selecting a specific size does not give rise to a problem - the main difference is that clicking the increase or decrease size is a "one click" operation whereas selecting a specific size takes three clicks! One possible way forward would be to replace the two increase/decrease size buttons by a single "change size" button and then have a menu open up to select a size from a list - that would be reasonably quick for the user and I expect that if this could be done as an easy alternative to the current increase/decrease size button I imagine it would be received well in general? Thank you so much for looking into this - and for initiating the process of implementing a fix. It is much appreciated.

If the Thunderbird developers agree to stop using these commands, I have no problem with ripping them out of editor!

You mean rip out the increase/decrease buttons? So long as there is still a way to set a specific font size it would be OK? The current way of using Format->Size->chosen font does work but needs three steps - having a button Size->chosen size doing the same thing would be two steps and I would be happy with that if it was possible?

(In reply to Mike Cloaked from comment #14) > You mean rip out the increase/decrease buttons? So long as there is still a > way to set a specific font size it would be OK? The current way of using > Format->Size->chosen font does work but needs three steps - having a button > Size->chosen size doing the same thing would be two steps and I would be > happy with that if it was possible? No I'm talking about the implementation of those buttons. Thunderbird should still be able to provide the same UI buttons, or different ones, that's the call of the Thunderbird developers.

For me too will be ok to remove the increase/decrease font size buttons, but prefere to have a drop down list with size, like others text editors Just my 2 cent

(In reply to Ehsan Akhgari [:ehsan] from comment #13) > If the Thunderbird developers agree to stop using these commands, I have no > problem with ripping them out of editor! So it sounds like Thunderbird developers have no objection here in principle. I've filed bug 769604 to ask them to remove use of cmd_increaseFont/cmd_decreaseFont, and bug 769605 for Composer. If we can get it out of comm-central, I'll write a patch here to kill it for webpages too, so we don't have to worry about regressions. :)

If this is approved and the cmd_increaseFont/cmd_decreaseFont are removed can a new command to give a pull down menu to select a font from a list be put in instead? i.e. just Size->select font which is one less step than Format->Size->select font in Thunderbird composer? By the way in Thunderbird in the composer for Format->Size there currently remains the two options to increase/decrease size at the moment - I presume those would be removed also?

When we say "command" here, we're talking about the underlying code which implements what the user asks the UI to do. Thunderbird can still provide the same UI if it wants.

OK Ehsan thanks for clearing up that point.

(In reply to :Aryeh Gregor from comment #11) > data:text/html,<!doctype html> > <div contenteditable>foo<b>bar</b>baz</div> > <script> > getSelection().selectAllChildren(document.body.firstChild); > document.execCommand("increasefontsize", false, ""); > </script> I can reproduce the upsize only applying to the baz text in TB trunk, but only if it is enclosed in div tags. This is really atypical for TB compositions, you don't normally see div tags unless the user has purposely inserted them (and only advanced users) More typical would be: <font face="Arial">foo<b>bar</b>baz</font> Which selects and up-sizes perfectly fine for me. So, please let's not talk about throwing out a feature that seems to wfm most of the time.

(In reply to Joe Sabash from comment #21) > I can reproduce the upsize only applying to the baz text in TB trunk, but > only if it is enclosed in div tags. > This is really atypical for TB compositions, you don't normally see div tags > unless the user has purposely inserted them (and only advanced users) > More typical would be: > <font face="Arial">foo<b>bar</b>baz</font> > Which selects and up-sizes perfectly fine for me. I gave one example that's problematic; comment #0 implies that users are hitting other scenarios in real-world use. Something about the feature got badly broken, evidently. > So, please let's not talk about throwing out a feature that seems to wfm > most of the time. As it stands, this is a regression. So we have to deal with it somehow before the uplift on July 16. We don't want to back out the responsible patches, so either we have to fix the feature or get rid of it. Personally I don't think that the feature is desirable to start with -- other browsers don't support it, and nor (AFAICT) does Gmail or LibreOffice. They all allow you to just select a specific font size, not increase or decrease. And it adds a maintenance burden. So getting rid of it is the best course of action, IMO. If we do wind up keeping it, I'm okay with doing the work to fix it before the Aurora uplift -- probably not a lot of work -- but I think it makes more sense to ditch it.

Regression from bug 752210.

Comment on attachment 638192 [details] [diff] [review] Proposed patch Review of attachment 638192 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/html/nsHTMLEditorStyle.cpp @@ +1762,5 @@ > if (aNode->IsElement() && aNode->AsElement()->IsHTML(nsGkAtoms::font) && > aNode->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::size)) { > // Cycle through children and adjust relative font size. > + for (PRUint32 i = aNode->GetChildCount(); i--; ) { > + nsresult rv = RelativeFontChangeOnNode(aSizeChange, aNode->GetChildAt(i)); Is this really correct? The number of children might change, right? I'd do something like nsIContent* child = aNode->GetLastChild(); while (child) { nsIContent* prevChild = child->GetPreviousSibling(); nsresult rv = RelativeFontChangeOnNode(aSizeChange, child); child = prevChild; } (Also, needs a test, naturally.)

Can you explain your patch please? It's not immediately clear to me why this patch should fix this bug. Also, as Aryeh mentioned, the number of children may change between loop iterations.

(In reply to Ehsan Akhgari [:ehsan] from comment #26) > Can you explain your patch please? It's not immediately clear to me why > this patch should fix this bug. Also, as Aryeh mentioned, the number of > children may change between loop iterations. The current code calls RelativeFontChangeOnNode or RelativeFontChangeHelper on child, which may wrap it in a <big> or <small>. *Then* it sets child = child->GetPreviousSibling(). So in a case like <blockquote>foo<b>bar</b>baz</blockquote> it wraps "baz" in <big>, then looks for the previous sibling -- which is null, because "baz" is now the only child of <big>. Using GetChildAt() will handle this case correctly, but it will probably fail in a case like increaseFontSize on <blockquote>foo<small>bar<i>baz</i></small></blockquote> where I'm guessing it will produce <blockquote><big>foobar</big><i>baz</i></blockquote> which scales up "bar" by two steps, not one.

(In reply to Ehsan Akhgari from comment #26) > Can you explain your patch please? It's not immediately clear to me why > this patch should fix this bug. Also, as Aryeh mentioned, the number of > children may change between loop iterations. Sure, this patch simply switches back to a loop as in the pre-bug 752210 code. The point of using a descending counter is that any extra children appear after the children that have yet to be processed. In Aryeh's specific case, foo and bar are distinct text nodes, so there's no issue there. (If you remove the italics around bas, the two separate text nodes for bar and baz remain.)

Aha, of course. Yes, doing it backwards makes it work, because processing one child won't affect any child with lower index. I suggest you add a one-line comment to that effect, and a test.

Comment on attachment 638192 [details] [diff] [review] Proposed patch OK, this makes sense. But please add a test here too!

Neil asked me to write a test, so here you go. This uses execCommand(), so if we stop exposing these commands to the web while Composer or something still supports them, we'll have to remove or rewrite the regression test. Try: https://tbpl.mozilla.org/?tree=Try&rev=99ea14ae0402

Comment on attachment 639292 [details] [diff] [review] Test I'd say that this is a perfectly cromulent test :-)

Comment on attachment 639292 [details] [diff] [review] Test Review of attachment 639292 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 638192 [details] [diff] [review] Proposed patch Pushed mozilla-inbound changeset aa7493c796ce.

Comment on attachment 639292 [details] [diff] [review] Test Pushed mozilla-inbound changeset 7851dc0f395c.

https://hg.mozilla.org/mozilla-central/rev/aa7493c796ce https://hg.mozilla.org/mozilla-central/rev/7851dc0f395c

Comment on attachment 638192 [details] [diff] [review] Proposed patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 752210 User impact if declined: Composer functionality broken Testing completed (on m-c, etc.): Second patch provides test Risk to taking this patch (and alternatives if risky): Back out 752210 String or UUID changes made by this patch: None

Comment on attachment 639292 [details] [diff] [review] Test [Approval Request Comment] Test for above patch.

(In reply to neil@parkwaycc.co.uk from comment #37) > Risk to taking this patch (and alternatives if risky): Back out 752210 Hi Neil/Ehsan - taking a look at the patch, this doesn't appear to be particularly risky. I'd love to learn a bit more about the risk profile, however. Thanks in advance!

(In reply to Alex Keybl [:akeybl] from comment #39) > Hi Neil/Ehsan - taking a look at the patch, this doesn't appear to be > particularly risky. I'd love to learn a bit more about the risk profile, > however. Thanks in advance! Bug 752210 cleaned up some old, messy code. In the process, it changed the way a few loops work, basically from for (PRInt i = length - 1; i >= 0; i--) { // do stuff to array[i] } to for (nsIContent* child = array[length - 1]; child; child = child->GetPreviousSibling()) { // do stuff to child } which broke, because child was moved around in some cases and its previous sibling changed. This patch just reverts those specific loops back to way they used to be -- i.e., it reverts part of one of bug 752210's patches. So the risk should be low, because it's only trying to restore the code to the way it was. Backing out bug 752210 instead of taking this patch shouldn't be a problem either, though, if it backs out cleanly. That bug was just cleanup, so it will have no user-visible effect to back it out. That way we know all the code is being restored to exactly what it used to be, so regression potential would be even lower.

Yeah, I think I'd rather backout here, since the original patch didn't have much value behind code cleanup (even though the fix patch is also quite safe...)

(In reply to Ehsan Akhgari [:ehsan] from comment #41) > Yeah, I think I'd rather backout here, since the original patch didn't have > much value behind code cleanup (even though the fix patch is also quite > safe...) Given that, minusing the current patches for Aurora.

Comment on attachment 639292 [details] [diff] [review] Test Actually, it may be nice to take this test along with the backout of bug 752210. Let me know what you all think.

(In reply to comment #43) > Comment on attachment 639292 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=639292 > Test > > Actually, it may be nice to take this test along with the backout of bug > 752210. Let me know what you all think. Makes sense to me.

Hmm, Neil, this patch doesn't seem to apply cleanly to Aurora...

Pushed mozilla-aurora changeset cc86af7f1471. (In reply to Ehsan Akhgari from comment #46) > Hmm, Neil, this patch doesn't seem to apply cleanly to Aurora... That's because it's already there, I just hadn't updated the bug!

Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0 (20120724191344) Verified with the test case in comment 11. The whole test is enlarged.

I am not sure if the patches for this bug apply currently to 16a2 but for yesterday's 16a2 I still have a problem with the following test: 1) Compose a new mail and type several lines of text into the main body of the mail. 2) Highlight all of the new text in the compose window. 3) Select Format->Size and increase the size to extra large. 4) Now (realising you want to add some more text) click in the middle of the text area and type in new text, and edit some of the text already there. 5) Nothing seems wrong at this stage but if the mail is sent then sometimes it seems to be fine in the copy that is actually sent, as confirmed by what appears in the copy in the Sent folder, but sometimes the font size is broken into sections of different sizes despite it all looking uniform at the point the Send button is about to be clicked. This seems to have been the case for some time.

Sorry, that is entirely unrelated; this bug is only about the increase/decrease size buttons.

OK I will check the increase/decrease buttons and report the issue from #49 elsewhere.

Actually I just tested the increase size button after replying - putting in a few lines of text into the compose window - then highlighting all of the newly entered text, and hitting the "increase font size" icon- TB seemed to freeze but after about 10 seconds it unfroze with the weirdest set of font sizes I have ever seen in the compose window! The first few words had increased as expected but then every few sets of words that followed were increased again repeatedly so that the last few characters were so large that each letter overfilled the entire compose window! However hitting the "decrease font size" icon reverted to the original text size which was uniform across the paragraph - So I think there are a few residual problems with the current patches - at least for version Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/16.0 Thunderbird/16.0a2 ID:20120810042011

Please file a new bug with specific instructions as to how to reproduce the problem. You can mark it as blocking this bug, but it needs to be separate so it can be tracked separately. Thanks.

OK I have been testing with Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20120813 Thunderbird/16.0a2 ID:20120813042011 but not had a recurrence with this new version - if I see a recurrence I will post a new bug report for the later version with full details.