Font size changes inconsistently in the compose window when using the increase/decrease buttons

RESOLVED FIXED in Firefox 15

Status

()

Core
Editor
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mike Cloaked, Assigned: neil@parkwaycc.co.uk)

Tracking

(Depends on: 1 bug, {regression})

15 Branch
mozilla16
x86_64
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Updated

5 years ago
Severity: normal → major
(Reporter)

Comment 5

5 years ago
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.
Blocks: 590640
Component: Message Compose Window → Editor
Keywords: qawanted, regression, testcase-wanted
Product: Thunderbird → Core
QA Contact: message-compose → editor
Summary: Compose font changes inconsistently in version 15a2 → Font size changes inconsistently in the compose window when using the increase/decrease buttons
Version: 15 → 15 Branch
(Reporter)

Comment 8

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

Comment 10

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

Comment 12

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

Comment 14

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

Comment 16

5 years ago
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
Depends on: 769604
Depends on: 769605
(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.  :)
(Reporter)

Comment 18

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

Comment 20

5 years ago
OK Ehsan thanks for clearing up that point.

Comment 21

5 years ago
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 23

5 years ago
Regression from bug 752210.
Blocks: 752210
No longer blocks: 590640
(Assignee)

Comment 24

5 years ago
Created attachment 638192 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #638192 - Flags: review?(ehsan)
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.
(Assignee)

Comment 28

5 years ago
(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!
Attachment #638192 - Flags: review?(ehsan) → review+
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
Attachment #639292 - Flags: review?(ehsan)
(Assignee)

Comment 32

5 years ago
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!
Attachment #639292 - Flags: review?(ehsan) → review+
(Assignee)

Comment 34

5 years ago
Comment on attachment 638192 [details] [diff] [review]
Proposed patch

Pushed mozilla-inbound changeset aa7493c796ce.
(Assignee)

Comment 35

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Assignee)

Comment 37

5 years ago
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
Attachment #638192 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 38

5 years ago
Comment on attachment 639292 [details] [diff] [review]
Test

[Approval Request Comment]
Test for above patch.
Attachment #639292 - Flags: approval-mozilla-aurora?
status-firefox15: --- → affected
tracking-firefox15: --- → +
Keywords: qawanted, testcase-wanted
(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.

Updated

5 years ago
Attachment #638192 - Flags: approval-mozilla-aurora? → approval-mozilla-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.
(Assignee)

Comment 45

5 years ago
Created attachment 640536 [details] [diff] [review]
Branch backout plus test
Attachment #640536 - Flags: review?(ehsan)
Attachment #640536 - Flags: review?(ehsan)
Attachment #640536 - Flags: review+
Attachment #640536 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #639292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #640536 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hmm, Neil, this patch doesn't seem to apply cleanly to Aurora...
(Assignee)

Comment 47

5 years ago
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!
status-firefox15: affected → fixed

Comment 48

5 years ago
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.
status-firefox15: fixed → verified
(Reporter)

Comment 49

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

Comment 50

5 years ago
Sorry, that is entirely unrelated; this bug is only about the increase/decrease size buttons.
(Reporter)

Comment 51

5 years ago
OK I will check the increase/decrease buttons and report the issue from #49 elsewhere.
(Reporter)

Comment 52

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

Comment 54

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.