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>
Created attachment 66177 [details] work in progress Will submit patch tomorrow morning.
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
Created attachment 66240 [details] work in progress with tests
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
Attachment 66240 [details] did not build on mac: Build output: http://queues.webkit.org/results/3968020
Created attachment 66434 [details] fixes the regression
Attachment 66434 [details] did not build on mac: Build output: http://queues.webkit.org/results/3925061
Created attachment 66442 [details] fixed mac build error
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. :)
(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.
Created attachment 67109 [details] fixed StyleChange::extractTextStyles
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.
(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.
Thanks for the review, Eric. Will land it shortly.
Committed r67145: <http://trac.webkit.org/changeset/67145>
Committed r67170: <http://trac.webkit.org/changeset/67170>