Bug 76197 - [CSS Line Grid] Implement baseline grid alignment
Summary: [CSS Line Grid] Implement baseline grid alignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-12 12:00 PST by Dave Hyatt
Modified: 2012-01-17 11:16 PST (History)
3 users (show)

See Also:


Attachments
Patch (305.28 KB, patch)
2012-01-12 12:05 PST, Dave Hyatt
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch that should pass stylebot. (304.34 KB, patch)
2012-01-12 13:03 PST, Dave Hyatt
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix failing tests. Used the old webkit tests to generate. (305.47 KB, patch)
2012-01-12 15:25 PST, Dave Hyatt
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix the SVG test failure (305.49 KB, patch)
2012-01-13 09:20 PST, Dave Hyatt
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (305.78 KB, patch)
2012-01-16 11:35 PST, Dave Hyatt
simon.fraser: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2012-01-12 12:00:52 PST
Implementation of baseline grid alignment.
Comment 1 Dave Hyatt 2012-01-12 12:05:25 PST
Created attachment 122282 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-12 12:08:44 PST
Attachment 122282 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlock.h:243:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2012-01-12 12:58:10 PST
Comment on attachment 122282 [details]
Patch

Attachment 122282 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11227166

New failing tests:
fast/line-grid/line-grid-inside-columns.html
fast/line-grid/line-grid-floating.html
svg/clip-path/clip-path-text-and-stroke.svg
fast/line-grid/line-grid-positioned.html
fast/line-grid/line-grid-into-floats.html
fast/block/float/float-not-removed-from-next-sibling4.html
Comment 4 Dave Hyatt 2012-01-12 13:03:48 PST
Created attachment 122294 [details]
Patch that should pass stylebot.
Comment 5 WebKit Review Bot 2012-01-12 13:41:28 PST
Comment on attachment 122294 [details]
Patch that should pass stylebot.

Attachment 122294 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11228197

New failing tests:
fast/line-grid/line-grid-inside-columns.html
fast/line-grid/line-grid-floating.html
svg/clip-path/clip-path-text-and-stroke.svg
fast/line-grid/line-grid-positioned.html
fast/line-grid/line-grid-into-floats.html
fast/block/float/float-not-removed-from-next-sibling4.html
Comment 6 Dave Hyatt 2012-01-12 15:25:39 PST
Created attachment 122321 [details]
Fix failing tests. Used the old webkit tests to generate.
Comment 7 WebKit Review Bot 2012-01-12 16:17:48 PST
Comment on attachment 122321 [details]
Fix failing tests. Used the old webkit tests to generate.

Attachment 122321 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11152273

New failing tests:
fast/line-grid/line-grid-floating.html
fast/line-grid/line-grid-into-floats.html
fast/line-grid/line-grid-inside-columns.html
svg/clip-path/clip-path-text-and-stroke.svg
fast/line-grid/line-grid-positioned.html
Comment 8 Dave Hyatt 2012-01-13 09:20:21 PST
Created attachment 122438 [details]
Fix the SVG test failure
Comment 9 WebKit Review Bot 2012-01-13 10:07:20 PST
Comment on attachment 122438 [details]
Fix the SVG test failure

Attachment 122438 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11236083

New failing tests:
fast/line-grid/line-grid-floating.html
fast/line-grid/line-grid-into-floats.html
fast/line-grid/line-grid-inside-columns.html
fast/line-grid/line-grid-positioned.html
Comment 10 Dave Hyatt 2012-01-16 11:35:59 PST
Created attachment 122667 [details]
Patch
Comment 11 Simon Fraser (smfr) 2012-01-16 14:55:40 PST
Comment on attachment 122667 [details]
Patch

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

> Source/WebCore/rendering/LayoutState.cpp:234
> +        for (LayoutState* currentState = m_next; currentState; currentState = currentState->m_next) {
> +            if (currentState->m_currentLineGrid == currentGrid)
> +                continue;
> +            currentGrid = currentState->m_currentLineGrid;
> +            if (currentGrid->style()->lineGrid() == block->style()->lineGrid()) {
> +                m_currentLineGrid = currentGrid;
> +                m_currentLineGridOffset = currentState->m_currentLineGridOffset;
> +                return;

Would this make sense in its own findEnclosingLineGrid() method, for readability?

> Source/WebCore/rendering/LayoutState.h:123
> +    // The current line grid that we're snapping to and the offset of the start of the grid.
> +    RenderBlock* m_currentLineGrid;
> +    LayoutSize m_currentLineGridOffset;

LayoutState was originally added just as an optimization (and, I think it still serves that purpose?). This new code makes LayoutState something that alters how layout behaves, which I don't really like. I'd prefer that the "current grid" stuff be tracked outside of LayoutState.

Maybe this ship already sailed with isPaginated(). What if something inside a transform (where LayoutState is disabled) establishes a local line grid?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2785
> +    // FIXME: If any of the characteristics of the box change compared to the old one, then we need to do a deep dirtying
> +    // (similar to what happens when the page height changes). Ideally, though, we only do this if someone is actually snapping
> +    // to this grid.

File a bug to track this?

> LayoutTests/ChangeLog:16
> +        * platform/mac/fast/line-grid/line-grid-inside-columns-expected.png: Added.

This result needs to be regenerated. We have mock scrollbars on now.

> LayoutTests/fast/line-grid/line-grid-floating.html:7
> +.grid { -webkit-line-grid: simple; -webkit-line-grid-snap: baseline; 
> +        font-size:36px; float:left;border:2px solid black;
> +        padding:10px; margin:5px }

Can you unwrap these rules pls?
Comment 12 Dave Hyatt 2012-01-16 15:02:15 PST
Responding to your LayoutState concerns, I think the right way to clean it up is to get rid of the generic enable/disable terminology in favor of better-named methods that indicate exactly what is being turned on and off. I think it's ok that LayoutState evolve into the "stack of stuff" that you need to track as you push into layout, but that the generically named enable/disable methods need to be refined to say exactly what they're turning on and off.
Comment 13 WebKit Review Bot 2012-01-17 00:12:01 PST
Comment on attachment 122667 [details]
Patch

Attachment 122667 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11269113

New failing tests:
fast/line-grid/line-grid-floating.html
fast/line-grid/line-grid-into-floats.html
fast/line-grid/line-grid-inside-columns.html
fast/line-grid/line-grid-positioned.html
Comment 14 Dave Hyatt 2012-01-17 11:16:52 PST
Fixed in r105176.