Closed Bug 480647 Opened 15 years ago Closed 11 years ago

execCommand('fontSize') doesn't change font size if content font size is specified in css

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla15

People

(Reporter: marcosalmeida, Unassigned)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(6 files)

Patch part 1, v1 -- Clean up nsHTMLCSSUtils::IsCSSEquivalentToHTMLInlineStyleSet
8.09 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Move ParseLegacyFontSize to nsContentUtils so editor/ can use it
5.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up nsHTMLCSSUtils::GenerateCSSDeclarationsFromHTMLStyle
7.38 KB, patch
ehsan.akhgari
: review-
Details | Diff | Splinter Review
Patch part 4, v1 -- Clean up nsHTMLDocument::ConvertToMidasInternalCommandInner
6.20 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Reject invalid values from execCommand("fontSize")
10.64 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 6, v1 -- Handle CSS in execCommand("fontSize")
26.18 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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.
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>
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>
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(This is tested already by richtext and richtext2.)
Flags: in-testsuite+
Assignee: nobody → ayg
Blocks: richtext2
Status: NEW → ASSIGNED
Hardware: x86 → All
Version: unspecified → Trunk
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.
Attachment #615268 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
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.
Attachment #615268 - Flags: review?(ehsan) → review+
What indentation would you like to see for this sort of complicated conditional?
(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)) {
    // ...
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>.
Attachment #617474 - Flags: review?(ehsan)
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.
Attachment #617477 - Flags: review?(ehsan)
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. :(
Attachment #617470 - Flags: review?(ehsan) → review-
Attachment #617471 - Flags: review?(ehsan) → review+
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.
Attachment #617474 - Flags: review?(ehsan) → review+
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.
Attachment #617477 - Flags: review?(ehsan) → review+
(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.)
(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 on attachment 617469 [details] [diff] [review]
Patch part 2, v1 -- Move ParseLegacyFontSize to nsContentUtils so editor/ can use it

r=me
Attachment #617469 - Flags: review?(bzbarsky) → review+
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
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
Depends on: font_size_bloat
Bug 812638 backs out part 6 of this patch series.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: ayg → nobody
Flags: needinfo?(ehsan)
That sounds fine by me, but I'd like Daniel to weigh in as well.
Flags: needinfo?(ehsan) → needinfo?(daniel)
(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.
Flags: needinfo?(daniel)
Thanks!
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: