Bug 16768 – Position and thickness of underline should scale accordingly as font size changes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Niels Matthijs]

Maybe it's not really a bug, but it's an issue nonetheless.

I've been looking into the underlining of elements cross-browser. The results can be seen here:

http://www.onderhond.com/blog/work/underlining-inconsistency

As you can see, Webkit (Safari) underlines right beneath the baseline of the text. And as the font sizes, the underlining doesn't size with it.

First of all, when the underline hits the baseline of the text, this makes the text somewhat harder to read. And as people size their fonts (bad vision), the underlining doesn't size with it (so it won't be as visible to them).

Personally, I like the way Opera handles it, placing the underline between the baseline and the absbottom of the text, still sizing the underline as the font size grows.

It's a small issue, but hopefully something can be done about this.

[Mark Rowe (bdash)]

For what it's worth, at small font sizes the WebKit approach looks nicer to me as there is not a large gap between the text and the underline like there is in Firefox and Opera.  At larger font sizes, WebKits approach does not look so good.

[Niels Matthijs]

(In reply to comment #1)
> For what it's worth, at small font sizes the WebKit approach looks nicer to me
> as there is not a large gap between the text and the underline like there is in
> Firefox and Opera.  At larger font sizes, WebKits approach does not look so
> good.
> 

It still looks pretty cluttered to me at small font sizes, but it doesn't really touch the baseline of the text, which is at least something.

Seeing as the positioning isn't very stable when sizing the font, it would be suitable to change the algorithm to create a more logical output I think.

And the remark about the thickness of the line remains.

[Yusuke Sato]

Created attachment 31709 [details]
font_underlining_patch

 LayoutTests/ChangeLog                              |   14 +++++
 LayoutTests/fonts/underline.html                   |   26 +++++++++
 LayoutTests/fonts/underline_strict.html            |   27 +++++++++
 .../platform/mac/fonts/underline-expected.checksum |    1 +
 .../platform/mac/fonts/underline-expected.png      |  Bin 0 -> 19706 bytes
 .../platform/mac/fonts/underline-expected.txt      |   34 ++++++++++++
 .../mac/fonts/underline_strict-expected.checksum   |    1 +
 .../mac/fonts/underline_strict-expected.png        |  Bin 0 -> 19706 bytes
 .../mac/fonts/underline_strict-expected.txt        |   34 ++++++++++++
 WebCore/ChangeLog                                  |   50 +++++++++++++++++
 WebCore/platform/graphics/Font.cpp                 |   20 +++++++
 WebCore/platform/graphics/Font.h                   |    2 +
 WebCore/platform/graphics/FontFastPath.cpp         |   34 ++++++++++++
 WebCore/platform/graphics/SimpleFontData.cpp       |    4 +-
 WebCore/platform/graphics/SimpleFontData.h         |    4 ++
 .../chromium/SimpleFontDataChromiumWin.cpp         |   13 +++++
 .../graphics/opentype/OpenTypeUtilities.cpp        |   57 ++++++++++++++++++++
 .../platform/graphics/opentype/OpenTypeUtilities.h |    4 ++
 WebCore/rendering/InlineFlowBox.cpp                |   35 +++++++++++-
 WebCore/rendering/InlineTextBox.cpp                |   57 +++++++++++---------
 WebCore/rendering/InlineTextBox.h                  |    5 ++-
 21 files changed, 391 insertions(+), 31 deletions(-)

[Yusuke Sato]

This issue is also tracked in Chromium as http://crbug.com/3715 and http://crbug.com/13602.

I've created a patch which fixes this issue by extracting a proper underline position and thickness in OpenType table and uses them inside the renderer.

> at small font sizes the WebKit approach looks nicer to me
> as there is not a large gap between the text and the underline like there is in
> Firefox and Opera.

For CJK pages, current underlining algorithm in renderer/Inline{Flow,Text}Box.cpp seems not so good even at small font sizes. This is an example. 

http://chromium.googlecode.com/issues/attachment?aid=-4813356322599644233&name=Japanese_small_MSPGothic_before.jpg
http://chromium.googlecode.com/issues/attachment?aid=-458069664526333573&name=Japanese_small_MSPGothic_after.jpg (after applying my patch)
http://chromium.googlecode.com/issues/attachment?aid=-6748582343719241567&name=Japanese_small_Meiryo_before.jpg
http://chromium.googlecode.com/issues/attachment?aid=-4540227136271297334&name=Japanese_small_Meiryo_after.jpg

As you can see, underlines touch some Japanese glyphs. It's ugly and even hard to read sometimes. Though I used Chromium trunk for Windows in the screenshot, Safari 4 for Windows seems to have the same problem.

> At larger font sizes, WebKits approach does not look so good.

Yes, for larger fonts, most browsers (Safari Mac/Win and Chromium Win, at least) don't look so good as Mark mentioned. For Latin fonts, underline is too thin (I think this may cause accessibility problems), and for CJK fonts, in addition to that, the underline position is too high and touches most glyphs.

There are some examples too:
http://chromium.googlecode.com/issues/attachment?aid=-7537620041365127775&name=Chinese_large_defaultfont-arial_before.jpg
http://chromium.googlecode.com/issues/attachment?aid=-2218771908825657487&name=Chinese_large_defaultfont-arial_after.jpg
http://chromium.googlecode.com/issues/attachment?aid=-6827476323788836657&name=Japanese_large_Meiryo_before.jpg
http://chromium.googlecode.com/issues/attachment?aid=2117039701562505555&name=Japanese_large_Meiryo_after.jpg

Could someone review this? Though attached patch is only for Chromium, I'll modify Safari implementations of SimpleFontData if necessary.

Thanks.

[Mark Rowe (bdash)]

I'm a little bit confused.  The bug is a cross-platform issue, so a Chromium-only Windows-only patch does not address it.  There are pixel tests for this for Mac, even though the fix doesn't address the problem on Mac.  I'm also surprised that you added separate tests for quirks and strict mode.  We don't typically have tests for both unless we expect that there is a behavior difference.  We'd have twice the number of tests in our repository otherwise, and most would not be serving a useful purpose.

[Yusuke Sato]

Thanks for the comment.

> The bug is a cross-platform issue, so a Chromium-only Windows-only patch does not address it.

I see. I'll prepare Safari (Mac/Win) patches as well.

> I'm also surprised that you added separate tests for quirks and strict mode.  We don't typically have tests for both unless we expect that there is a behavior difference.

Oops, _strict files are added to the patch by mistake... These files are meant for my private use. Will remove them, sorry.

[David Levin]

Comment on attachment 31709 [details]
font_underlining_patch

r- because there needs to be another patch simply based on the comments in the bug so far by Mark Rowe and Yusuke Sato.

[Jungshik Shin]

Is the Mozilla blacklist comprehensive enough? At least, the native names for gulim and gulimche should be added :

U+AD74 U+B9BC U+CCB4
U+AD74 U+B9BC

along with batang,batangche, dotum,dotumche, gungseo, gungseoche and their native names (see http://mxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#1167 )

[Yusuke Sato]

Created attachment 31903 [details]
font_underlining_patch_2

 LayoutTests/ChangeLog                              |   34 ++++++++++++
 .../float-in-float-hit-testing-expected.checksum   |    2 +-
 .../float/float-in-float-hit-testing-expected.png  |  Bin 34637 -> 34652 bytes
 .../block/positioning/auto/007-expected.checksum   |    2 +-
 .../fast/block/positioning/auto/007-expected.png   |  Bin 34855 -> 34836 bytes
 .../dom/clone-node-dynamic-style-expected.checksum |    2 +-
 .../fast/dom/clone-node-dynamic-style-expected.png |  Bin 41001 -> 41004 bytes
 .../shadow-multiple-horizontal-expected.checksum   |    2 +-
 .../shadow-multiple-horizontal-expected.png        |  Bin 56799 -> 56492 bytes
 .../shadow-multiple-vertical-expected.checksum     |    2 +-
 .../repaint/shadow-multiple-vertical-expected.png  |  Bin 56797 -> 56491 bytes
 .../rtl-white-space-pre-wrap-expected.checksum     |    2 +-
 .../rtl-white-space-pre-wrap-expected.png          |  Bin 39695 -> 39668 bytes
 .../mac/fast/text/line-breaks-expected.checksum    |    2 +-
 .../mac/fast/text/line-breaks-expected.png         |  Bin 29790 -> 29962 bytes
 .../text/stroking-decorations-expected.checksum    |    2 +-
 .../fast/text/stroking-decorations-expected.png    |  Bin 139991 -> 137834 bytes
 .../text/trailing-white-space-2-expected.checksum  |    2 +-
 .../fast/text/trailing-white-space-2-expected.png  |  Bin 47615 -> 47982 bytes
 .../text/trailing-white-space-expected.checksum    |    2 +-
 .../fast/text/trailing-white-space-expected.png    |  Bin 46871 -> 47228 bytes
 ...vg-file-image-repaint-problem-expected.checksum |    2 +-
 ...ded-svg-file-image-repaint-problem-expected.png |  Bin 135974 -> 136063 bytes
 .../tables/mozilla/bugs/bug10633-expected.checksum |    2 +-
 .../mac/tables/mozilla/bugs/bug10633-expected.png  |  Bin 28748 -> 28749 bytes
 .../mozilla/bugs/bug126742-expected.checksum       |    2 +-
 .../mac/tables/mozilla/bugs/bug126742-expected.png |  Bin 28462 -> 28531 bytes
 WebCore/ChangeLog                                  |   50 +++++++++++++++++
 WebCore/platform/graphics/Font.cpp                 |   20 +++++++
 WebCore/platform/graphics/Font.h                   |    2 +
 WebCore/platform/graphics/FontFastPath.cpp         |   34 ++++++++++++
 WebCore/platform/graphics/SimpleFontData.cpp       |    4 +-
 WebCore/platform/graphics/SimpleFontData.h         |    4 ++
 .../chromium/SimpleFontDataChromiumWin.cpp         |   13 +++++
 WebCore/platform/graphics/mac/SimpleFontDataMac.mm |   13 +++++
 .../graphics/opentype/OpenTypeUtilities.cpp        |   57 ++++++++++++++++++++
 .../platform/graphics/opentype/OpenTypeUtilities.h |    4 ++
 .../platform/graphics/win/SimpleFontDataWin.cpp    |   10 ++++
 WebCore/rendering/InlineFlowBox.cpp                |   35 +++++++++++-
 WebCore/rendering/InlineTextBox.cpp                |   57 +++++++++++---------
 WebCore/rendering/InlineTextBox.h                  |    5 ++-
 41 files changed, 324 insertions(+), 44 deletions(-)

[Yusuke Sato]

Oops, I missed Jungshik's comment.
Please ignore the attachment I've just added. I'll upload a new patch later.

[Yusuke Sato]

Created attachment 31915 [details]
font_underlining_patch_3

 LayoutTests/ChangeLog                              |   34 +++++++++
 .../float-in-float-hit-testing-expected.checksum   |    2 +-
 .../float/float-in-float-hit-testing-expected.png  |  Bin 34637 -> 34652 bytes
 .../block/positioning/auto/007-expected.checksum   |    2 +-
 .../fast/block/positioning/auto/007-expected.png   |  Bin 34855 -> 34836 bytes
 .../dom/clone-node-dynamic-style-expected.checksum |    2 +-
 .../fast/dom/clone-node-dynamic-style-expected.png |  Bin 41001 -> 41004 bytes
 .../shadow-multiple-horizontal-expected.checksum   |    2 +-
 .../shadow-multiple-horizontal-expected.png        |  Bin 56799 -> 56492 bytes
 .../shadow-multiple-vertical-expected.checksum     |    2 +-
 .../repaint/shadow-multiple-vertical-expected.png  |  Bin 56797 -> 56491 bytes
 .../rtl-white-space-pre-wrap-expected.checksum     |    2 +-
 .../rtl-white-space-pre-wrap-expected.png          |  Bin 39695 -> 39668 bytes
 .../mac/fast/text/line-breaks-expected.checksum    |    2 +-
 .../mac/fast/text/line-breaks-expected.png         |  Bin 29790 -> 29962 bytes
 .../text/stroking-decorations-expected.checksum    |    2 +-
 .../fast/text/stroking-decorations-expected.png    |  Bin 139991 -> 137834 bytes
 .../text/trailing-white-space-2-expected.checksum  |    2 +-
 .../fast/text/trailing-white-space-2-expected.png  |  Bin 47615 -> 47982 bytes
 .../text/trailing-white-space-expected.checksum    |    2 +-
 .../fast/text/trailing-white-space-expected.png    |  Bin 46871 -> 47228 bytes
 ...vg-file-image-repaint-problem-expected.checksum |    2 +-
 ...ded-svg-file-image-repaint-problem-expected.png |  Bin 135974 -> 136063 bytes
 .../tables/mozilla/bugs/bug10633-expected.checksum |    2 +-
 .../mac/tables/mozilla/bugs/bug10633-expected.png  |  Bin 28748 -> 28749 bytes
 .../mozilla/bugs/bug126742-expected.checksum       |    2 +-
 .../mac/tables/mozilla/bugs/bug126742-expected.png |  Bin 28462 -> 28531 bytes
 WebCore/ChangeLog                                  |   50 ++++++++++++++
 WebCore/platform/graphics/Font.cpp                 |   20 ++++++
 WebCore/platform/graphics/Font.h                   |    2 +
 WebCore/platform/graphics/FontFastPath.cpp         |   34 +++++++++
 WebCore/platform/graphics/SimpleFontData.cpp       |    4 +-
 WebCore/platform/graphics/SimpleFontData.h         |    4 +
 .../chromium/SimpleFontDataChromiumWin.cpp         |   13 ++++
 WebCore/platform/graphics/mac/SimpleFontDataMac.mm |   13 ++++
 .../graphics/opentype/OpenTypeUtilities.cpp        |   72 ++++++++++++++++++++
 .../platform/graphics/opentype/OpenTypeUtilities.h |    4 +
 .../platform/graphics/win/SimpleFontDataWin.cpp    |   10 +++
 WebCore/rendering/InlineFlowBox.cpp                |   35 +++++++++-
 WebCore/rendering/InlineTextBox.cpp                |   57 +++++++++-------
 WebCore/rendering/InlineTextBox.h                  |    5 +-
 41 files changed, 339 insertions(+), 44 deletions(-)

[Mark Rowe (bdash)]

Is it really necessary to post 40+ lines of diffstat output every time you attach a patch?  It's useless.  We can see what files changed from the patch!

[Eric Seidel (no email)]

He's using git-send-bugzilla I bet.  Which the default version uses --stat instead of --shortstat.

We should update our docs to tell people to use:
bugzilla-tool post-commits
instead.  Since it's a drop-in replacement for git-send-bugzilla these days.

[Yusuke Sato]

- Added Safari (for Mac and Windows) support.
- Added PNG files for pixel tests.
- Removed my layout tests since underlining is tested thoroughly by various existing tests.
- Modified hasBadUnderlineMetrics function following Jungshik's comment.

Please take another look.


Mark, Eric,

Yes, I'm using git-send-bugzilla.
I'll try the new tool from next time. Thanks for the suggestion.

--Yusuke

[Eric Seidel (no email)]

Comment on attachment 31915 [details]
font_underlining_patch_3

The ChangeLog detail is very helpful, thank you.

Is it possible to do this in smaller pieces?  That would make review much easier.

I dont' think we generally use L"" constants.  Maybe we do and I just don't realize.

 413         // This list is from Firefox.
Depending on the license, we may not be able to just copy the code...

Indent:
 470     if (!fontNameSet) {
 471       fontNameSet = new HashSet<String>;
 472       size_t numElements = sizeof(badFonts) / sizeof(badFonts[0]);
 473       for (size_t i = 0; i < numElements; ++i)
 474           fontNameSet->add(badFonts[i]);
 475     }

 58 bool hasBadUnderlineMetrics(const String& name);
That should be fontName, not just "name", or is it a fontFamily?  If so, we tend to use AtomicString for font family names, at least that how they're stored on FontDescription.

Seems all the code you added to paintTextDecorations should be a static inline function.  Something like paintUnderline(...)  No need to make paintTextDecorations so much bigger.

Normally we don't use "tmp" in variable names:
 989                             int tmpPosition;
 990                             int tmpThickness;

The extra context save/restores could slow down this code path.

Honestly, this change is rather large, and difficult for me to review, so since it's languishing in the queue I'm sorta looking for reasons to r- it.  You'll have more luck with smaller changes.  Also I think you may in the en need to involve Dave Hyatt or Dan Bernstien in the final review.

[Yusuke Sato]

Thanks for the review, Eric! I'll try to split the patch following your suggestion.

-- Yusuke

[Yusuke Sato]

Created attachment 38786 [details]
get_underline_metrics_interface_v1


---
 3 files changed, 22 insertions(+), 1 deletions(-)

[Yusuke Sato]

Created attachment 38787 [details]
get_underline_metrics_mac_impl_v1


---
 2 files changed, 23 insertions(+), 0 deletions(-)

[Yusuke Sato]

Created attachment 38788 [details]
calc_underline_metrics_for_a_textrun_v1


---
 4 files changed, 71 insertions(+), 0 deletions(-)

[Yusuke Sato]

Created attachment 38789 [details]
draw_underline_v1


---
 5 files changed, 72 insertions(+), 9 deletions(-)

[Yusuke Sato]

Created attachment 38790 [details]
underline_rebaseline_v1


---
 31 files changed, 56 insertions(+), 15 deletions(-)

[Yusuke Sato]

> Is it possible to do this in smaller pieces?  That would make review much
> easier.

I've split the previous change into 5 pieces. Could you take another look?

 1. get_underline_metrics_interface_v1
 2. get_underline_metrics_mac_impl_v1 (depends on 1. Safari Mac implementation of 1.)
 3. calc_underline_metrics_for_a_textrun_v1 (depends on 1.)
 4. draw_underline_v1 (depends on 3.)
 5. underline_rebaseline_v1 (rebaselined pixel tests)

I've dropped Safari Win and Chromium Win implementations from the changes above this time for simplicity. After having submitted the changes, I'll ask you to review these extra implementations.

> 413         // This list is from Firefox.
> Depending on the license, we may not be able to just copy the code...

I read the document of Mozilla (http://kb.mozillazine.org/Font.blacklist.underline_offset) and reused the list of blacklisted font family names, but didn't copy the code. So I guess there's no license issue.

That said, this time I've dropped changes against OpenTypeUtilities.cpp (for simplicity, either), since the hack I added to the OpenTypeUtilities seems not to be indispensable for most Safari/Mac users. I'll add the blacklisting hack later if necessary.

> Seems all the code you added to paintTextDecorations should be a static inline
> function.  Something like paintUnderline(...)  No need to make
> paintTextDecorations so much bigger.

Done.

> Normally we don't use "tmp" in variable names:
>  989                             int tmpPosition;
>  990                             int tmpThickness;

Fixed.

> The extra context save/restores could slow down this code path.

Done. Removed all saves and restores. I think the change list #4 is now fairly small.

[Eric Seidel (no email)]

Comment on attachment 38786 [details]
get_underline_metrics_interface_v1

Looks fine.  We can set cq+ when the rest are approved.

[Eric Seidel (no email)]

Comment on attachment 38787 [details]
get_underline_metrics_mac_impl_v1

Looks sane to me, can be set to cq+ once the rest are reviewed.

[Eric Seidel (no email)]

Comment on attachment 38788 [details]
calc_underline_metrics_for_a_textrun_v1

I wonder how much of a perf hit this will be?  (if any)

I'm not sure I understand this logic:
 275         if (!result || *position <= fontData->underlinePosition()) {

why we're looking for the largest position.  I guess thats' the lowest underline?  Because some characters might need a lower underline than others?  Seems strange that this could mean that if 'g' has a lower underline than all other characters that we'd end up with discontigous underlines for something like:

<u>g<s>b</s>h</u>

I would expect there to be a single underline along the whole bottom of that, but if 'g' has a different underline position than b and h it won't.  Maybe that's just an error in the font?

[Eric Seidel (no email)]

Comment on attachment 38790 [details]
underline_rebaseline_v1

If this was an svn-create-patch instead of a git patch, we could actually see the image files for comparison. :(  Since I can't actually make intelligent comments about this one, r-.  rubber stamping is the only other option available to me. :)

[Eric Seidel (no email)]

You may need to track down Hyatt or Mitz via irc or email to get final review of the two remaining r=? patches.

[Eric Seidel (no email)]

Did you have any luck in tracking down mitz or hyatt via IRC?  Do we know if either of them approve of this type of change?

[Yusuke Sato]

Created attachment 40714 [details]
underline_rebaseline_v2

[Yusuke Sato]

Sorry for the very late reply, and thanks for the review.

> why we're looking for the largest position.  I guess thats' the lowest
> underline?  Because some characters might need a lower underline than others? 
> Seems strange that this could mean that if 'g' has a lower underline than all
> other characters that we'd end up with discontigous underlines for something
> like:

It searches for the lowest underline position because a single TextRun might contain characters for two or more languages (e.g. "aあ") and in such case, the fonts (e.g. for "a" and for "あ") might have different underline positions.

> <u>g<s>b</s>h</u>
>
> I would expect there to be a single underline along the whole bottom of that,
> but if 'g' has a different underline position than b and h it won't.  Maybe
> that's just an error in the font?

However, in the example above, the underline can't be discontiguous since "g", "b", and "h" are all English and therefore are rendered with a single font.
(Please check the rebaselined version of platform/mac/fast/dom/clone-node-dynamic-style-expected.png. The test contains similar markups. You can see the underlines in the png are contiguous.)

If the example is something like this: 
    <u>a sentence in lang A <s>a sentence in lang B</s></u>
the underline could be discontiguous (depending on the font metrics and the font size), but I'd guess it's rare.

> If this was an svn-create-patch instead of a git patch, we could actually see
> the image files for comparison. :(  Since I can't actually make intelligent
> comments about this one, r-.  rubber stamping is the only other option
> available to me. :)

I've prepared a new patch. Thanks.

>  Did you have any luck in tracking down mitz or hyatt via IRC?

I haven't had cycles to work on this patch these days... I'm going to ask them tomorrow.

Thanks,
Yusuke

Comment on attachment 38787 [details]
get_underline_metrics_mac_impl_v1

> +#ifndef BUILDING_ON_TIGER
> +    float fUnderlinePosition = fAscent - CTFontGetUnderlinePosition(toCTFontRef(m_platformData.font()));
> +    float fUnderlineThickness = CTFontGetUnderlineThickness(toCTFontRef(m_platformData.font()));
> +#endif

I think you need to null-check m_platformData.font() here. But it is also unnecessary to go through Core Text (and thus exclude Tiger), because -[NSFont underlinePosition] and -[NSFont underlineThickness] are available in Mac OS X v10.0 and later.

> +#ifndef BUILDING_ON_TIGER
> +    m_underlinePosition = std::max(lroundf(fUnderlinePosition), m_ascent + 1L);
> +    m_underlineThickness = std::max(lroundf(fUnderlineThickness), 1L);
> +    // Note that we use the default (hard coded) position/thickness on Tiger.
> +    // FIXME: whould be better to update WebKitLibraries/ so that we can get proper metrics on Tiger?
> +#endif

If you use the aforementioned NSFont methods you won’t need to make any changes for Tiger.

Instead of using 'std::max' you should add 'using namespace std;' near the beginning of the file and just use 'max' here. I don’t think you need the #import <algorithm> statement in this file.

There is no need to for the "1L" literal form. You can use roundf() and max<int>().

I am curious how you arrived at the formulas for m_underlinePosition and m_underlineThickness. Do they match what AppKit does on Mac OS X? In particular I don’t think AppKit derives the underline position from the font’s -underlinePosition.

> I am curious how you arrived at the formulas for m_underlinePosition and
> m_underlineThickness. Do they match what AppKit does on Mac OS X? In particular
> I don’t think AppKit derives the underline position from the font’s
> -underlinePosition.

For example, looking at the expected image for fast/text/international/rtl-white-space-pre-wrap.html in attachment 40714 [details], the underline is 1px thick, but when I type the same text in TextEdit in the same font and size, I get a 2px-thick underline.

Should this apply to other text decorations? AppKit scales up strikethrough lines as well.

Further comments:

1. How do you ensure that underlines are accounted for in repainting? If you use -underlinePosition or its equivalent, is there a guarantee that the underline will not go below the descender? It it does (and you don’t change that behavior; I think the AppKit behavior is to keep it within the descender), then you should add it to visual overflow.

2. This runs the width iterator an extra time on every underlined text run at paint time. It would be better to avoid that, because I think it may impact performance. Perhaps following a model similar to Font::floatWidth()’s fallbackFonts parameter (and utilizing the existing logic in all code paths that support it) would be better. Then you could just have one place that goes over the list of fonts used in the text run, instead of iterating over the entire glyph buffer.

3. I have mentioned that AppKit behavior on Mac OS X a few times. The reason is that AppKit text views are used alongside WebViews on Mac OS X, and it would be good to have consistency between the two. It would be unfortunate to change WebKit to match AppKit “in spirit” without getting the details right.

[Yusuke Sato]

Created attachment 40781 [details]
get_underline_metrics_mac_impl_v2


---
 2 files changed, 21 insertions(+), 0 deletions(-)

[Yusuke Sato]

Created attachment 40782 [details]
calc_underline_metrics_for_a_textrun_v2


---
 4 files changed, 72 insertions(+), 0 deletions(-)

[Yusuke Sato]

Created attachment 40783 [details]
underline_rebaseline_v3

[Yusuke Sato]

mitz, thanks for the review!

I've modified the get_underline_metrics_mac_impl patch as follows:

- Stopped using CoreText APIs. Replaced them with NSFont calls.
- Removed the #ifdefs for Tiger.
- Fixed styles following your comment.


For the calc_underline_metrics_for_a_textrun patch, I did the following:

> 1. How do you ensure that underlines are accounted for in repainting?

Added a few lines of code which keeps the underline within the descender.

> 2. This runs the width iterator an extra time on every underlined text run at ...

Modified Font::underlineMetricsForSimpleText(). Now the function scans a run only once. Thanks! 

Please take another look.


> Should this apply to other text decorations? AppKit scales up strikethrough lines as well.

I agree that it'd be better to apply this kind of change to strike-through lines, but I'd like to finish the underline issue first.

>  In particular I don’t think AppKit derives the underline position from the font’s -underlinePosition.
...
> 3. I have mentioned that AppKit behavior on Mac OS X a few times. The reason is
> that AppKit text views are used alongside WebViews on Mac OS X, and it would be
> good to have consistency between the two.

So you're saying that just using NSFont methods is not perfect, right? If so, could you let me know if there is a document/code/... of AppKit which I could use as a reference to emulate the AppKit's behavior?
(Sorry if it's a silly question. I'm not so familiar with Mac programming...)

[Adam Barth]

In the future, can we use the one-patch-per-bug process?  This bug is out of control and incomprehensible.

[Yusuke Sato]

Yeah, I'll do so from next time, whenever possible.
BTW, mitz, don't you have time to review the new patches?

Created attachment 41751 [details]
Underlined Times 72px with the patch, compared to TextEdit

I applied the patches to TOT WebKit and built on Mac OS X v10.6, then I tested it with 72px Times. Both the position and the thickness of the underline did not match TextEdit (see screenshot). Like I said, it would be a shame to change this and still not match the AppKit rendering.

Comment on attachment 40781 [details]
get_underline_metrics_mac_impl_v2

r- based on comment #40. I think it’s very desirable to get this to match AppKit.

[Mark Rowe (bdash)]

There are three different patches marked for review on this bug and it’s impossible to tell how they’re related.

[Adam Barth]

Please divide this bug into sensible pieces.  I have no idea what the status of this bug is.

[Yusuke Sato]

Cleared all r? flags. Sorry for the mess.

At the moment, I have no idea how I can emulate the AppKit behavior.

[Mark Rowe (bdash)]

Why did this get closed?  It is clearly not fixed yet.

<rdar://problem/8037569>

[Jungshik Shin]

Would it be OK to fix this issue on Windows (including Safari) and Linux before addressing AppKit compatibility issue on Mac (if Appkit compatibility issue is blocking Mac change)?

[Myles C. Maxfield]

Created attachment 223908 [details]
Patch

[Dean Jackson]

Comment on attachment 223908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223908&action=review

> Source/WebCore/ChangeLog:8
> +        This patch simply triggers the previous iOS codepath for underline drawing code.

Maybe this should say: This patch adopts the iOS codepath for underlines on OS X.

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-thickness.html:8
> +#t {
> +    text-decoration: underline;
> +    font-size: 10000px;
> +}

Why not just put this in the style attribute?

> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-thickness.html:16
> +<div style="position: absolute; left: -3000px; top: -9650px;" id="t">T</div>

You should use Ahem font so that you can be sure the metrics are consistent.

[Myles C. Maxfield]

Comment on attachment 223908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223908&action=review

>> Source/WebCore/ChangeLog:8
>> +        This patch simply triggers the previous iOS codepath for underline drawing code.
> 
> Maybe this should say: This patch adopts the iOS codepath for underlines on OS X.

Done.

>> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-thickness.html:8
>> +}
> 
> Why not just put this in the style attribute?

Done.

>> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-thickness.html:16
>> +<div style="position: absolute; left: -3000px; top: -9650px;" id="t">T</div>
> 
> You should use Ahem font so that you can be sure the metrics are consistent.

Done.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/163921