Last Comment Bug 767684 - Font size changes inconsistently in the compose window when using the increase/decrease buttons
: Font size changes inconsistently in the compose window when using the increas...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 15 Branch
: x86_64 Linux
: -- major (vote)
: mozilla16
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 769605 769604
Blocks: 752210
  Show dependency treegraph
 
Reported: 2012-06-23 02:24 PDT by Mike Cloaked
Modified: 2012-08-14 10:30 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Image showing the font change problem in compose (82.11 KB, image/jpeg)
2012-06-23 02:46 PDT, Mike Cloaked
no flags Details
Proposed patch (2.25 KB, patch)
2012-07-01 08:44 PDT, neil@parkwaycc.co.uk
ehsan: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Review
Test (1.73 KB, patch)
2012-07-05 04:49 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
Branch backout plus test (12.50 KB, patch)
2012-07-10 01:53 PDT, neil@parkwaycc.co.uk
ehsan: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mike Cloaked 2012-06-23 02:24:39 PDT
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.
Comment 1 Mike Cloaked 2012-06-23 02:26:02 PDT
The specific build tested is Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120622 Thunderbird/15.0a2 ID:20120622042001
Comment 2 Mike Cloaked 2012-06-23 02:46:49 PDT
Created attachment 636054 [details]
Image showing the font change problem in compose

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.
Comment 3 Mike Cloaked 2012-06-23 02:50:44 PDT
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.
Comment 4 Mike Cloaked 2012-06-23 02:58:35 PDT
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.
Comment 5 Mike Cloaked 2012-06-23 13:03:14 PDT
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.
Comment 6 Mark Banner (:standard8) 2012-06-25 01:39:55 PDT
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.
Comment 7 Mark Banner (:standard8) 2012-06-25 01:51:30 PDT
Given the window of when this occured, I suspect it is a regression from bug 590640.
Comment 8 Mike Cloaked 2012-06-25 01:54:50 PDT
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?
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-25 07:58:43 PDT
It would be great if we can see the email you were replying to.  That will help us reproduce and fix this problem.
Comment 10 Mike Cloaked 2012-06-25 09:22:50 PDT
Ehsan - I have emailed you privately with a forwarded email and some additional information on the problem.
Comment 11 :Aryeh Gregor (away until August 15) 2012-06-27 06:16:27 PDT
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.
Comment 12 Mike Cloaked 2012-06-27 06:55:17 PDT
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.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-27 09:36:51 PDT
If the Thunderbird developers agree to stop using these commands, I have no problem with ripping them out of editor!
Comment 14 Mike Cloaked 2012-06-27 09:49:23 PDT
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?
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-27 09:58:50 PDT
(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.
Comment 16 Massimo Fidanza 2012-06-27 10:45:21 PDT
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
Comment 17 :Aryeh Gregor (away until August 15) 2012-06-29 03:58:01 PDT
(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.  :)
Comment 18 Mike Cloaked 2012-06-29 11:32:08 PDT
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?
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-29 12:39:27 PDT
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.
Comment 20 Mike Cloaked 2012-06-29 12:43:53 PDT
OK Ehsan thanks for clearing up that point.
Comment 21 Joe Sabash [:JoeS1] 2012-06-30 11:59:37 PDT
(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.
Comment 22 :Aryeh Gregor (away until August 15) 2012-07-01 02:16:10 PDT
(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.
Comment 23 neil@parkwaycc.co.uk 2012-07-01 08:29:04 PDT
Regression from bug 752210.
Comment 24 neil@parkwaycc.co.uk 2012-07-01 08:44:04 PDT
Created attachment 638192 [details] [diff] [review]
Proposed patch
Comment 25 :Aryeh Gregor (away until August 15) 2012-07-01 08:53:11 PDT
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.)
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-01 10:49:30 PDT
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.
Comment 27 :Aryeh Gregor (away until August 15) 2012-07-01 12:13:13 PDT
(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.
Comment 28 neil@parkwaycc.co.uk 2012-07-01 12:52:08 PDT
(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.)
Comment 29 :Aryeh Gregor (away until August 15) 2012-07-02 01:26:54 PDT
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 30 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-03 11:11:09 PDT
Comment on attachment 638192 [details] [diff] [review]
Proposed patch

OK, this makes sense.  But please add a test here too!
Comment 31 :Aryeh Gregor (away until August 15) 2012-07-05 04:49:08 PDT
Created attachment 639292 [details] [diff] [review]
Test

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 32 neil@parkwaycc.co.uk 2012-07-05 05:41:08 PDT
Comment on attachment 639292 [details] [diff] [review]
Test

I'd say that this is a perfectly cromulent test :-)
Comment 33 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-05 10:19:43 PDT
Comment on attachment 639292 [details] [diff] [review]
Test

Review of attachment 639292 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Comment 34 neil@parkwaycc.co.uk 2012-07-05 13:36:40 PDT
Comment on attachment 638192 [details] [diff] [review]
Proposed patch

Pushed mozilla-inbound changeset aa7493c796ce.
Comment 35 neil@parkwaycc.co.uk 2012-07-05 13:37:19 PDT
Comment on attachment 639292 [details] [diff] [review]
Test

Pushed mozilla-inbound changeset 7851dc0f395c.
Comment 37 neil@parkwaycc.co.uk 2012-07-06 00:29:53 PDT
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 38 neil@parkwaycc.co.uk 2012-07-06 00:30:21 PDT
Comment on attachment 639292 [details] [diff] [review]
Test

[Approval Request Comment]
Test for above patch.
Comment 39 Alex Keybl [:akeybl] 2012-07-08 18:08:11 PDT
(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!
Comment 40 :Aryeh Gregor (away until August 15) 2012-07-09 01:31:57 PDT
(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.
Comment 41 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-09 14:21:50 PDT
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...)
Comment 42 Alex Keybl [:akeybl] 2012-07-09 18:35:48 PDT
(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 43 Alex Keybl [:akeybl] 2012-07-09 18:36:37 PDT
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.
Comment 44 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-09 18:38:04 PDT
(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.
Comment 45 neil@parkwaycc.co.uk 2012-07-10 01:53:58 PDT
Created attachment 640536 [details] [diff] [review]
Branch backout plus test
Comment 46 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-13 07:14:15 PDT
Hmm, Neil, this patch doesn't seem to apply cleanly to Aurora...
Comment 47 neil@parkwaycc.co.uk 2012-07-13 08:48:05 PDT
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!
Comment 48 Ioana (away) 2012-07-27 10:04:43 PDT
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.
Comment 49 Mike Cloaked 2012-08-11 02:01:26 PDT
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.
Comment 50 neil@parkwaycc.co.uk 2012-08-11 02:59:57 PDT
Sorry, that is entirely unrelated; this bug is only about the increase/decrease size buttons.
Comment 51 Mike Cloaked 2012-08-11 09:16:07 PDT
OK I will check the increase/decrease buttons and report the issue from #49 elsewhere.
Comment 52 Mike Cloaked 2012-08-11 09:37:49 PDT
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
Comment 53 :Aryeh Gregor (away until August 15) 2012-08-14 06:15:26 PDT
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.
Comment 54 Mike Cloaked 2012-08-14 10:30:56 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.