WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45026
REGRESSION: applying new font size causes font-size outside selection to change
https://bugs.webkit.org/show_bug.cgi?id=45026
Summary
REGRESSION: applying new font size causes font-size outside selection to change
Ryosuke Niwa
Reported
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>
Attachments
work in progress
(6.51 KB, patch)
2010-09-01 01:12 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
work in progress with tests
(24.75 KB, patch)
2010-09-01 11:38 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
fixes the regression
(34.66 KB, patch)
2010-09-02 17:11 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
fixed mac build error
(34.84 KB, patch)
2010-09-02 17:40 PDT
,
Ryosuke Niwa
no flags
Details
Formatted Diff
Diff
fixed StyleChange::extractTextStyles
(46.19 KB, patch)
2010-09-09 16:05 PDT
,
Ryosuke Niwa
eric
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
2010-09-01 01:12:34 PDT
Created
attachment 66177
[details]
work in progress Will submit patch tomorrow morning.
Ryosuke Niwa
Comment 2
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
Ryosuke Niwa
Comment 3
2010-09-01 11:38:14 PDT
Created
attachment 66240
[details]
work in progress with tests
Ryosuke Niwa
Comment 4
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
Eric Seidel (no email)
Comment 5
2010-09-01 11:53:22 PDT
Attachment 66240
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/3968020
Ryosuke Niwa
Comment 6
2010-09-02 17:11:07 PDT
Created
attachment 66434
[details]
fixes the regression
Eric Seidel (no email)
Comment 7
2010-09-02 17:25:06 PDT
Attachment 66434
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/3925061
Ryosuke Niwa
Comment 8
2010-09-02 17:40:59 PDT
Created
attachment 66442
[details]
fixed mac build error
Eric Seidel (no email)
Comment 9
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. :)
Ryosuke Niwa
Comment 10
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
.
Ryosuke Niwa
Comment 11
2010-09-09 16:05:39 PDT
Created
attachment 67109
[details]
fixed StyleChange::extractTextStyles
Eric Seidel (no email)
Comment 12
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.
Ryosuke Niwa
Comment 13
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.
Ryosuke Niwa
Comment 14
2010-09-09 16:30:05 PDT
Thanks for the review, Eric. Will land it shortly.
Ryosuke Niwa
Comment 15
2010-09-09 19:09:55 PDT
Committed
r67145
: <
http://trac.webkit.org/changeset/67145
>
Ryosuke Niwa
Comment 16
2010-09-09 22:39:29 PDT
Committed
r67170
: <
http://trac.webkit.org/changeset/67170
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug