Bug 45026

Summary: REGRESSION: applying new font size causes font-size outside selection to change
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Major CC: darin, enrica, eric, mjs, ojan, tony
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 45050, 45121    
Bug Blocks: 28968    
Attachments:
Description Flags
work in progress
none
work in progress with tests
none
fixes the regression
none
fixed mac build error
none
fixed StyleChange::extractTextStyles eric: review+

Description Ryosuke Niwa 2010-09-01 01:11:37 PDT
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>
Comment 1 Ryosuke Niwa 2010-09-01 01:12:34 PDT
Created attachment 66177 [details]
work in progress

Will submit patch tomorrow morning.
Comment 2 Ryosuke Niwa 2010-09-01 10:34:10 PDT
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
Comment 3 Ryosuke Niwa 2010-09-01 11:38:14 PDT
Created attachment 66240 [details]
work in progress with tests
Comment 4 Ryosuke Niwa 2010-09-01 11:38:45 PDT
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
Comment 5 Eric Seidel (no email) 2010-09-01 11:53:22 PDT
Attachment 66240 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3968020
Comment 6 Ryosuke Niwa 2010-09-02 17:11:07 PDT
Created attachment 66434 [details]
fixes the regression
Comment 7 Eric Seidel (no email) 2010-09-02 17:25:06 PDT
Attachment 66434 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3925061
Comment 8 Ryosuke Niwa 2010-09-02 17:40:59 PDT
Created attachment 66442 [details]
fixed mac build error
Comment 9 Eric Seidel (no email) 2010-09-09 13:21:53 PDT
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. :)
Comment 10 Ryosuke Niwa 2010-09-09 14:28:12 PDT
(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.
Comment 11 Ryosuke Niwa 2010-09-09 16:05:39 PDT
Created attachment 67109 [details]
fixed StyleChange::extractTextStyles
Comment 12 Eric Seidel (no email) 2010-09-09 16:08:31 PDT
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.
Comment 13 Ryosuke Niwa 2010-09-09 16:28:47 PDT
(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.
Comment 14 Ryosuke Niwa 2010-09-09 16:30:05 PDT
Thanks for the review, Eric.  Will land it shortly.
Comment 15 Ryosuke Niwa 2010-09-09 19:09:55 PDT
Committed r67145: <http://trac.webkit.org/changeset/67145>
Comment 16 Ryosuke Niwa 2010-09-09 22:39:29 PDT
Committed r67170: <http://trac.webkit.org/changeset/67170>