Bug 45026 – REGRESSION: applying new font size causes font-size outside selection to change

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Run execCommand('fontSize', 0, 3) on <font size="7">hello <div>world</div></font>

Expected result:
hello <div style="font-size: -webkit-xxx-large; ">world</div>

Actual result:
hello <div style="font-size: 7px; ">world</div>

[Ryosuke Niwa]

Created attachment 66177 [details]
work in progress

Will submit patch tomorrow morning.

[Ryosuke Niwa]

WebKit currently has a bug that it doesn't add font tag for font-size when the size is specified by a relative term such as x-large, x-small, and I found the following comment inside StyleChange::extractTextStyles.

            // Huge quirk in Microsoft Entourage is that they understand CSS font-size, but also write 
            // out legacy 1-7 values in font tags (I guess for mailers that are not CSS-savvy at all, 
            // like Eudora). Yes, they write out *both*. We need to write out both as well.

Furthermore, we don't currently write out both font tag and style span when font-size are specified by relative terms (x-large, etc...).  We should deprecate this comment since Entourage 2008 uses WebKit:
http://blogs.technet.com/b/amir/archive/2008/01/27/entourage-2008-new-features.aspx

[Ryosuke Niwa]

Created attachment 66240 [details]
work in progress with tests

[Ryosuke Niwa]

In addition to the ones included in the patch, the following tests need rebaseline:
diting/deleting/delete-br-012.html
editing/deleting/delete-select-all-001.html
editing/inserting/5994480-2.html
editing/pasteboard/select-element-1.html
editing/style/block-style-004.html
editing/style/block-style-005.html
editing/style/block-style-006.html
editing/style/fontsize-1.html

[Eric Seidel (no email)]

Attachment 66240 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3968020

[Ryosuke Niwa]

Created attachment 66434 [details]
fixes the regression

[Eric Seidel (no email)]

Attachment 66434 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3925061

[Ryosuke Niwa]

Created attachment 66442 [details]
fixed mac build error

[Eric Seidel (no email)]

Comment on attachment 66442 [details]
fixed mac build error

View in context: https://bugs.webkit.org/attachment.cgi?id=66442&action=prettypatch

> LayoutTests/editing/deleting/delete-br-012-expected.txt:21
> -|     <span>
> -|       class="Apple-style-span"
> -|       style="font-size: 24px;"
> -|       <#selection-caret>
> -|       <br>
> +|     <#selection-caret>
Did the style information get lost here?

> LayoutTests/editing/deleting/delete-select-all-001-expected.txt:2
> +execDeleteCommand: <font class="Apple-style-span" size="3"><span class="Apple-style-span" style="-webkit-border-horizontal-spacing: 2px; -webkit-border-vertical-spacing: 2px;"><br></span></font>
Should this have "Apple-style-span"?  It's not a span.

> LayoutTests/editing/inserting/5994480-2-expected.txt:1
> -<font class="Apple-style-span" face="'Lucida Grande'" size="3"><span class="Apple-style-span" style="font-size: 11px;"><br></span></font>
> +<font class="Apple-style-span" face="'Lucida Grande'" size="3"><br></font>
Did the style get lost here?

Looks OK.  Please respond to above before landing. :)

[Ryosuke Niwa]

(In reply to comment #9)
> (From update of attachment 66442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66442&action=prettypatch
> 
> > LayoutTests/editing/deleting/delete-br-012-expected.txt:21
> > -|     <span>
> > -|       class="Apple-style-span"
> > -|       style="font-size: 24px;"
> > -|       <#selection-caret>
> > -|       <br>
> > +|     <#selection-caret>
> Did the style information get lost here?

No, above that line, we have 
|   <font>
|     class="Apple-style-span"
|     size="6"

and 24px = 6 legacy font size.  That's what I thought but with the default font size, legacy font size 6 corresponds to 26px :(  It turned out that StyleChange::extractTextStyles has the following conversion logic:

263	            // Only accept absolute scale
264	            if (value->primitiveType() >= CSSPrimitiveValue::CSS_PX && value->primitiveType() <= CSSPrimitiveValue::CSS_PC) {
265	                float number = value->getFloatValue(CSSPrimitiveValue::CSS_PX);
266	                if (number <= 9)
267	                    m_applyFontSize = "1";
268	                else if (number <= 10)
269	                    m_applyFontSize = "2";
270	                else if (number <= 13)
271	                    m_applyFontSize = "3";
272	                else if (number <= 16)
273	                    m_applyFontSize = "4";
274	                else if (number <= 18)
275	                    m_applyFontSize = "5";
276	                else if (number <= 24)
277	                    m_applyFontSize = "6";
278	                else
279	                    m_applyFontSize = "7";
280	            }

I need to replace all of that with legacyFontSize that I just committed in http://trac.webkit.org/changeset/67102.

[Ryosuke Niwa]

Created attachment 67109 [details]
fixed StyleChange::extractTextStyles

[Eric Seidel (no email)]

Comment on attachment 67109 [details]
fixed StyleChange::extractTextStyles

View in context: https://bugs.webkit.org/attachment.cgi?id=67109&action=prettypatch

> LayoutTests/editing/deleting/delete-br-012-expected.txt:22
> -|     size="6"
> -|     <span>
> -|       class="Apple-style-span"
> -|       style="font-size: 24px;"
> -|       <#selection-caret>
> -|       <br>
> +|     size="5"
> +|     <#selection-caret>
> +|     <br>
So again, this is an intentional change from 6 to 5, right?

Looks sane.  I trust you.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 67109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67109&action=prettypatch
> 
> > LayoutTests/editing/deleting/delete-br-012-expected.txt:22
> > -|     size="6"
> > -|     <span>
> > -|       class="Apple-style-span"
> > -|       style="font-size: 24px;"
> > -|       <#selection-caret>
> > -|       <br>
> > +|     size="5"
> > +|     <#selection-caret>
> > +|     <br>
> So again, this is an intentional change from 6 to 5, right?
> 
> Looks sane.  I trust you.

Yes.  With proportional font's default font size, we should be getting legacy font size of 5 instead of 6 for the pixel font size of 24px. What was happening was that we were converting value incorrectly to 6 and added <font size="6"> at one point, but later we added another span with font-size: 24px because computed style inside this font element had font-size: 32px.

[Ryosuke Niwa]

Thanks for the review, Eric.  Will land it shortly.

[Ryosuke Niwa]

Committed r67145: <http://trac.webkit.org/changeset/67145>

[Ryosuke Niwa]

Committed r67170: <http://trac.webkit.org/changeset/67170>