Closed Bug 824926 Opened 12 years ago Closed 12 years ago

Relative font size doesn't handle nested font size tags correctly

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox-esr17 20+ fixed

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file)

Proposed patch
1.00 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-aurora-
akeybl
: approval-mozilla-esr17+
Details | Diff | Splinter Review 
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.
Attached patch Proposed patch — — Splinter Review 
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #695992 - Flags: review?(ehsan)
Do all of the editor tests pass with this patch?
Of course they do ;-)
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.
Attachment #695992 - Flags: review?(ehsan) → review+
Pushed comm-central changeset dec6aa71da64.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #695992 - Flags: approval-mozilla-aurora?
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?
(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 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.
Attachment #695992 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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.
Blocks: 833689
(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?
Blocks: 802997, 801400
Severity: normal → major
(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.
status-firefox-esr17: --- → affected
tracking-firefox-esr17: --- → ?
Target Milestone: --- → mozilla20
I think this patch is fairly safe for Aurora.
gah! make that ESR.
Blocks: 746515
(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?
Flags: needinfo?(neil)
(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.
Severity: major → normal
Target Milestone: mozilla20 → ---
(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.
Flags: needinfo?(neil)
Severity: normal → major
Target Milestone: --- → mozilla20
(comment #19 was a reply to comment #17...)
(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
(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 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
Attachment #695992 - Flags: approval-mozilla-esr17?
(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.)
(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?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
Attachment #695992 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: