The default bug view has changed. See this FAQ.

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

RESOLVED WONTFIX

Status

()

Core
Editor
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: Marcos Almeida, Unassigned)

Tracking

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

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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

8 years ago
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: 685931
Status: NEW → ASSIGNED
Hardware: x86 → All
Version: unspecified → Trunk
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.
Attachment #615268 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 6

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

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

Updated

5 years ago
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)) {
    // ...
http://hg.mozilla.org/projects/birch/rev/90030a1e28e0
Whiteboard: [Leave open after merge]
Depends on: 747879
Depends on: 747889
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.  :)
Attachment #617469 - Flags: review?(bzbarsky)
Created attachment 617470 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsHTMLCSSUtils::GenerateCSSDeclarationsFromHTMLStyle
Attachment #617470 - Flags: review?(ehsan)
Created attachment 617471 [details] [diff] [review]
Patch part 4, v1 -- Clean up nsHTMLDocument::ConvertToMidasInternalCommandInner
Attachment #617471 - Flags: review?(ehsan)
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>.
Attachment #617474 - Flags: review?(ehsan)
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.
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-

Updated

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

Comment 24

5 years ago
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
http://hg.mozilla.org/projects/birch/rev/1b262dac0f23
http://hg.mozilla.org/projects/birch/rev/0c7b1a2b2472
http://hg.mozilla.org/projects/birch/rev/4096c5e3d194
http://hg.mozilla.org/projects/birch/rev/e6f2d9243ee8
http://hg.mozilla.org/projects/birch/rev/7718c5794946
Whiteboard: [Leave open after merge]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/90030a1e28e0
https://hg.mozilla.org/mozilla-central/rev/1b262dac0f23
https://hg.mozilla.org/mozilla-central/rev/0c7b1a2b2472
https://hg.mozilla.org/mozilla-central/rev/4096c5e3d194
https://hg.mozilla.org/mozilla-central/rev/e6f2d9243ee8
https://hg.mozilla.org/mozilla-central/rev/7718c5794946
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 812638

Comment 28

4 years ago
Bug 812638 backs out part 6 of this patch series.

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Part 6 backout:
https://hg.mozilla.org/mozilla-central/rev/41a3c03feff3
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
Last Resolved: 5 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.