Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2065243003:
[css-sizing] Ensure positive available size on replaced elements
|
Created:
4 years, 6 months ago by jfernandez Modified:
3 years, 6 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-sizing] Ensure positive available size on replaced elements
When computing the preferred widths of replaced elements some values
need to be ignored, since we can't computed them properly before performing
a layout.
We have protected already some cases in r143539 (commit 40beaf69), like
fit-content, fit-available or percentage and calculated values. However, we
still need to protect cases where these content-dependent values are used
on min-width/max-width properties.
BUG=635019
Patch Set 1 #
Total comments: 4
Patch Set 2 : New approach. #Patch Set 3 : 'auto' width should be computed on container's constrained size. #Patch Set 4 : We only need to address the min-width codepath. #Patch Set 5 : Don't mess with max-width. #Patch Set 6 : Address only fit-content and fit-available cases. #Patch Set 7 : A different approach. #
Total comments: 4
Patch Set 8 : Defining a new term 'isLayoutDependent' for Length. #Patch Set 9 : Added some comments for the new Length API function. #Patch Set 10 : Rebaselined test. #Patch Set 11 : Rebaselined test. #Messages
Total messages: 63 (19 generated)
Description was changed from ========== [css-sizing] Ensure positive available size on replaced elements When using fit-content as min-width on replaced elements it's possible to use a negative available size when computing the mentioned intrinsic size. We don't protect against negative available width in case of float containers with replaced element children. This patch ensure the container's width obtained is at least 0px, as we currently do for other cases. BUG=620235 ========== to ========== [css-sizing] Ensure positive available size on replaced elements When using fit-content as min-width on replaced elements it's possible to use a negative available size when computing the mentioned intrinsic size. We don't protect against negative available width in case of float containers with replaced element children. This patch ensure the container's width obtained is at least 0px, as we currently do for other cases. BUG=620235 ==========
jfernandez@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com, svillar@igalia.com
https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2842: const LayoutUnit cw = std::max(LayoutUnit(), isOutOfFlowPositioned() ? containingBlockLogicalWidthForPositioned(toLayoutBoxModelObject(container())) : containingBlockLogicalWidthForContent()); So, why do these functions return a negative value? Wouldn't it be better to fix them not to do that?
https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:2842: const LayoutUnit cw = std::max(LayoutUnit(), isOutOfFlowPositioned() ? containingBlockLogicalWidthForPositioned(toLayoutBoxModelObject(container())) : containingBlockLogicalWidthForContent()); On 2016/06/27 18:43:49, cbiesinger wrote: > So, why do these functions return a negative value? Wouldn't it be better to fix > them not to do that? I though about that, actually. However, I've got lots of doubts on that approach. First of all, these functions are quite old and widely used. Actually, there are similar defensive logic in both, webkit and blink (look at computeLogicalWidth function). My impression is that this logic is far for being homogeneous; while the function containingBlockLogicalWidthForPositioned avoids any negative value on each of its branches, the case of containingBlockLogicalWidthForContent is different on that regard. The negative values comes from the fact that width (border or content box, depending on the box sizing, I guess) can be less than border, padding, margin and scrollbar. Hence, it's quite easy to define cases here the content width is a negative value. In the case of the above mentioned function, it returns either overrideContainingBlockContentLogicalWidth, or availableLogicalWidth or clientLogicalWidth, all of them unprotected against negative values. Additionally, containingBlockLogicalWidthForContent is a virtual method, which is defined as availableLogicalWidth, so we should protect upper classes as well against negative values. So I'm not sure there is an easy fix for that, considering how sensible those functions are.
jfernandez@igalia.com changed reviewers: + mstensho@opera.com, rune@opera.com
@cbiesigner What do you think about this ? Perhaps @rune and @mstensho think we should follow a more ambitious approach as well, but I think it's a bit risky.
I think we need to get to the bottom of this.
Simplified test (based on the LayoutTest that you add in this CL):
<!DOCTYPE html>
<div style="border:2px solid red; float:left;">
<canvas style="min-width:fit-content;"></canvas>
</div>
We're asking for the DIV's logical content width as part of preferred logical
width calculation, which seems bad, because:
1. Its width hasn't been set yet
And besides:
2. Its width shouldn't affect preferred width calculation of any descendant
LayoutReplaced::computePreferredLogicalWidths() calls
computeReplacedLogicalWidth(), which seems suspicious. Well, we pass a
ComputePreferred value, but the computeReplacedLogicalWidth() implementation
doesn't seem to care enough about that, and we end up calling
LayoutBox::computeReplacedLogicalWidth() further down the road. But we haven't
even laid out the DIV yet, so that seems wrong.
https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fit-content-min-width-with-zero-width-container-should-not-crash.html
(right):
https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fit-content-min-width-with-zero-width-container-should-not-crash.html:12:
<script src="../../resources/js-test.js"></script>
Nothing is tested in this test, apart from the crash. Did you include this just
to get testRunner.dumpAsText()?
On 2016/08/09 11:35:26, mstensho wrote:
> I think we need to get to the bottom of this.
>
> Simplified test (based on the LayoutTest that you add in this CL):
>
> <!DOCTYPE html>
> <div style="border:2px solid red; float:left;">
> <canvas style="min-width:fit-content;"></canvas>
> </div>
>
> We're asking for the DIV's logical content width as part of preferred logical
> width calculation, which seems bad, because:
I don't understand why you stated that. Where are we requesting DIV's logical
content width ?
I've been debugging the simplified test case you described above and as far as I
could so, so far
we are always operating on the canvas element. As a matter of fact, the
assertion is violated
while processing LayoutBox::computeReplacedLogicalWidthUsing on the
LayoutHTMLCanvas
element:
LayoutView 0x3fd900a04010 #document 0x1abfeb5a3a90
LayoutBlockFlow 0x3fd900a24010 HTML 0x1abfeb5a4870
LayoutBlockFlow 0x3fd900a24130 BODY 0x1abfeb5a49b0
LayoutBlockFlow (floating) 0x3fd900a24250 DIV 0x1abfeb5a4a50
STYLE="border:2px solid red; float:left;"
* LayoutHTMLCanvas 0x3fd900a141f0 CANVAS 0x1abfeb5a4b60
STYLE="min-width:fit-content;"
LayoutText 0x3fd900a30010 #text 0x1abfeb5a4cd0 "\n"
> LayoutReplaced::computePreferredLogicalWidths() calls
> computeReplacedLogicalWidth(), which seems suspicious.
Why suspicious ?
> Well, we pass a
> ComputePreferred value, but the computeReplacedLogicalWidth() implementation
> doesn't seem to care enough about that,
It's used by LayoutBox::computeReplacedLogicalWidthRespectingMinMaxWidth to
decide whether to early return the already computed intrinsic width as the
preferred size.
> and we end up calling
> LayoutBox::computeReplacedLogicalWidth() further down the road. But we haven't
> even laid out the DIV yet, so that seems wrong.
Again, I'm not following the rationale at this point. Why on the DIV ?
https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fit-content-min-width-with-zero-width-container-should-not-crash.html (right): https://codereview.chromium.org/2065243003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fit-content-min-width-with-zero-width-container-should-not-crash.html:12: <script src="../../resources/js-test.js"></script> On 2016/08/09 11:35:26, mstensho wrote: > Nothing is tested in this test, apart from the crash. Did you include this just > to get testRunner.dumpAsText()? I think we can remove it, indeed.
On 2016/08/18 19:20:29, jfernandez wrote: > On 2016/08/09 11:35:26, mstensho wrote: > > I think we need to get to the bottom of this. > > > > Simplified test (based on the LayoutTest that you add in this CL): > > > > <!DOCTYPE html> > > <div style="border:2px solid red; float:left;"> > > <canvas style="min-width:fit-content;"></canvas> > > </div> > > > > We're asking for the DIV's logical content width as part of preferred logical > > width calculation, which seems bad, because: > > I don't understand why you stated that. Where are we requesting DIV's logical > content width ? > I've been debugging the simplified test case you described above and as far as I > could so, so far > we are always operating on the canvas element. As a matter of fact, the > assertion is violated > while processing LayoutBox::computeReplacedLogicalWidthUsing on the > LayoutHTMLCanvas > element: > > LayoutView 0x3fd900a04010 #document 0x1abfeb5a3a90 > LayoutBlockFlow 0x3fd900a24010 HTML 0x1abfeb5a4870 > LayoutBlockFlow 0x3fd900a24130 BODY 0x1abfeb5a49b0 > LayoutBlockFlow (floating) 0x3fd900a24250 DIV 0x1abfeb5a4a50 > STYLE="border:2px solid red; float:left;" > * LayoutHTMLCanvas 0x3fd900a141f0 CANVAS 0x1abfeb5a4b60 > STYLE="min-width:fit-content;" > LayoutText 0x3fd900a30010 #text 0x1abfeb5a4cd0 "\n" > > > > LayoutReplaced::computePreferredLogicalWidths() calls > > computeReplacedLogicalWidth(), which seems suspicious. > > Why suspicious ? While working on calculating the preferred min/max widths of the DIV and its children, we calculate the actual width of the CANVAS. This is bad, because, in order to calculate the width of the CANVAS, we need to know the width of the containing block, which isn't known yet. Stack trace from the assertion failure (without your patch): #0 0x00007fffeafe7bc9 in blink::LayoutBox::fillAvailableMeasure (this=0x29a776c24010, availableLogicalWidth=-4px, marginStart=0px, marginEnd=0px) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2343 #1 0x00007fffeafe7b46 in blink::LayoutBox::fillAvailableMeasure (this=0x29a776c24010, availableLogicalWidth=-4px) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2338 #2 0x00007fffeafe7db7 in blink::LayoutBox::computeIntrinsicLogicalWidthUsing (this=0x29a776c24010, logicalWidthLength=Length(MaxContent), availableLogicalWidth=-4px, borderAndPadding=0px) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2369 #3 0x00007fffeafea6b2 in blink::LayoutBox::computeReplacedLogicalWidthUsing (this=0x29a776c24010, sizeType=blink::MinSize, logicalWidth=Length(MaxContent)) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2926 #4 0x00007fffeafea36d in blink::LayoutBox::computeReplacedLogicalWidthRespectingMinMaxWidth (this=0x29a776c24010, logicalWidth=300px, shouldComputePreferred=blink::ComputeActual) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2894 #5 0x00007fffeafea2da in blink::LayoutBox::computeReplacedLogicalWidth (this=0x29a776c24010, shouldComputePreferred=blink::ComputeActual) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2889 #6 0x00007fffeb08abeb in blink::LayoutReplaced::constrainIntrinsicSizeToMinMax (this=0x29a776c24010, intrinsicSizingInfo=...) at ../../third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:187 #7 0x00007fffeb08cb44 in blink::LayoutReplaced::computeReplacedLogicalWidth (this=0x29a776c24010, shouldComputePreferred=blink::ComputePreferred) at ../../third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:585 #8 0x00007fffeb08d1ba in blink::LayoutReplaced::computePreferredLogicalWidths (this=0x29a776c24010) at ../../third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:675 #9 0x00007fffeafe0cf6 in blink::LayoutBox::minPreferredLogicalWidth (this=0x29a776c24010) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:1067 #10 0x00007fffeafa3950 in blink::LayoutBlock::computeChildPreferredLogicalWidths (this=0x29a776c18250, child=..., minPreferredLogicalWidth=0px, maxPreferredLogicalWidth=0px) at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1397 #11 0x00007fffeafced96 in blink::LayoutBlockFlow::computeInlinePreferredLogicalWidths (this=0x29a776c18250, minLogicalWidth=0px, maxLogicalWidth=0px) at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:1347 #12 0x00007fffeafa2a79 in blink::LayoutBlock::computeIntrinsicLogicalWidths (this=0x29a776c18250, minLogicalWidth=0px, maxLogicalWidth=0px) at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1233 #13 0x00007fffeafa3557 in blink::LayoutBlock::computePreferredLogicalWidths (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1268 #14 0x00007fffeafe0cf6 in blink::LayoutBox::minPreferredLogicalWidth (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:1067 #15 0x00007fffeafde926 in blink::LayoutBox::computeLogicalWidthUsing (this=0x29a776c18250, widthType=blink::MainOrPreferredSize, logicalWidth=Length(Auto), availableLogicalWidth=769px, cb=0x29a776c18130) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2398 #16 0x00007fffeafe6927 in blink::LayoutBox::computeLogicalWidth (this=0x29a776c18250, computedValues=...) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2302 #17 0x00007fffeafe611d in blink::LayoutBox::updateLogicalWidth (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBox.cpp:2209 #18 0x00007fffeaf9e993 in blink::LayoutBlock::updateLogicalWidthAndColumnWidth (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:408 #19 0x00007fffeafaf11f in blink::LayoutBlockFlow::updateLogicalWidthAndColumnWidth (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:231 #20 0x00007fffeafc0d6b in blink::LayoutBlockFlow::layoutBlockFlow (this=0x29a776c18250, relayoutChildren=false, pageLogicalHeight=0px, layoutScope=...) at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:423 #21 0x00007fffeafafe63 in blink::LayoutBlockFlow::layoutBlock (this=0x29a776c18250, relayoutChildren=false) at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:403 #22 0x00007fffeaf9e7bf in blink::LayoutBlock::layout (this=0x29a776c18250) at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:374 I think it goes wrong in frame #8, which jumps directly from the preferred width computation machinery to the actual layout machinery (computePreferredLogicalWidths() calls computeReplacedLogicalWidth()). That would only be safe if the object in question doesn't depend on anything in the ancestry (such as e.g. the actual width of its containing block). git blame gives me commit 40beaf69345c5d0ce2dd8ac8d35d9f0265847347 - https://bugs.webkit.org/show_bug.cgi?id=110393 > > > Well, we pass a > > ComputePreferred value, but the computeReplacedLogicalWidth() implementation > > doesn't seem to care enough about that, > > It's used by LayoutBox::computeReplacedLogicalWidthRespectingMinMaxWidth to > decide whether to early return the already computed intrinsic width as the > preferred size. > > > and we end up calling > > LayoutBox::computeReplacedLogicalWidth() further down the road. But we haven't > > even laid out the DIV yet, so that seems wrong. > > Again, I'm not following the rationale at this point. Why on the DIV ? We're NOT on the DIV. We're on the CANVAS. But we haven't yet laid out the DIV at this point, and the DIV's actual width is involved in calculating the width of the CANVAS. That's my point. :)
Any progress here?
On 2016/10/31 12:03:24, mstensho wrote: > Any progress here? Not yet, sorry. I was busy with other grid related tasks but I have plans to retake this issue ASAP.
Description was changed from ========== [css-sizing] Ensure positive available size on replaced elements When using fit-content as min-width on replaced elements it's possible to use a negative available size when computing the mentioned intrinsic size. We don't protect against negative available width in case of float containers with replaced element children. This patch ensure the container's width obtained is at least 0px, as we currently do for other cases. BUG=620235 ========== to ========== [css-sizing] Ensure positive available size on replaced elements When using fit-content as min-width on replaced elements it's possible to use a negative available size when computing the mentioned intrinsic size. We don't protect against negative available width in case of float containers with replaced element children. This patch ensure the container's width obtained is at least 0px, as we currently do for other cases. BUG=635019 ==========
@mstensho, what do you think about the new approach ?
On 2016/11/02 19:19:22, jfernandez wrote: > @mstensho, what do you think about the new approach ? It doesn't seems the right approach, I'll have to look further into this.
PatchSet 7 looks like the way to go. It might need some code clean up, though, but I think it's ready to be reviewed.
seems reasonable to me, lgtm, but please also let Morten take a look
I think this makes sense, but you need to update the commit comment now, right? https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:3263: style()->logicalMaxWidth().isFillAvailable())) || So, what's a good term to use when you're either percent, calc, fit-content or fill-available? :) It would be nice to introduce a Length::isSomethingBlah() for this, but maybe we don't need to figure it out as part of this CL. https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutReplaced.cpp (right): https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:829: style()->logicalMinWidth().isFitContent() || Why no isPercentOrCalc() here?
https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBox.cpp:3263: style()->logicalMaxWidth().isFillAvailable())) || On 2016/11/10 19:54:25, mstensho wrote: > So, what's a good term to use when you're either percent, calc, fit-content or > fill-available? :) > > It would be nice to introduce a Length::isSomethingBlah() for this, but maybe we > don't need to figure it out as part of this CL. If I manage to do something very simple, I think it'd make sense to do it as part of this CL, so the chance becomes simpler and clearer. https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutReplaced.cpp (right): https://codereview.chromium.org/2065243003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:829: style()->logicalMinWidth().isFitContent() || On 2016/11/10 19:54:25, mstensho wrote: > Why no isPercentOrCalc() here? umm, not sure, I thought about that. I tried to keep the change as simple as possible, only addressing the specific issue. However, it mat sense to evaluate other cases. I'll try to figure out other test cases to reproduce the issue so it can get fixed by this CL as well.
Description was changed from ========== [css-sizing] Ensure positive available size on replaced elements When using fit-content as min-width on replaced elements it's possible to use a negative available size when computing the mentioned intrinsic size. We don't protect against negative available width in case of float containers with replaced element children. This patch ensure the container's width obtained is at least 0px, as we currently do for other cases. BUG=635019 ========== to ========== [css-sizing] Ensure positive available size on replaced elements When computing the preferred widths of replaced elements some values need to be ignored, since we can't computed them properly before performing a layout. We have protected already some cases in r143539 (commit 40beaf69), like fit-content, fit-available or percentage and calculated values. However, we still need to protect cases where these content-dependent values are used on min-width/max-width properties. BUG=635019 ==========
PatchSet7 defines a new term 'isLayoutDependent" which includes the cases we need to avoid when computing the intrinsic size of replaced elements, since they depend on the available size provided by the containing block, which can't only be determined after laying it out. I have some doubts about that new term, so I'd appreciate some feedback: 1- I defined it as part of the Length class, which is widely used. There are other values that would be depend on layout, so perhaps its not accurate enough. 2- I could define some utility method specifically for the replaced elements case, but we need to use it at least in LayoutBox and LayoutReplaced, so not a good option. 3- More than depending on layout, these values have in common that they require the available size in the containing block to be properly computed. My doubt is whether 'auto' will also be in this category. 4- Some alternate terms that come to my mind are: - requiresContainerAvailableSize - requiresContainerSize - isContainerDependent If we are not sure, we can skip this step and land the patch with the complex clauses, so we can figure out a more precise term later.
It may turn out that different object types will need to define different length types as "layout dependent" (in which case Length is the wrong place to define it), but I suggest that we deal with that if the problem arises. I like the name, but some documentation in front of the declaration would be nice.
jfernandez@igalia.com changed reviewers: + esprehn@chromium.org, tkent@chromium.org
Added @esprehn and @tkent as platform OWNERs .
third_party/WebKit/Source/platform/Length.h lgtm. It's a simple reasonable change.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2065243003/#ps160001 (title: "Added some comments for the new Length API function.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jfernandez@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
It seems my patch causes 2 browser tests to fail: - PluginPowerSaverBrowserTest.SmallerThanPlayIcon - PluginPowerSaverBrowserTest.PosterTests I confirmed locally that it's indeed my patch the root cause of such failures, but I've been unable to figure out why.
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:220001) has been deleted
Even though a rebasline of SmallerThanPlayIcon seems correct, and the bots indicate so, the PosterTests still fail in the linux bot. However, such test passes locally in my current linux build.
jfernandez@igalia.com changed reviewers: + tommycli@chromium.org
pleas tommycli@, could you please take a look ?
On 2017/02/08 23:54:48, jfernandez wrote: > pleas tommycli@, could you please take a look ? Hey if you look at the images, the behavior is changed. Some of the play buttons are squashed... Is that intended? The CSS is at https://cs.chromium.org/chromium/src/chrome/renderer/resources/plugins/plugin...
On 2017/02/08 23:58:19, tommycli wrote: > On 2017/02/08 23:54:48, jfernandez wrote: > > pleas tommycli@, could you please take a look ? > > Hey if you look at the images, the behavior is changed. Some of the play buttons > are squashed... > > Is that intended? The CSS is at > https://cs.chromium.org/chromium/src/chrome/renderer/resources/plugins/plugin... Well, my patch could have that collateral effect, since it changes how replaced elements compute their intrinsic size. The algorithm we were using was not correct and lead to negative values, which was what triggered the assertion, in some cases. I already had to rebaseline the SmallerThanPlayIcon tests because of that, which after looking at the image diff I seemed correct to me; a second opinion will be indeed more than welcome. What I can't understand is the failure of the PosterTests, which I couldn't reproduce locally on my linux. It also worries me the InputRouterImplTest.AckedTouchEventState, which I can't reproduce locally either. Where I can get the images diff from the bot results ?
On 2017/02/09 08:34:17, jfernandez wrote: > On 2017/02/08 23:58:19, tommycli wrote: > > On 2017/02/08 23:54:48, jfernandez wrote: > > > pleas tommycli@, could you please take a look ? > > > > Hey if you look at the images, the behavior is changed. Some of the play > buttons > > are squashed... > > > > Is that intended? The CSS is at > > > https://cs.chromium.org/chromium/src/chrome/renderer/resources/plugins/plugin... > > Well, my patch could have that collateral effect, since it changes how replaced > elements compute their intrinsic size. The algorithm we were using was not > correct > and lead to negative values, which was what triggered the assertion, in some > cases. > > I already had to rebaseline the SmallerThanPlayIcon tests because of that, which > > after looking at the image diff I seemed correct to me; a second opinion will be > indeed more than welcome. > > What I can't understand is the failure of the PosterTests, which I couldn't > reproduce > locally on my linux. It also worries me the > InputRouterImplTest.AckedTouchEventState, > which I can't reproduce locally either. > > Where I can get the images diff from the bot results ? Hey, it looks like the test is failing because the rebaseline didn't produce a result that matched the linux bot. There's no easy way to get the images from the bot results. As for the images being squashed -- they shouldn't be squashed. The solution isn't to rebaseline the images, it's to make the play icon non-squashed. Either my CSS is subtly incorrect (probably a one-line fix in above file) or the layout algorithm change changes the behavior of image fit in an undesirable way. In that case, it will break images / cause unexpected changes on images in the web in general.
Please, cbiesinger@, could you take a look at this patch ? As it was commented previously during the review, we should avoid using the layout logic for computing the intrinsic size; that's what this patch does for replaced elements. Using the layout logic for computing the intrinsic size could lead to negative Layoutunit values for the availableSize, used when computing content-sized preferred sizes, as it' shown in the linked bug of this CL. In addition to protect the already implemented cases, we considered that max/min-width restrictions depending on the container size could lead to the above mentioned issues. These new cases are the ones that cause the failures in browser tests. The reason is that those plugins use flexbox for building the LayoutTree associated to such flash objects. src/chrome/renderer/resources/plugins/plugin_poster.html I think the patch is correct, but it triggers some cases where flexbox is not dealing correctly with items that need to keep the aspect ratio. I filed bug #697252 to illustrate the issue using a reduce test case.
On 2017/03/01 00:08:15, jfernandez wrote: > These new cases are the ones that cause the failures in > browser tests. The reason is that those plugins use > flexbox for building the LayoutTree associated to such > flash objects. > > src/chrome/renderer/resources/plugins/plugin_poster.html > > I think the patch is correct, but it triggers some cases > where flexbox is not dealing correctly with items that > need to keep the aspect ratio. I'm not sure about this flexbox cases, but here is one example: http://jsbin.com/zadisi/1/edit?html,output The image is 50x50, and the flex container 64x16. The result is: 1) width: auto; => Image is 50x16 2) width: auto; max-height: 100%; => Image is 16x16 3) width: auto; max-width: 100%; max-height: 100%; => With this patch image is 50x16 => Without this patch image is 16x16 I believe that it should be 16x16 so this patch is introducing a mistake. But please @cbiesinger confirm this.
On 2017/03/01 14:06:17, Manuel Rego wrote: > On 2017/03/01 00:08:15, jfernandez wrote: > > These new cases are the ones that cause the failures in > > browser tests. The reason is that those plugins use > > flexbox for building the LayoutTree associated to such > > flash objects. > > > > src/chrome/renderer/resources/plugins/plugin_poster.html > > > > I think the patch is correct, but it triggers some cases > > where flexbox is not dealing correctly with items that > > need to keep the aspect ratio. > > I'm not sure about this flexbox cases, but here is one example: > http://jsbin.com/zadisi/1/edit?html,output > > The image is 50x50, and the flex container 64x16. > The result is: > 1) width: auto; > => Image is 50x16 > 2) width: auto; max-height: 100%; > => Image is 16x16 > 3) width: auto; max-width: 100%; max-height: 100%; > => With this patch image is 50x16 > => Without this patch image is 16x16 > > I believe that it should be 16x16 so this patch is introducing a mistake. > But please @cbiesinger confirm this. The issue I'd need to know is why a change in the intrinsic size computation of a replaced element could affect like that. As far as I understand, a replaced element with width auto and max-width:100% should not use the layout logic (LayoutBox::computeReplacedLogicalWidth) to determine its intrinsic size; exactly the same we do for width: 100%. If my assumption above is correct, there is something Flexbox is not considering when using the replaced element intrinsic size as the flex item inner-size.
Some additional info; the patch avoids the call to LayoutReplaced::computeReplacedLogicalWidth in case of the replaced element has width 'auto' and max-width as percentage. This change is intended to avoid using specific layout logic during the intrinsic size computation, since we may still miss the container's size data. The above mentioned method does something very special with the function: LayoutReplaced::constrainIntrinsicSizeToMinMax This is what Flexbox eventually receives as flex item's inner size.
On 2017/03/01 17:30:16, jfernandez wrote: > Some additional info; the patch avoids the call to > LayoutReplaced::computeReplacedLogicalWidth in case of > the replaced element has width 'auto' and max-width as > percentage. This change is intended to avoid using specific layout logic during > the intrinsic size computation, since > we may still miss the container's size data. > > The above mentioned method does something very special with > the function: LayoutReplaced::constrainIntrinsicSizeToMinMax > > This is what Flexbox eventually receives as flex item's > inner size. FWIW, Firefox also displays the #3 case image as 16x16. BTW, Manuel, thanks for the minimal test case!
This CL seems to be stuck. Can I help in any way?
On 2017/04/04 10:58:20, mstensho wrote: > This CL seems to be stuck. Can I help in any way? yeah, I asked @cbiesinger about it in comment #50. I don't find a way to solve this issue without breaking current flexbox behavior regarding intrinsic size. The patch looks correct, since we shouldn't execute the layout logic during intrinsic size computation, because as you pointed out, we might unknown container's size, which eventually leads to violate the assertion originally reported in the bug.
On 2017/04/04 11:08:22, jfernandez wrote: > On 2017/04/04 10:58:20, mstensho wrote: > > This CL seems to be stuck. Can I help in any way? > > yeah, I asked @cbiesinger about it in comment #50. I don't find > a way to solve this issue without breaking current flexbox behavior > regarding intrinsic size. The patch looks correct, since we shouldn't > execute the layout logic during intrinsic size computation, because > as you pointed out, we might unknown container's size, which eventually > leads to violate the assertion originally reported in the bug. So in order to avoid regressions, we first need to fix https://bugs.chromium.org/p/chromium/issues/detail?id=697252 ? The changes in the PNGs don't look like something we should accept.
On 2017/04/04 12:18:44, mstensho wrote: > On 2017/04/04 11:08:22, jfernandez wrote: > > On 2017/04/04 10:58:20, mstensho wrote: > > > This CL seems to be stuck. Can I help in any way? > > > > yeah, I asked @cbiesinger about it in comment #50. I don't find > > a way to solve this issue without breaking current flexbox behavior > > regarding intrinsic size. The patch looks correct, since we shouldn't > > execute the layout logic during intrinsic size computation, because > > as you pointed out, we might unknown container's size, which eventually > > leads to violate the assertion originally reported in the bug. > > So in order to avoid regressions, we first need to fix > https://bugs.chromium.org/p/chromium/issues/detail?id=697252 ? The changes in > the PNGs don't look like something we should accept. Yes, we need either to fix the bug, if finally confirmed, or to change Flexbox logic that now relies on the layout codepath to determine the item intrinsic size, as I explain in comment #53.
In any case, I have plans to get back on this issue, definitively. It's just that I ran out of ideas and needed to clean up a bit my tasks backlog.
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
On 2017/04/04 22:41:46, jfernandez wrote: > In any case, I have plans to get back on this issue, definitively. It's just > that I ran out of ideas and needed to clean up a bit my tasks backlog. OK. In the meantime I'm removing myself as reviewer. Feel free to re-add me if you make some more progress.
mstensho@opera.com changed reviewers: - mstensho@opera.com
tommycli@chromium.org changed reviewers: - tommycli@chromium.org |
