Summary: | [CSS Line Grid] Implement baseline grid alignment | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Dave Hyatt <hyatt> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | dglazkov, jdaggett, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Dave Hyatt
2012-01-12 12:00:52 PST
Created attachment 122282 [details]
Patch
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 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 Created attachment 122294 [details]
Patch that should pass stylebot.
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 Created attachment 122321 [details]
Fix failing tests. Used the old webkit tests to generate.
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 Created attachment 122438 [details]
Fix the SVG test failure
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 Created attachment 122667 [details]
Patch
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? 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 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 Fixed in r105176. |