Last Comment Bug 824926 - Relative font size doesn't handle nested font size tags correctly
: Relative font size doesn't handle nested font size tags correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla20
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 829844 (view as bug list)
Depends on:
Blocks: 746515 801400 802997 833689
  Show dependency treegraph
 
Reported: 2012-12-27 03:41 PST by neil@parkwaycc.co.uk
Modified: 2013-03-04 16:11 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
20+
fixed


Attachments
Proposed patch (1.00 KB, patch)
2012-12-27 03:55 PST, neil@parkwaycc.co.uk
ehsan: review+
akeybl: approval‑mozilla‑aurora-
akeybl: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2012-12-27 03:41:09 PST
Steps to reproduce problem:
1. Type lots of text
2. Select all the text and change its font size
3. Select a word in the text and change its font size again
4. Select all the text and change its relative font size

Expected result: <body><font size="+1"><small>lots <font size="+2"><small>of</small></font> text</small></font></body>

Actual result: <body><font size="+1"><small>lots <font size="+2"><small><small>of</small></small></font> text</small></font></body>

Bug 824924 makes this issue more obvious, but is otherwise unrelated.
Comment 1 neil@parkwaycc.co.uk 2012-12-27 03:55:54 PST
Created attachment 695992 [details] [diff] [review]
Proposed patch

In pseudocode, the relative font change code looks like this:
function RelativeFontChangeOnNode(aTag, aNode)
{
  if (TagCanContain(aTag, aNode)) {
    RecursivelyFindFontSizeNodes(aTag, aNode);
    WrapTagAroundNode(aTag, aNode);
  } else {
    aNode.childNodes.forEach(RelativeFontChangeOnNode.bind(aTag));
  }
}
Which is straightforward enough. The trouble comes when we find a font size tag:
function RecursivelyFindFontSizeNodes(aTag, aNode)
{
  if (IsFontSizeNode(aNode)) {
    RelativeFontChangeOnNode(aTag, aNode)
  }
  aNode.childNodes.forEach(RecursivelyFindFontSizeNodes.bind(aTag));
}
The problem here is that RelativeFontChangeOnNode itself ends up calling RecursivelyFindFontSizeNodes, so that nested font size nodes are found twice, once by the original change and once by the relative font change on the outer font size node.

The quick fix is to early return after changing the relative font on the font size node, since any nested font size nodes will have already been processed.
Comment 2 :Ehsan Akhgari 2012-12-29 11:18:32 PST
Do all of the editor tests pass with this patch?
Comment 3 neil@parkwaycc.co.uk 2012-12-29 14:47:57 PST
Of course they do ;-)
Comment 4 :Ehsan Akhgari 2012-12-31 11:31:23 PST
Comment on attachment 695992 [details] [diff] [review]
Proposed patch

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +1783,5 @@
>        nsresult rv = RelativeFontChangeOnNode(aSizeChange, aNode->GetChildAt(i));
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
> +
> +    return NS_OK;

