Initial implementation of font-size-adjust

Initial implementation of font-size-adjust

This patch implements the font-size-adjust css property specified in
http://www.w3.org/TR/css-fonts-3/#font-size-adjust-prop
How it works is simple. A content author set a aspect value (x-height of font / font-size)
to font-size-adjust then blink will adjust the actual font size to look similar size
whatever font is used. The fomula to adjust the size is like below.
adjustedFontSize = (aspectValue specified by font-size-adjust * aspectValue of actual font) * font-size

BUG=451346
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191273

[changseok, 2015-02-19 09:15:01]

shivamidow@gmail.com changed reviewers:
+ darin@chromium.org, eae@chromium.org, tabatkins@chromium.org,
thakis@chromium.org

[changseok, 2015-02-19 09:15:01]

I don't know who is a proper reviewer for the font module so that I add a few
reviewers recommended by git-cl. Please add a better reviewer if you know.
Anyway please have a look.

[changseok, 2015-02-19 09:18:33]

On 2015/02/19 09:15:01, changseok wrote:
> I don't know who is a proper reviewer for the font module so that I add a few
> reviewers recommended by git-cl. Please add a better reviewer if you know.
> Anyway please have a look.

One more thing, I implemented this on mac. I believe the layout test on mac will
pass though, not sure if other platforms do. 
Please trigger a layout test for linux, window and android.

[Nico, 2015-02-19 15:29:37]

eae is probably a good reviewer for this.

[rune, 2015-02-19 20:28:26]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2015-02-19 20:28:27]

Drive-by-review:

In general, you need to support 'none'. Both for parsing, computed style, and
handle it in the used style.

You should test parsing, both for valid and invalid values. Also a serialization
test for CSSOM/computedStyle.

Text rendering is platform dependent, so you need to have separate png and
render tree dump expectations for each platform. Also, I don't think you can
expect the bots to have certain fonts installed.

The question is if it's possible to make a reference test for this. If you have
a font resource in LayoutTests with a known aspect ratio, and pick a
font-size-adjust value in the test that will end up with a given adjusted
font-size that you can use explicitly for font-size in the reference?

https://codereview.chromium.org/943463002/diff/1/Source/core/css/LayoutStyleC...
File Source/core/css/LayoutStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/943463002/diff/1/Source/core/css/LayoutStyleC...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1561: return
cssValuePool().createValue(style.fontDescription().sizeAdjust(),
CSSPrimitiveValue::CSS_NUMBER);
Need to handle 'none'.

https://codereview.chromium.org/943463002/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/943463002/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:1052: case
CSSPropertyFontSizeAdjust: // none | <number>
Missing handling for 'none' keyword?

[Timothy Loh, 2015-02-19 23:00:46]

https://codereview.chromium.org/943463002/diff/1/Source/core/css/CSSPropertie...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/943463002/diff/1/Source/core/css/CSSPropertie...
Source/core/css/CSSProperties.in:88: font-size-adjust
runtime_flag=CSSFontSizeAdjust, animatable, inherited, font, type_name=float,
name_for_methods=SizeAdjust
I'd drop the animatable flag for a later patch, since you need to touch a few
other files for it to work properly.

[eae, 2015-02-19 23:32:23]

Implementation looks good, I do wonder if we could change computedSize to adjust
for the adjustedSize instead of introducing yet another font-size accessor.

[changseok, 2015-02-24 16:16:48]

On 2015/02/19 20:28:27, rune wrote:
> Drive-by-review:
> 
> In general, you need to support 'none'. Both for parsing, computed style, and
> handle it in the used style.
Yup. fixed.

> You should test parsing, both for valid and invalid values. Also a
serialization
> test for CSSOM/computedStyle.
O.K added one more test.
 
> Text rendering is platform dependent, so you need to have separate png and
> render tree dump expectations for each platform. Also, I don't think you can
> expect the bots to have certain fonts installed.
> 
> The question is if it's possible to make a reference test for this. If you
have
> a font resource in LayoutTests with a known aspect ratio, and pick a
> font-size-adjust value in the test that will end up with a given adjusted
> font-size that you can use explicitly for font-size in the reference?
It sounds make sense. Luckily I found available web fonts in LayoutTests,
LinLibertine_R.woff and OpenSans-Regular.woff.
We can make all platforms use them with @font-face rule.  
 
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/LayoutStyleC...
> File Source/core/css/LayoutStyleCSSValueMapping.cpp (right):
> 
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/LayoutStyleC...
> Source/core/css/LayoutStyleCSSValueMapping.cpp:1561: return
> cssValuePool().createValue(style.fontDescription().sizeAdjust(),
> CSSPrimitiveValue::CSS_NUMBER);
> Need to handle 'none'.
> 
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/parser/CSSPr...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/parser/CSSPr...
> Source/core/css/parser/CSSPropertyParser.cpp:1052: case
> CSSPropertyFontSizeAdjust: // none | <number>
> Missing handling for 'none' keyword?
Covered 'none'

