Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
373503002:
Add first letter test when first letter is inside a floated block. (Closed)
|
Created:
6 years, 5 months ago by dsinclair Modified:
6 years, 5 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionAdd first letter test when first letter is inside a floated block.
This CL adds a test for the RenderBlock::computePreferredLogicalWidths
call to updateFirstLetter. In the case where we don't call
updateFirstLetter we will incorrectly draw the word Foo flowing out
of the enclosing border.
BUG=391288
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178584
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase to master #Patch Set 3 : Rebase; Remove list item change #Patch Set 4 : git cl web #Patch Set 5 : Use ref test instead. #
Messages
Total messages: 18 (0 generated)
PTAL. Suggestions on how I can add layout tests to trigger this code?
Elliot is the correct reviewer. He's been in the trenches of first-letter most recently.
I don't think this is right, you broke first letter in floats. https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBlock.cpp (left): https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBlock.cpp:3589: updateFirstLetter(); If you don't call updateFirstLetter() when computing the preferred widths then the width of a floated block (or one with intrinsic widths like min-content) will be wrong. It's sad we don't have test coverage.
On 2014/07/14 at 22:55:01, esprehn wrote: > I don't think this is right, you broke first letter in floats. > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > File Source/core/rendering/RenderBlock.cpp (left): > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > Source/core/rendering/RenderBlock.cpp:3589: updateFirstLetter(); > If you don't call updateFirstLetter() when computing the preferred widths then the width of a floated block (or one with intrinsic widths like min-content) will be wrong. It's sad we don't have test coverage. Cool, I'll try to create a layout test for this tomorrow. Please let me know if you can think of other things I've busted. I'm hoping to get better layout test coverage before moving the update first letter out of layout so I'll have some idea that it's working.
On 2014/07/15 at 01:03:59, dsinclair wrote: > On 2014/07/14 at 22:55:01, esprehn wrote: > > I don't think this is right, you broke first letter in floats. > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > File Source/core/rendering/RenderBlock.cpp (left): > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > Source/core/rendering/RenderBlock.cpp:3589: updateFirstLetter(); > > If you don't call updateFirstLetter() when computing the preferred widths then the width of a floated block (or one with intrinsic widths like min-content) will be wrong. It's sad we don't have test coverage. > > > Cool, I'll try to create a layout test for this tomorrow. Please let me know if you can think of other things I've busted. I'm hoping to get better layout test coverage before moving the update first letter out of layout so I'll have some idea that it's working. Anything that depends on preferred width, so off the top of my head: floats, inline-block, absolute position, flex: 1 auto, probably grid layout. As to the RenderListItem change, I'm not even sure how first-letter can affect the list marker. Did you look back at the history to see why it was added in the first place?
On 2014/07/15 03:04:30, ojan-only-code-yellow-reviews wrote: > On 2014/07/15 at 01:03:59, dsinclair wrote: > > On 2014/07/14 at 22:55:01, esprehn wrote: > > > I don't think this is right, you broke first letter in floats. > > > > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > > File Source/core/rendering/RenderBlock.cpp (left): > > > > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > > Source/core/rendering/RenderBlock.cpp:3589: updateFirstLetter(); > > > If you don't call updateFirstLetter() when computing the preferred widths > then the width of a floated block (or one with intrinsic widths like > min-content) will be wrong. It's sad we don't have test coverage. > > > > > > Cool, I'll try to create a layout test for this tomorrow. Please let me know > if you can think of other things I've busted. I'm hoping to get better layout > test coverage before moving the update first letter out of layout so I'll have > some idea that it's working. > > Anything that depends on preferred width, so off the top of my head: floats, > inline-block, absolute position, flex: 1 auto, probably grid layout. > > As to the RenderListItem change, I'm not even sure how first-letter can affect > the list marker. Did you look back at the history to see why it was added in the > first place? Sorry, forgot to link the CL. Both of these calls were added back in 2007 by hyatt. https://src.chromium.org/viewvc/blink?revision=21093&view=revision Rearchitect calcPrefWidths. The calculation is now done lazily only when minPrefWidth or maxPrefWidth are asked for. The result of the calculation is cached. The new invalidation scheme for pref width invalidation follows the containing block hierarchy and knows to halt at positioned objects, since they cannot influence the size of their containers.
On 2014/07/15 at 13:20:42, dsinclair wrote: > On 2014/07/15 03:04:30, ojan-only-code-yellow-reviews wrote: > > On 2014/07/15 at 01:03:59, dsinclair wrote: > > > On 2014/07/14 at 22:55:01, esprehn wrote: > > > > I don't think this is right, you broke first letter in floats. > > > > > > > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > > > File Source/core/rendering/RenderBlock.cpp (left): > > > > > > > > > > https://codereview.chromium.org/373503002/diff/1/Source/core/rendering/Render... > > > > Source/core/rendering/RenderBlock.cpp:3589: updateFirstLetter(); > > > > If you don't call updateFirstLetter() when computing the preferred widths > > then the width of a floated block (or one with intrinsic widths like > > min-content) will be wrong. It's sad we don't have test coverage. > > > > > > > > > Cool, I'll try to create a layout test for this tomorrow. Please let me know > > if you can think of other things I've busted. I'm hoping to get better layout > > test coverage before moving the update first letter out of layout so I'll have > > some idea that it's working. > > > > Anything that depends on preferred width, so off the top of my head: floats, > > inline-block, absolute position, flex: 1 auto, probably grid layout. > > > > As to the RenderListItem change, I'm not even sure how first-letter can affect > > the list marker. Did you look back at the history to see why it was added in the > > first place? > > > Sorry, forgot to link the CL. Both of these calls were added back in 2007 by hyatt. > > https://src.chromium.org/viewvc/blink?revision=21093&view=revision > > Rearchitect calcPrefWidths. The calculation is now done lazily only when minPrefWidth > or maxPrefWidth are asked for. The result of the calculation is cached. > > The new invalidation scheme for pref width invalidation follows the > containing block hierarchy and knows to halt at positioned objects, since > they cannot influence the size of their containers. So, I added the following to RenderBlock::computePreferredLogicalWidths(): ASSERT((document().lifecycle().state() == DocumentLifecycle::InPerformLayout)); This passes all of the layout tests except for: fast/autoresize/autoresize-with-iframe.html fast/autoresize/basic.html fast/autoresize/turn-off-autoresize.html So, I believe that means, for every other time we called into computePreferredLogicalWidth() we're currently in layout. Inside RenderBlock::layout() we call updateFirstLetter(). Is it correct that we won't call computePreferredLogicalWidths for a child before we've called layout() on that child? If so, I think we just need to figure out how to deal with the autoresize code correctly?
On 2014/07/15 at 17:05:29, dsinclair wrote: > > Sorry, forgot to link the CL. Both of these calls were added back in 2007 by hyatt. > > > > https://src.chromium.org/viewvc/blink?revision=21093&view=revision > > > > Rearchitect calcPrefWidths. The calculation is now done lazily only when minPrefWidth > > or maxPrefWidth are asked for. The result of the calculation is cached. > > > > The new invalidation scheme for pref width invalidation follows the > > containing block hierarchy and knows to halt at positioned objects, since > > they cannot influence the size of their containers. Ugh. OK. Removing the RenderListMarker one looks good to me. > So, I added the following to RenderBlock::computePreferredLogicalWidths(): > > ASSERT((document().lifecycle().state() == DocumentLifecycle::InPerformLayout)); > > This passes all of the layout tests except for: > > fast/autoresize/autoresize-with-iframe.html > fast/autoresize/basic.html > fast/autoresize/turn-off-autoresize.html > > So, I believe that means, for every other time we called into computePreferredLogicalWidth() we're currently in layout. Inside RenderBlock::layout() we call updateFirstLetter(). Is it correct that we won't call computePreferredLogicalWidths for a child before we've called layout() on that child? When we call computePreferredLogicalWidth on a renderer, that call recurses down the subtree. So, the case I think would break is something like: <div style="display: inline-block; border: 3px solid;"> <div>Foo</div> <!-- This div has a first letter styling that makes the F huge. --> </div> The outer div will be too small, i.e. the inner div will overflow outside the border.
lgtm
The CQ bit was unchecked by ojan@chromium.org
On 2014/07/17 at 18:30:03, ojan-only-code-yellow-reviews wrote: > The CQ bit was unchecked by ojan@chromium.org Whoops. Wrong patch.
PTAL. I changed this CL into a test case for the computePreferredLogicalWidths call to updateFirstLetter.
lgtm, you should be able to make this a ref test though by doing <span>F</span>oo and then styling the span to have the huge font.
On 2014/07/21 19:16:17, esprehn wrote: > lgtm, you should be able to make this a ref test though by doing > <span>F</span>oo and then styling the span to have the huge font. Awesome, that's exactly what I wanted. Thanks.
The CQ bit was checked by dsinclair@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/373503002/80001
Message was sent while issue was closed.
Change committed as 178584 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