Please add a short comment on why we're returning early, as you describe in the bug.
Comment 5 neil@parkwaycc.co.uk 2012-12-31 13:59:02 PST
Pushed comm-central changeset dec6aa71da64.
Comment 6 neil@parkwaycc.co.uk 2012-12-31 14:29:43 PST
Comment on attachment 695992 [details] [diff] [review]
Proposed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression as such but bug 824924 (caused by bug 590640) significantly increased this bug's visibility.
User impact if declined: Difficult to increase font size in Message Compose.
Testing completed (on m-c, etc.): Ran all editor tests locally.
Risk to taking this patch (and alternatives if risky): Low, in Firefox it only affects non-standard usage of execCommand; mitigating alternatives are to fix bug 824924 (hard) or back out bug 590640 (very hard).
String or UUID changes made by this patch: None.
Comment 7 Alex Keybl [:akeybl] 2013-01-03 14:59:54 PST
Given this has been in the build for a couple of releases with little impact to Firefox users, we'd only uplift to Aurora in preparation for uplift to ESR17 (to support the TB ESR). Is that your plan?
Comment 8 Justin Wood (:Callek) 2013-01-03 15:35:32 PST
(In reply to Alex Keybl [:akeybl] from comment #7)
> Given this has been in the build for a couple of releases with little impact
> to Firefox users, we'd only uplift to Aurora in preparation for uplift to
> ESR17 (to support the TB ESR). Is that your plan?

It also does have a relatively large impact on SeaMonkey/Composer which is still riding the trains as normal. Which is stated to help inform the choice of approval.
Comment 9 Alex Keybl [:akeybl] 2013-01-04 15:26:30 PST
Comment on attachment 695992 [details] [diff] [review]
Proposed patch

The affected population is not large, nor the impact critical, so I'm hesitant to make any editor changes that could raise new edge case regressions. Let's let this ride the trains.
Comment 10 Joe Sabash [:JoeS1] 2013-01-05 17:03:07 PST
(In reply to Alex Keybl [:akeybl] from comment #9)
> Comment on attachment 695992 [details] [diff] [review]
> Proposed patch
> 
> The affected population is not large, nor the impact critical, so I'm
> hesitant to make any editor changes that could raise new edge case
> regressions. Let's let this ride the trains.

FWIW That train for Thunderbird is a very slow freight train. The next projected release for TB will be TB24. That's a very long time to wait for the average user.
There is also the appearance of stagnation of TB release to consider, and the ESR.
Comment 11 Joe Sabash [:JoeS1] 2013-01-12 14:31:14 PST
*** Bug 829844 has been marked as a duplicate of this bug. ***
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2013-01-22 22:31:01 PST
(In reply to Alex Keybl [:akeybl] from comment #9)
> Comment on attachment 695992 [details] [diff] [review]
> Proposed patch
> 
> The affected population is not large, nor the impact critical, so I'm
> hesitant to make any editor changes that could raise new edge case
> regressions. Let's let this ride the trains.

This remains to be tested by a crashing user, but I believe this patch may fix the #1 cause of Thunderbird crashes (considering several different crash signatures) ... so impact is far from small and non-critical. (more to come)


question to akeybl, Neil or whoever - is Neil's estimation of low risk to Firefox off the mark?
Comment 13 Mark Banner (:standard8) 2013-01-23 00:42:17 PST
(In reply to Alex Keybl [:akeybl] from comment #7)
> Given this has been in the build for a couple of releases with little impact
> to Firefox users, we'd only uplift to Aurora in preparation for uplift to
> ESR17 (to support the TB ESR). Is that your plan?

We would like to get this into ESR 17 for mainstream Thunderbird users as well as those ESR ones. I think waiting until gecko 20 is released, that's fine, but I'd like to get it into ESR 17 at the same time as that release.
Comment 14 :Ehsan Akhgari 2013-01-23 15:22:49 PST
I think this patch is fairly safe for Aurora.
Comment 15 :Ehsan Akhgari 2013-01-23 15:23:04 PST
gah! make that ESR.
Comment 16 Wayne Mery (:wsmwk, NI for questions) 2013-02-23 09:45:44 PST
(In reply to :Ehsan Akhgari from comment #15)
> gah! make that ESR.

we seem to have missed this for ESR. :(
Neil, can  you and it on ESR?
Comment 17 Joe Sabash [:JoeS1] 2013-02-23 10:36:04 PST
(In reply to Wayne Mery (:wsmwk) from comment #16)
> (In reply to :Ehsan Akhgari from comment #15)
> > gah! make that ESR.
> 
> we seem to have missed this for ESR. :(
> Neil, can  you and it on ESR?

Wayne, I pulled a tinderbox zip build Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130222 Thunderbird/17.0.3 and the fix seems to be there.
Comment 18 neil@parkwaycc.co.uk 2013-02-23 11:09:54 PST
(In reply to Wayne Mery from comment #16)
> Neil, can you land it on ESR?
I don't have an ESR tree, nor do I intend to pull one.
Comment 19 rsx11m 2013-02-23 13:25:32 PST
https://hg.mozilla.org/releases/mozilla-esr17/log?rev=824926 produces no match;
https://hg.mozilla.org/releases/mozilla-aurora/log?rev=824926 does.

https://hg.mozilla.org/releases/mozilla-esr17/file/3e527a204136/editor/libeditor/html/nsHTMLEditorStyle.cpp (current tip) doesn't show the patch applied.

Thus, no evidence that it has been checked in for the 17.0 ESR branch.
Comment 20 rsx11m 2013-02-23 13:28:06 PST
(comment #19 was a reply to comment #17...)
Comment 21 Joe Sabash [:JoeS1] 2013-02-23 16:25:21 PST
(In reply to rsx11m from comment #20)
> (comment #19 was a reply to comment #17...)

Yeah, I forgot that to reproduce you must have <font size="x"> in the mix.
So the bug remains in ESR
Comment 22 :Ehsan Akhgari 2013-02-24 16:48:00 PST
(In reply to comment #18)
> (In reply to Wayne Mery from comment #16)
> > Neil, can you land it on ESR?
> I don't have an ESR tree, nor do I intend to pull one.

If you request approval for the ESR branch, I will happily land it for you when the patch is approved.
Comment 23 rsx11m 2013-02-24 18:26:55 PST
Comment on attachment 695992 [details] [diff] [review]
Proposed patch

> Hunk #1 succeeded at 1773 (offset -5 lines).

Patch applies against mozilla-esr17, I didn't test it though (I don't have an 17.0 ESR build either). Thus, requesting approval on Wayne's behalf.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

User impact if declined: Current Thunderbird users will remain affected by this issue until 24.0 ESR is released, given that TB mainstream releases are now built from comm-esr17/mozilla-esr17 as well

Fix Landed on Version: 20.0

Risk to taking this patch (and alternatives if risky): I cannot assess this, but it's a simple change and it seems to work fine on trunk.

String or UUID changes made by this patch: none
Comment 24 rsx11m 2013-02-24 18:29:59 PST
(oops, you replied to Neil and not to Wayne in comment #22 - well, anyway, let's keep the approval request up, I assume that Neil will amend it as necessary.)
Comment 25 Alex Keybl [:akeybl] 2013-03-04 10:47:39 PST
(In reply to rsx11m from comment #23)
> Risk to taking this patch (and alternatives if risky): I cannot assess this,
> but it's a simple change and it seems to work fine on trunk.

Neil - does your comment 6 apply to ESR17 as well?
Comment 26 neil@parkwaycc.co.uk 2013-03-04 11:49:30 PST
(In reply to Alex Keybl from comment #25)
> (In reply to rsx11m from comment #23)
> > Risk to taking this patch (and alternatives if risky): I cannot assess this,
> > but it's a simple change and it seems to work fine on trunk.
> 
> Neil - does your comment 6 apply to ESR17 as well?

Yes, in as much as the assessment of the risk is based on the size of the patch and the areas of the code which are affected.

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