[changseok, 2015-02-24 16:18:36]

On 2015/02/19 23:00:46, Timothy Loh wrote:
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/CSSPropertie...
> File Source/core/css/CSSProperties.in (right):
> 
>
https://codereview.chromium.org/943463002/diff/1/Source/core/css/CSSPropertie...
> Source/core/css/CSSProperties.in:88: font-size-adjust
> runtime_flag=CSSFontSizeAdjust, animatable, inherited, font, type_name=float,
> name_for_methods=SizeAdjust
> I'd drop the animatable flag for a later patch, since you need to touch a few
> other files for it to work properly.

Oops. right I'd like to deal with the animation stuff later. I forgot to remove
the animatable. Thanks for pointing out.

[changseok, 2015-02-24 16:33:06]

On 2015/02/19 23:32:23, eae wrote:
> Implementation looks good, I do wonder if we could change computedSize to
adjust
> for the adjustedSize instead of introducing yet another font-size accessor.
Good question. According to the spec, font-size-adjust should not affect
line-height or computed font-size. If we just set the calculated adjustedSize to
computedSize, font-size and line-height would be affected because of the changed
computedSize. I ran and tested how firefox worked with font-size-adjust. It
showed same font-size and line-height regardless of font-size-adjust. Actually
firefox has used a seperated variable for the adjusted size.
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#2039

[Tab Atkins, 2015-02-24 18:35:16]

On 2015/02/24 16:33:06, changseok wrote:
> On 2015/02/19 23:32:23, eae wrote:
> > Implementation looks good, I do wonder if we could change computedSize to
> adjust
> > for the adjustedSize instead of introducing yet another font-size accessor.
> Good question. According to the spec, font-size-adjust should not affect
> line-height or computed font-size. If we just set the calculated adjustedSize
to
> computedSize, font-size and line-height would be affected because of the
changed
> computedSize. I ran and tested how firefox worked with font-size-adjust. It
> showed same font-size and line-height regardless of font-size-adjust. Actually
> firefox has used a seperated variable for the adjusted size.
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#2039

Correct, font-size-adjust does not affect the line-height; if it did, you'd get
unpredictable and unattractive line-height differences within paragraphs.

It also doesn't affect font-size, because it's defined *in terms of* font-size.

[changseok, 2015-02-25 17:46:06]

Hrm. I just ran and found the pixel test failed on linux. The x-height is
slightly different on mac and linux even though we set a same font through
@font-face.
So.. seperated test results for each platform are required. Let me update the
patch soon.

[changseok, 2015-02-26 05:25:00]

Please review again.

[rune, 2015-02-27 10:52:04]

On 2015/02/25 17:46:06, changseok wrote:
> Hrm. I just ran and found the pixel test failed on linux. The x-height is
> slightly different on mac and linux even though we set a same font through
> @font-face.
> So.. seperated test results for each platform are required. Let me update the
> patch soon.

Would that be the case for Ahem as well? And does Ahem have an x-height
different from 1 to make the test interesting?

It would be good to avoid png and platform expectations if possible, I think.

[changseok, 2015-03-01 17:40:17]

On 2015/02/27 10:52:04, rune wrote:
> Would that be the case for Ahem as well? And does Ahem have an x-height
> different from 1 to make the test interesting?
No. Ahem is also interpreted differently depending on mac and linux(ubuntu
14.10). 
Its aspectValue is 1.001000 on mac, but 0.800000 on linux. I guess this comes
from a difference of font interpreting engines between CTFont module for mac and
FreeType for linux, but no detail I have.

> It would be good to avoid png and platform expectations if possible, I think.
Generally I agree with you. I also prefer not to use a pixel test though, I
don't know how we can perfectly test this except the pixel test.
As you already know, font-size-adjust doesn't affect line-height and
computed-font-size so that it's meaningless comparing them between a test font
and a reference font.
And to make it worse, no api is provided to get a actual adjusted size. 

What I can think out is to compare font sizes with an indirect way through
getBoundingClientRect. 
It would not require png test results, but we would still need to add each
platform expectations because of the differences among font engines as I
mentioned above. 
I like this way. Any better idea?

[changseok, 2015-03-02 07:20:08]

lgtm

[changseok, 2015-03-02 07:20:36]

On 2015/03/02 at 07:20:08, changseok wrote:
> lgtm

