Bug 76197 - [CSS Line Grid] Implement baseline grid alignment
: [CSS Line Grid] Implement baseline grid alignment
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-01-12 12:00 PST by
Modified: 2012-01-17 11:16 PST (History)


Attachments
Patch (305.28 KB, patch)
2012-01-12 12:05 PST, Dave Hyatt
webkit.review.bot: commit‑queue-
Review Patch | 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-
Review Patch | 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-
Review Patch | 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-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-12 12:00:52 PST
Implementation of baseline grid alignment.
------- Comment #1 From 2012-01-12 12:05:25 PST -------
Created an attachment (id=122282) [details]
Patch
------- Comment #2 From 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 From 2012-01-12 12:58:10 PST -------
(From update of attachment 122282 [details])
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 From 2012-01-12 13:03:48 PST -------
Created an attachment (id=122294) [details]
Patch that should pass stylebot.
------- Comment #5 From 2012-01-12 13:41:28 PST -------
(From update of attachment 122294 [details])
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 From 2012-01-12 15:25:39 PST -------
Created an attachment (id=122321) [details]
Fix failing tests. Used the old webkit tests to generate.
------- Comment #7 From 2012-01-12 16:17:48 PST -------
(From update of attachment 122321 [details])
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 From 2012-01-13 09:20:21 PST -------
Created an attachment (id=122438) [details]
Fix the SVG test failure
------- Comment #9 From 2012-01-13 10:07:20 PST -------
(From update of attachment 122438 [details])
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 From 2012-01-16 11:35:59 PST -------
Created an attachment (id=122667) [details]
Patch
------- Comment #11 From 2012-01-16 14:55:40 PST -------
(From update of attachment 122667 [details])
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 From 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 From 2012-01-17 00:12:01 PST -------
(From update of attachment 122667 [details])
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 From 2012-01-17 11:16:52 PST -------
Fixed in r105176.