Bug 48537 - Fonts with no vertical metrics should synthesize baselines
Summary: Fonts with no vertical metrics should synthesize baselines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on:
Blocks: 46123
  Show dependency treegraph
 
Reported: 2010-10-28 11:45 PDT by Dave Hyatt
Modified: 2010-11-17 22:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (256.24 KB, patch)
2010-11-17 02:49 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (256.01 KB, patch)
2010-11-17 09:16 PST, Dave Hyatt
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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'])