Oops. sorry.

[rune, 2015-03-02 09:05:16]

On 2015/03/01 17:40:17, changseok wrote:
> On 2015/02/27 10:52:04, rune wrote:
> > Would that be the case for Ahem as well? And does Ahem have an x-height
> > different from 1 to make the test interesting?
> No. Ahem is also interpreted differently depending on mac and linux(ubuntu
> 14.10). 
> Its aspectValue is 1.001000 on mac, but 0.800000 on linux. I guess this comes
> from a difference of font interpreting engines between CTFont module for mac
and
> FreeType for linux, but no detail I have.
> 
> > It would be good to avoid png and platform expectations if possible, I
think.
> Generally I agree with you. I also prefer not to use a pixel test though, I
> don't know how we can perfectly test this except the pixel test.
> As you already know, font-size-adjust doesn't affect line-height and
> computed-font-size so that it's meaningless comparing them between a test font
> and a reference font.
> And to make it worse, no api is provided to get a actual adjusted size. 
> 
> What I can think out is to compare font sizes with an indirect way through
> getBoundingClientRect. 
> It would not require png test results, but we would still need to add each
> platform expectations because of the differences among font engines as I
> mentioned above. 
> I like this way. Any better idea?

If we can't get platform-independent x-heights, I'd say we stick with the png
results.

[rune, 2015-03-02 09:08:36]

Could you add tests to check that:

- Negative values are dropped at parse time
- 0 value is dropped at parse time
- 'none' is an accepted value at parse time

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/co...
File
LayoutTests/fast/text/computed-line-height-and-font-size-with-font-size-adjust.html
(right):

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/co...
LayoutTests/fast/text/computed-line-height-and-font-size-with-font-size-adjust.html:1:
<html>
- Use <!DOCTYPE html>
- No html, head, or body tags when not necessary.
- Run test inline in script. No need for onload handler here.

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
File LayoutTests/fast/text/font-size-adjust.html (right):

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-size-adjust.html:2: <html>
Skip html, head, and body tags.

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-size-adjust.html:5: div.container {
Nit: no need to add the tag name in any of the selectors here.

[changseok, 2015-03-02 11:26:07]

On 2015/03/02 09:08:36, rune wrote:
> Could you add tests to check that:
> 
> - Negative values are dropped at parse time
> - 0 value is dropped at parse time
> - 'none' is an accepted value at parse time
Is there any formal test for these? I would appreciate if you could point out a
similar test to these. =)

[rune, 2015-03-02 11:53:52]

On 2015/03/02 11:26:07, changseok wrote:
> On 2015/03/02 09:08:36, rune wrote:
> > Could you add tests to check that:
> > 
> > - Negative values are dropped at parse time
> > - 0 value is dropped at parse time
> > - 'none' is an accepted value at parse time
> Is there any formal test for these? I would appreciate if you could point out
a
> similar test to these. =)

Valid value parsing is not tested in a consistent manner afaik. I thought about
something like this:

<!DOCTYPE html>
<style>
div { font-size-adjust: none }
div { font-size-adjust: 0 }
div { font-size-adjust: -1 }
</style>
<script>
description("...");

var rules = document.styleSheets[0].cssRules;

shouldBe("rules.length", "3");
shouldBe("rules[0].style.length", "1");
shouldBe("rules[1].style.length", "0");
shouldBe("rules[2].style.length", "0");
</script>

[Timothy Loh, 2015-03-02 12:04:32]

On 2015/03/02 11:53:52, rune wrote:
> Valid value parsing is not tested in a consistent manner afaik. I thought
about
> something like this:
> 
> <!DOCTYPE html>
> <style>
> div { font-size-adjust: none }
> div { font-size-adjust: 0 }
> div { font-size-adjust: -1 }
> </style>
> <script>
> description("...");
> 
> var rules = document.styleSheets[0].cssRules;
> 
> shouldBe("rules.length", "3");
> shouldBe("rules[0].style.length", "1");
> shouldBe("rules[1].style.length", "0");
> shouldBe("rules[2].style.length", "0");
> </script>

The helper LayoutTests/css-parser/resources/property-parsing-test.js should make
this easy to do.

[rune, 2015-03-02 12:14:41]

On 2015/03/02 12:04:32, Timothy Loh wrote:

> The helper LayoutTests/css-parser/resources/property-parsing-test.js should
make
> this easy to do.

Ah. Thanks.

[changseok, 2015-03-02 15:57:49]

