Last Comment Bug 480647 - execCommand('fontSize') doesn't change font size if content font size is specified in css
: execCommand('fontSize') doesn't change font size if content font size is spec...
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 747879 747889 font_size_bloat
Blocks: richtext2
  Show dependency treegraph
 
Reported: 2009-02-27 15:01 PST by Marcos Almeida
Modified: 2013-07-17 14:32 PDT (History)
6 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Clean up nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet (8.09 KB, patch)
2012-04-16 01:00 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Move ParseLegacyFontSize to nsContentUtils so editor/ can use it (5.10 KB, patch)
2012-04-23 07:44 PDT, :Aryeh Gregor (working until September 2)
bzbarsky: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up nsHTMLCSSUtils::GenerateCSSDeclarationsFromHTMLStyle (7.38 KB, patch)
2012-04-23 07:45 PDT, :Aryeh Gregor (working until September 2)
ehsan: review-
Details | Diff | Splinter Review
Patch part 4, v1 -- Clean up nsHTMLDocument::ConvertToMidasInternalCommandInner (6.20 KB, patch)
2012-04-23 07:46 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Reject invalid values from execCommand("fontSize") (10.64 KB, patch)
2012-04-23 07:48 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 6, v1 -- Handle CSS in execCommand("fontSize") (26.18 KB, patch)
2012-04-23 07:53 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description Marcos Almeida 2009-02-27 15:01:26 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.6) Gecko/2009011912 Firefox/3.0.6

If a span has a style attribute that sets the font size, using execCommand to change that text's size will not work, because the span will simply be wrapped by a font tag, but the style in the nested span will override the inserted font tag's size.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.mozilla.org/editor/midasdemo/
2. Check the "View HTML Source checkbox at the bottom.
3. Enter the following HTML: <span style="font-size:small">Hello World</span>
4. Uncheck the "View HTML Source checkbox at the bottom.
5. Select the "Hello World" text.
6. Choose "7" from the Size dropdown.
--> Nothing happens (the font size should have increased).
7. Check the "View HTML Source checkbox at the bottom.
--> The resulting HTML is: <font size="7"><span style="font-size:small">Hello World</span></font>
Actual Results:  
Nothing visibly happens to the html. When viewing the source you see that the original source was wrapped with a <font size="7"> tag.

Expected Results:  
Font size should have increased to huge. When viewing the source you would see that font-size:small would be replaced by font-size:xx-large.

This affects Google properties.

https://bugzilla.mozilla.org/show_bug.cgi?id=394302 and https://bugzilla.mozilla.org/show_bug.cgi?id=394303 seem to be related bugs.

A fix where the above expected result happens only when 'useCSS' is active would be sufficient.
Comment 1 Dan POPA 2009-09-03 23:18:21 PDT
I can confirm this, still happening in FF 3.5.2

Simple reproduction steps:
 - have some text in midasdemo
 - select, apply font size when usecss is set, check source:
Actual results: <font size="5">text</font>
Expected results: <span style="font-size: 24px | 18pt;">text</font>
Comment 2 Dan POPA 2009-09-03 23:46:59 PDT
Correction/Alternative to comment 1:
Use:
xx-small	for fontSize 1
x-small	for fontSize 2
small	for fontSize 3
medium	for fontSize 4
large	for fontSize 5
x-large	for fontSize 6
xx-large	for fontSize 7

Actual results: <font size="5">text</font>
Expected results: <span style="font-size: large;">text</font>
Comment 3 :Aryeh Gregor (working until September 2) 2012-04-04 02:48:36 PDT
Confirmed in mozilla-central.  Standalone test-case:

data:text/html,<!doctype html>
<div contenteditable><span style=font-size:small>foo</span></div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("fontSize", false, "7");
document.body.textContent =
document.body.firstChild.innerHTML;
</script>

A mozilla-central build outputs

  <font size="7"><span style="font-size:small">foo</span></font>

Expected is

  <font size="7">foo</font>

