Bug 76197 – [CSS Line Grid] Implement baseline grid alignment

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Implementation of baseline grid alignment.

[Dave Hyatt]

Created attachment 122282 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Dave Hyatt]

Created attachment 122294 [details]
Patch that should pass stylebot.

[WebKit Review Bot]

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

[Dave Hyatt]

Created attachment 122321 [details]
Fix failing tests. Used the old webkit tests to generate.

[WebKit Review Bot]

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

[Dave Hyatt]

Created attachment 122438 [details]
Fix the SVG test failure

[WebKit Review Bot]

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

[Dave Hyatt]

Created attachment 122667 [details]
Patch

[Simon Fraser (smfr)]

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?

[Dave Hyatt]

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.

[WebKit Review Bot]

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

[Dave Hyatt]

Fixed in r105176.