Bug 48537

Summary: Fonts with no vertical metrics should synthesize baselines
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, eric, mrobinson, rniwa, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 46123    
Attachments:
Description Flags
Patch
none
Patch sam: review+

Description Dave Hyatt 2010-10-28 11:45:59 PDT
Fonts with no vertical metrics should synthesize baselines when they appear on lines that do use fonts with vertical metrics.  Basically we want to make sure that English text behaves like vertical-align:middle, i.e., its baseline should be treated as though it cuts through the middle of the ascent+descent.
Comment 1 Adele Peterson 2010-10-29 15:12:20 PDT
<rdar://problem/8612034>
Comment 2 Dave Hyatt 2010-11-17 02:49:09 PST
Created attachment 74096 [details]
Patch
Comment 3 Eric Seidel (no email) 2010-11-17 03:05:06 PST
Attachment 74096 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6238014
Comment 4 Early Warning System Bot 2010-11-17 03:06:41 PST
Attachment 74096 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6177018
Comment 5 Build Bot 2010-11-17 03:22:58 PST
Attachment 74096 [details] did not build on win:
Build output: http://queues.webkit.org/results/6165018
Comment 6 Dave Hyatt 2010-11-17 09:16:29 PST
Created attachment 74124 [details]
Patch
Comment 7 Sam Weinig 2010-11-17 11:45:36 PST
Comment on attachment 74124 [details]
Patch

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

> WebCore/rendering/InlineFlowBox.cpp:484
> +            if (verticalAlign == SUB)
> +                verticalPosition += fontSize / 5 + 1;
> +            else if (verticalAlign == SUPER)
> +                verticalPosition -= fontSize / 3 + 1;
> +            else if (verticalAlign == TEXT_TOP)
> +                verticalPosition += renderer->baselinePosition(baselineType, firstLine, lineDirection) - font.ascent(baselineType);
> +            else if (verticalAlign == MIDDLE)
> +                verticalPosition += -static_cast<int>(font.xHeight() / 2) - renderer->lineHeight(firstLine, lineDirection) / 2 + renderer->baselinePosition(baselineType, firstLine, lineDirection);
> +            else if (verticalAlign == TEXT_BOTTOM) {
> +                verticalPosition += font.descent(baselineType);
> +                // lineHeight - baselinePosition is always 0 for replaced elements (except inline blocks), so don't bother wasting time in that case.
> +                if (!renderer->isReplaced() || renderer->isInlineBlockOrInlineTable())
> +                    verticalPosition -= (renderer->lineHeight(firstLine, lineDirection) - renderer->baselinePosition(baselineType, firstLine, lineDirection));
> +            } else if (verticalAlign == BASELINE_MIDDLE)
> +                verticalPosition += -renderer->lineHeight(firstLine, lineDirection) / 2 + renderer->baselinePosition(baselineType, firstLine, lineDirection);
> +            else if (verticalAlign == LENGTH)
> +                verticalPosition -= renderer->style()->verticalAlignLength().calcValue(renderer->lineHeight(firstLine, lineDirection));
> +        }

If you wanted, you could change this to a switch statement.
Comment 8 Dave Hyatt 2010-11-17 12:56:20 PST
Fixed in r72235.
Comment 9 WebKit Review Bot 2010-11-17 14:07:55 PST
http://trac.webkit.org/changeset/72235 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/blockflow/vertical-font-fallback.html
fast/repaint/repaint-across-writing-mode-boundary.html
Comment 10 Ryosuke Niwa 2010-11-17 20:00:07 PST
It seems like this change requires rebaselines for the following tests on GTK:
fast/css/line-height-determined-by-primary-font.html
fast/css/negative-leading.html

+mrobinson for GTK rebaseline.
Comment 11 Eric Seidel (no email) 2010-11-17 22:26:07 PST
This broke snow leopard tests, and thus the commit-queue:

SUCCESS: Build 20999 (r72235) was the first to show failures: set([u'fast/blockflow/vertical-font-fallback.html', u'fast/repaint/repaint-across-writing-mode-boundary.html'])