The fontSize code needs to clear the CSS font-size property, not just <font> tags.
Comment 4 :Aryeh Gregor (working until September 2) 2012-04-15 02:21:43 PDT
(This is tested already by richtext and richtext2.)
Comment 5 :Aryeh Gregor (working until September 2) 2012-04-16 01:00:12 PDT
Created attachment 615268 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet

This does nothing to fix the bug, which is proving harder than I expected, but it makes me want to stab my eyes out a bit less, which is good.
Comment 6 Mozilla RelEng Bot 2012-04-16 01:03:25 PDT
Autoland Patchset:
	Patches: 615268
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=3acdb28510f4
Try run started, revision 3acdb28510f4. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=3acdb28510f4
Comment 7 Mozilla RelEng Bot 2012-04-16 05:30:26 PDT
Try run for 3acdb28510f4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3acdb28510f4
Results (out of 222 total builds):
    exception: 1
    success: 186
    warnings: 35
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-3acdb28510f4
Comment 8 :Ehsan Akhgari 2012-04-16 12:12:35 PDT
Comment on attachment 615268 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet

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

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +1115,3 @@
>               ( (nsEditProperty::font == aHTMLProperty && 
>                  aHTMLAttribute->EqualsLiteral("color")) ||
>                 aHTMLAttribute->EqualsLiteral("bgcolor"))) {

Nit: fix the indentation here, please.

@@ +1153,1 @@
>               && aHTMLAttribute->EqualsLiteral("face")) {

Ditto.

@@ +1171,1 @@
>               && aHTMLAttribute->EqualsLiteral("align")) {

Ditto.
Comment 9 :Aryeh Gregor (working until September 2) 2012-04-17 03:24:31 PDT
What indentation would you like to see for this sort of complicated conditional?
Comment 10 :Ehsan Akhgari 2012-04-17 10:39:38 PDT
(In reply to Aryeh Gregor from comment #9)
> What indentation would you like to see for this sort of complicated
> conditional?

Something like:

  if (((foo->bar() && baz->foo()) ||
       (condition1 && condition2)) &&
      (myFunc()->value > 10)) {
    // ...
Comment 11 :Aryeh Gregor (working until September 2) 2012-04-18 07:27:18 PDT
http://hg.mozilla.org/projects/birch/rev/90030a1e28e0
Comment 12 :Aryeh Gregor (working until September 2) 2012-04-23 07:44:59 PDT
Created attachment 617469 [details] [diff] [review]
Patch part 2, v1 -- Move ParseLegacyFontSize to nsContentUtils so editor/ can use it

I had an ulterior motive for bug 745685.  :)
Comment 13 :Aryeh Gregor (working until September 2) 2012-04-23 07:45:30 PDT
Created attachment 617470 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsHTMLCSSUtils::GenerateCSSDeclarationsFromHTMLStyle
Comment 14 :Aryeh Gregor (working until September 2) 2012-04-23 07:46:11 PDT
Created attachment 617471 [details] [diff] [review]
Patch part 4, v1 -- Clean up nsHTMLDocument::ConvertToMidasInternalCommandInner
Comment 15 :Aryeh Gregor (working until September 2) 2012-04-23 07:48:48 PDT
Created attachment 617474 [details] [diff] [review]
Patch part 5, v1 -- Reject invalid values from execCommand("fontSize")

Previously we would just pass through any garbage value in fontSize, so document.execCommand("fontsize", false, "potatoes") would produce <font size="potatoes">.  With the final patch in this series applied, this would cause empty <span>s to be output, because the conversion to a CSS value would fail.  This is a desirable improvement to correctness anyway; it more closely matches the spec.  I filed bug 747879 as a followup to make it match the spec better.

The richtext2 regressions here are filed as <http://code.google.com/p/browserscope/issues/detail?id=326>.
Comment 16 :Aryeh Gregor (working until September 2) 2012-04-23 07:53:17 PDT
Created attachment 617477 [details] [diff] [review]
Patch part 6, v1 -- Handle CSS in execCommand("fontSize")

This fixes the actual bug.  At the same time, it supports output of CSS for fontSize in CSS mode, instead of always using <font size>.  It still always uses <font size=7>, per spec, because there's no CSS equivalent.  queryCommandValue() still ignores CSS values entirely; I filed bug 747889 as a followup.  I had to make mildly invasive changes to account for the concept of an attribute that only supported CSS for certain values, but it seems to work.

This code is really horrible and I want to rewrite it.  :(

I'll submit to try after m-c reopens and birch is merged, because I have all these patches on top of birch and they'll almost surely conflict if I try submitting them to try as-is.  Tests in editor/ all pass locally, though.
Comment 17 :Ehsan Akhgari 2012-04-23 07:56:40 PDT
Comment on attachment 617470 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsHTMLCSSUtils::GenerateCSSDeclarationsFromHTMLStyle

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

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +852,5 @@
>                                                       bool aGetOrRemoveRequest)
>  {
>    MOZ_ASSERT(aElement);
>    nsIAtom* tagName = aElement->Tag();
> +  const nsHTMLCSSUtils::CSSEquivTable* equivTable;

Hmm, this is not initialized, and if I'm reading the code below right, it is possible for this variable to never be initialized, so the if (equivTable) check below would almost always pass (unless you happen to have all 0 bits at the right place on your stack.)

You should init this to null.

@@ +863,5 @@
> +    equivTable = underlineEquivTable;
> +  } else if (nsEditProperty::strike == aHTMLProperty) {
> +    equivTable = strikeEquivTable;
> +  } else if (nsEditProperty::tt == aHTMLProperty) {
> +    equivTable = ttEquivTable;

This makes my head spin.  Can you please change this to use a switch statement?

@@ +909,1 @@
>      }

Hmm, I wish aAttribute was an atom, but doing that requires way more pervasive changes. :(
Comment 18 :Ehsan Akhgari 2012-04-23 08:04:04 PDT
Comment on attachment 617474 [details] [diff] [review]
Patch part 5, v1 -- Reject invalid values from execCommand("fontSize")

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2973,5 @@
> +  } else if (outCommandID.EqualsLiteral("cmd_fontSize")) {
> +    // Per editing spec as of April 23, 2012, we need to reject the value if
> +    // it's not a valid floating-point number surrounded by optional whitespace.
> +    // Otherwise, we parse it as a legacy font size.  For now, we just parse as
> +    // a legacy font size regardless (matching Chrome) -- bug 747879.

Nit: s/Chrome/WebKit/  :-)

::: editor/libeditor/html/tests/test_bug480647.html
@@ +66,5 @@
> +parseFontSize("7", 7);
> +parseFontSize("8", 7);
> +parseFontSize("9", 7);
> +parseFontSize("10", 7);
> +parseFontSize("1000000000000000000000", 7);

I resisted the urge to ask why you're using this number.  ;-)

@@ +100,5 @@
> +parseFontSize("+9", 7);
> +parseFontSize("+10", 7);
> +parseFontSize("+1000000000000000000000", 7);
> +parseFontSize("+1.72", 4);
> +parseFontSize("+1.72e9", 4);

It may also be useful to test strings with leading and trailing whitespaces.
Comment 19 :Ehsan Akhgari 2012-04-23 08:15:41 PDT
Comment on attachment 617477 [details] [diff] [review]
Patch part 6, v1 -- Handle CSS in execCommand("fontSize")

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

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +226,5 @@
> +        aOutputString.AssignLiteral("xx-large");
> +        return;
> +      case 7:
> +        // No corresponding CSS size
> +        return;

Please add a default case which uses NS_NOTREACHED or something similar.
Comment 20 :Aryeh Gregor (working until September 2) 2012-04-23 10:38:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Hmm, this is not initialized, and if I'm reading the code below right, it is
> possible for this variable to never be initialized, so the if (equivTable)
> check below would almost always pass (unless you happen to have all 0 bits
> at the right place on your stack.)
>
> You should init this to null.

The compiler doesn't automatically initialize pointers to null, and doesn't give an error if they're checked before initialization?  That seems kind of evil . . . but I guess C++ is evil.

> This makes my head spin.  Can you please change this to use a switch
> statement?

That doesn't work, since the things I'm comparing to aren't constant.  Can I take this as an r+ with just the pointer initialization change?

(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> I resisted the urge to ask why you're using this number.  ;-)

To test that we treat numbers > MAX_INT properly.  (Which we do, because my ParseLegacyFontSize() implementation bails out as soon as the number it reads in is > 7.)
Comment 21 :Ehsan Akhgari 2012-04-23 11:23:32 PDT
(In reply to Aryeh Gregor from comment #20)
> (In reply to Ehsan Akhgari [:ehsan] from comment #17)
> > Hmm, this is not initialized, and if I'm reading the code below right, it is
> > possible for this variable to never be initialized, so the if (equivTable)
> > check below would almost always pass (unless you happen to have all 0 bits
> > at the right place on your stack.)
> >
> > You should init this to null.
> 
> The compiler doesn't automatically initialize pointers to null,

Not for stack variables.

> and doesn't
> give an error if they're checked before initialization?  That seems kind of
> evil . . . but I guess C++ is evil.

It should give you a warning.

> > This makes my head spin.  Can you please change this to use a switch
> > statement?
> 
> That doesn't work, since the things I'm comparing to aren't constant.  Can I
> take this as an r+ with just the pointer initialization change?

Yes.

> (In reply to Ehsan Akhgari [:ehsan] from comment #18)
> > I resisted the urge to ask why you're using this number.  ;-)
> 
> To test that we treat numbers > MAX_INT properly.  (Which we do, because my
> ParseLegacyFontSize() implementation bails out as soon as the number it
> reads in is > 7.)

That number is greater than 7 indeed!  :-)
Comment 22 Boris Zbarsky [:bz] 2012-04-23 20:16:36 PDT
Comment on attachment 617469 [details] [diff] [review]
Patch part 2, v1 -- Move ParseLegacyFontSize to nsContentUtils so editor/ can use it

r=me
Comment 23 :Aryeh Gregor (working until September 2) 2012-04-23 23:22:34 PDT
Try run, although I'm not sure how exactly it works given that the patches almost surely don't apply to m-c directly: https://tbpl.mozilla.org/?tree=Try&rev=038ffdbc35f4
Comment 24 Mozilla RelEng Bot 2012-04-24 03:30:21 PDT
Try run for 038ffdbc35f4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=038ffdbc35f4
Results (out of 220 total builds):
    exception: 1
    success: 184
    warnings: 33
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-038ffdbc35f4
Comment 28 Jet Villegas (:jet) 2013-02-06 07:18:05 PST
Bug 812638 backs out part 6 of this patch series.
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-02-07 07:11:18 PST
Part 6 backout:
https://hg.mozilla.org/mozilla-central/rev/41a3c03feff3
Comment 30 :Aryeh Gregor (working until September 2) 2013-03-14 06:51:30 PDT
I'm not sure if we actually want to fix this bug any further than it already is.  If we want a command that supports CSS font sizes, it should probably be an entirely new one, like fontSizePt.  Should we maybe call this WONTFIX?  If so, the spec needs to be updated, probably by changing "effective command value" to treat fontSize more like sub/sup so it ignores CSS.
Comment 31 :Ehsan Akhgari 2013-03-14 08:13:09 PDT
That sounds fine by me, but I'd like Daniel to weigh in as well.
Comment 32 Daniel Glazman (:glazou) 2013-07-17 10:41:16 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #31)
> That sounds fine by me, but I'd like Daniel to weigh in as well.

This seems fine, go ahead.
Comment 33 :Ehsan Akhgari 2013-07-17 14:32:03 PDT
Thanks!

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