Description was changed from ========== WIP: underline-position for fonts with internal leading BUG=681857 ========== to ...
3 years, 11 months ago
(2017-01-21 17:32:34 UTC)
#1
Description was changed from
==========
WIP: underline-position for fonts with internal leading
BUG=681857
==========
to
==========
WIP: underline-position for fonts with internal leading
BUG=681857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
kojii
Patchset #15 (id:280001) has been deleted
3 years, 7 months ago
(2017-05-01 06:41:28 UTC)
#2
Patchset #15 (id:280001) has been deleted
kojii
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-01 09:26:18 UTC)
#3
3 years, 7 months ago
(2017-05-01 11:42:47 UTC)
#6
Dry run: This issue passed the CQ dry run.
kojii
Description was changed from ========== WIP: underline-position for fonts with internal leading BUG=681857 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== ...
3 years, 7 months ago
(2017-05-01 13:04:01 UTC)
#7
Description was changed from
==========
WIP: underline-position for fonts with internal leading
BUG=681857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fix 'text-underline-position: under' to use em height ascent/descent
This patch changes underline position when 'text-underline-position:
under' is set to the "em height ascent/descent".
Due to large internal leadings in many modern East Asian fonts,
underlines are often drawn too far. This patch changes it to match to
Gecko.
The spec[1] defines to draw under the text content area, which CSS2
leaves explicitly undefined[2].
[1] https://drafts.csswg.org/css-text-decor-3/#underline-under
[2] https://drafts.csswg.org/css2/visudet.html#leading
BUG=681857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/443458)
3 years, 7 months ago
(2017-05-02 14:45:47 UTC)
#21
CQ is committing da patch. Bot data: {"patchset_id": 440001, "attempt_start_ts": 1493768036915640, "parent_rev": "ba2e08f19f4a5967a1eb9c06edc7938c89144063", "commit_rev": "103954da11bdf022c300c4620d39b3b38ef15baa"}
3 years, 7 months ago
(2017-05-02 23:40:01 UTC)
#31
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1493768036915640,
"parent_rev": "ba2e08f19f4a5967a1eb9c06edc7938c89144063", "commit_rev":
"103954da11bdf022c300c4620d39b3b38ef15baa"}
commit-bot: I haz the power
Description was changed from ========== Fix 'text-underline-position: under' to use em height ascent/descent This patch ...
3 years, 7 months ago
(2017-05-02 23:40:09 UTC)
#32
Message was sent while issue was closed.
Description was changed from
==========
Fix 'text-underline-position: under' to use em height ascent/descent
This patch changes underline position when 'text-underline-position:
under' is set to the "em height ascent/descent".
Due to large internal leadings in many modern East Asian fonts,
underlines are often drawn too far. This patch changes it to match to
Gecko.
The spec[1] defines to draw under the text content area, which CSS2
leaves explicitly undefined[2].
[1] https://drafts.csswg.org/css-text-decor-3/#underline-under
[2] https://drafts.csswg.org/css2/visudet.html#leading
BUG=681857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Fix 'text-underline-position: under' to use em height ascent/descent
This patch changes underline position when 'text-underline-position:
under' is set to the "em height ascent/descent".
Due to large internal leadings in many modern East Asian fonts,
underlines are often drawn too far. This patch changes it to match to
Gecko.
The spec[1] defines to draw under the text content area, which CSS2
leaves explicitly undefined[2].
[1] https://drafts.csswg.org/css-text-decor-3/#underline-under
[2] https://drafts.csswg.org/css2/visudet.html#leading
BUG=681857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2643413002
Cr-Commit-Position: refs/heads/master@{#468821}
Committed:
https://chromium.googlesource.com/chromium/src/+/103954da11bdf022c300c4620d39...
==========
commit-bot: I haz the power
Committed patchset #17 (id:440001) as https://chromium.googlesource.com/chromium/src/+/103954da11bdf022c300c4620d39b3b38ef15baa
3 years, 7 months ago
(2017-05-02 23:40:11 UTC)
#33
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right): https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp#newcode417 third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:417: // TODO(kojii): This should move to Skia once finalized. ...
3 years, 7 months ago
(2017-05-03 13:56:37 UTC)
#35
Message was sent while issue was closed.
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:417: // TODO(kojii):
This should move to Skia once finalized. We can then move
What is the reason for not using SkPaint::getFontMetrics and using the fAscent /
fDescent from that? Those are almost always these values anyway, and when
they're not they're some other value the font specified for this purpose. Isn't
that information already available? It appears you're already 'normalizing'
these anyway, so is there a reason that only and exactly the 'typo' versions are
valid?
kojii
On 2017/05/03 at 13:56:37, bungeman wrote: > https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp > File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right): > > https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp#newcode417 ...
3 years, 7 months ago
(2017-05-03 18:07:31 UTC)
#36
Message was sent while issue was closed.
On 2017/05/03 at 13:56:37, bungeman wrote:
>
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):
>
>
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:417: //
TODO(kojii): This should move to Skia once finalized. We can then move
> What is the reason for not using SkPaint::getFontMetrics and using the fAscent
/ fDescent from that? Those are almost always these values anyway, and when
they're not they're some other value the font specified for this purpose. Isn't
that information already available? It appears you're already 'normalizing'
these anyway, so is there a reason that only and exactly the 'typo' versions are
valid?
Hi Ben, thank you for commenting here. I was to ask you a discussion at some
point, but not too early. Originally I was trying to use cap height and asked
your time on it, but then after discussed with Mozilla, it was changed. It was
my bad, sorry for I wasted your time without enough research, and I hope next
time not to waste your time. This is why I thought "not too early."
fAscent/fDescent from SkPaint::getFontMetrics is not always
typoAscender/typoDescender. I'm sure you're more familiar on it, correct me if
my code reading is wrong, but:
* On SkFontHost_FreeType, it's typoAscender/typoDescender if
kUseTypoMetricsMask. Many fonts don't set this flag.
* On Win/Mac, they're from OS. Usually they're not typoAscender/typoDescender.
For the web-compat, we can't change them for layout purposes, but this underline
positioning is quite new, we can still change it.
I'm trying to discuss this results with other browsers, hopefully with feedback
from web authors, to standardize the "em height" based on
typoAscender/typoDescender, and new features (and features that can still
change) to follow it. If this effort is successful, Blink FontMetrics can expose
two sets of ascent/descent; one legacy "ascent/descent" and the other the
standardized "em height ascent/descent", and I'd like to get the two sets from
Skia.
We can do this in Skia either now and follow the discussions as it goes, or wait
for the discussions to stabilize more before working in Skia. Whichever
comfortable for you is great for me. Should we open a tracking bug in skia at
least?
Does this explain well? If anything unclear, I'm more than happy to discuss
more. I'm afraid my English isn't good.
drott
Thanks, Koji, LGTM as discussed in Tokyo, thanks for addressing this issue. Sorry I missed ...
3 years, 7 months ago
(2017-05-08 08:04:00 UTC)
#37
Message was sent while issue was closed.
Thanks, Koji, LGTM as discussed in Tokyo, thanks for addressing this issue.
Sorry I missed the review request earlier. There are good functional
descriptions in the code as comments. As a suggestion for the future, I
personally like longer descriptions in the commit log as well - these are very
helpful when trying to figure out the origins of a certain piece of code later.
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:420: size_t size =
typeface->getTableData(SkSetFourByteTag('O', 'S', '/', '2'), 68,
What's 68 here? Could you make a constant for this?
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:458: // This matches
to how Gecko computes "em height":
Would be great if you had a Mozilla DXR link to where it happens in Gecko.
3 years, 7 months ago
(2017-05-09 04:37:16 UTC)
#38
Message was sent while issue was closed.
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:420: size_t size =
typeface->getTableData(SkSetFourByteTag('O', 'S', '/', '2'), 68,
On 2017/05/08 at 08:04:00, drott wrote:
> What's 68 here? Could you make a constant for this?
It's the offset. Skia has access to os2table so they don't need it but we need
to hard code until this is moved to Skia. Should have commented, thank you
pointing out.
https://codereview.chromium.org/2643413002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:458: // This matches
to how Gecko computes "em height":
On 2017/05/08 at 08:04:00, drott wrote:
> Would be great if you had a Mozilla DXR link to where it happens in Gecko.
Yeah, not great at reading Mozilla code yet...
Issue 2643413002: Fix 'text-underline-position: under' to use em height ascent/descent
(Closed)
Created 3 years, 11 months ago by kojii
Modified 3 years, 7 months ago
Reviewers: bungeman-chromium, drott, eae
Base URL:
Comments: 14