Thank you all for the kind comments. It was much helpful! The updated patch is
coming.

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/co...
File
LayoutTests/fast/text/computed-line-height-and-font-size-with-font-size-adjust.html
(right):

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/co...
LayoutTests/fast/text/computed-line-height-and-font-size-with-font-size-adjust.html:1:
<html>
On 2015/03/02 09:08:36, rune wrote:
> - Use <!DOCTYPE html>
> - No html, head, or body tags when not necessary.
> - Run test inline in script. No need for onload handler here.

Done.

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
File LayoutTests/fast/text/font-size-adjust.html (right):

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-size-adjust.html:2: <html>
On 2015/03/02 09:08:36, rune wrote:
> Skip html, head, and body tags.

Done.

https://codereview.chromium.org/943463002/diff/40001/LayoutTests/fast/text/fo...
LayoutTests/fast/text/font-size-adjust.html:5: div.container {
On 2015/03/02 09:08:36, rune wrote:
> Nit: no need to add the tag name in any of the selectors here.

Done.

[changseok, 2015-03-03 15:05:24]

Any more comments? =)

[eae, 2015-03-03 18:14:10]

LGTM but please wait for rune to sign off before landing.

Thank you!

[rune, 2015-03-03 18:58:48]

Sorry for not seeing the 0/none issue in previous reviews.

One note: you should rebase in separate patch sets so that it's easier to see
the differences and quicker to review.

https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
File LayoutTests/css-parser/font-size-adjust.html (right):

https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
LayoutTests/css-parser/font-size-adjust.html:11:
assert_valid_value("fontSizeAdjust", "0");
I think 0 should be allowed, but IIRC, Gecko drops it. But, since you represent
'none' as 0 internally, won't this be interpreted as 'none'?

[changseok, 2015-03-03 19:39:42]

> One note: you should rebase in separate patch sets so that it's easier to see
the differences and quicker to review.
O.K. I'll keep in mind.

https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
File LayoutTests/css-parser/font-size-adjust.html (right):

https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
LayoutTests/css-parser/font-size-adjust.html:11:
assert_valid_value("fontSizeAdjust", "0");
On 2015/03/03 18:58:48, rune wrote:
> I think 0 should be allowed, but IIRC, Gecko drops it. But, since you
represent
> 'none' as 0 internally, won't this be interpreted as 'none'?

Gecko doesn't drop 0 and it interprets 0 as none internally for the computed
style of font-size-adjust as you said.
Current implementation shows exactly same behavior with that of Gecko. =)

You can try it in
http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_style_fontsizeadjust

[rune, 2015-03-03 20:09:21]

On 2015/03/03 19:39:42, changseok wrote:

>
https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
> File LayoutTests/css-parser/font-size-adjust.html (right):
> 
>
https://codereview.chromium.org/943463002/diff/60001/LayoutTests/css-parser/f...
> LayoutTests/css-parser/font-size-adjust.html:11:
> assert_valid_value("fontSizeAdjust", "0");
> On 2015/03/03 18:58:48, rune wrote:
> > I think 0 should be allowed, but IIRC, Gecko drops it. But, since you
> represent
> > 'none' as 0 internally, won't this be interpreted as 'none'?
> 
> Gecko doesn't drop 0 and it interprets 0 as none internally for the computed
> style of font-size-adjust as you said.
> Current implementation shows exactly same behavior with that of Gecko. =)
> 
> You can try it in
>
http://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_style_fontsizeadjust

You're right.

I wouldn't expect 0 to behave like 'none', but since Gecko does, I'm OK with it.

lgtm

[changseok, 2015-03-04 04:38:18]

The CQ bit was checked by shivamidow@gmail.com

[commit-bot: I haz the power, 2015-03-04 04:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/943463002/60001

[commit-bot: I haz the power, 2015-03-04 05:34:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-04 05:34:45]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[changseok, 2015-03-04 08:21:35]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-03-04 08:21:36]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
rune@opera.com
Link to the patchset: https://codereview.chromium.org/943463002/#ps80001 (title:
"RebasedPatch")

[commit-bot: I haz the power, 2015-03-04 08:22:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/943463002/80001

[changseok, 2015-03-04 10:00:56]

The CQ bit was checked by shivamidow@gmail.com

[changseok, 2015-03-04 10:00:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
rune@opera.com
Link to the patchset: https://codereview.chromium.org/943463002/#ps100001
(title: "Add some tests which need a rebaseline to TestExpectation")

[commit-bot: I haz the power, 2015-03-04 10:01:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/943463002/100001

[commit-bot: I haz the power, 2015-03-04 11:29:18]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191273

[changseok, 2015-03-04 11:30:39]

On 2015/03/04 11:29:18, I haz the power (commit-bot) wrote:
> Committed patchset #6 (id:100001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=191273

Finally! Thanks all \o/