87905 2012-05-30 15:57:38 -0700 Can avoid a second layout of flex-items in some cases 2024-07-22 09:30:33 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 62048 1 ojan webkit-unassigned ahmad.saleem792 cgarcia eric hyatt jchaffraix sgill26 tony zalan oldest_to_newest 637776 0 ojan 2012-05-30 15:57:38 -0700 Whenever we're extending the logicalHeight of a flex-item that we've already laid out (e.g. due to flexing or flex-align:stretch), we only need to layout after modifying the height if the contents of the item depend on it's height (e.g. contains % height elements and is a containing block, has a background-color, etc) because we already. The tricky bit here is maintaining the function of when we need to do a relayout. In theory, in some of these cases we could take this a step further and avoid doing a full relayout, e.g. we don't actually need to relayout just to extend the background color further up/down; for percentage height decendants, we already keep a list of them and only need to relayout those items, etc. 766542 1 174091 cgarcia 2012-11-14 00:13:38 -0800 Created attachment 174091 Patch I'm getting a failure in 3 tests that I'm not sure why. css3/flexbox/inline-flex-crash.html css3/flexbox/inline-flex-crash2.html The output is different in both cases, but it doesn't crash. css3/flexbox/auto-margins.html The output is correct, but the first time it's called it fails to get the margin values, although the inspector shows the right ones and simply reloading the page fixes the test. All other css3 flexbox tests pass. 767065 2 174091 tony 2012-11-14 11:01:48 -0800 Comment on attachment 174091 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:1090 > + LayoutUnit childCurrentSize = isColumnFlow() ? child->logicalHeight() : child->logicalWidth(); You want mainAxisExtentForChild(). You don't want the child's logical height since it can have an orthogonal writing mode to the flexbox. 767162 3 174091 ojan 2012-11-14 12:14:54 -0800 Comment on attachment 174091 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174091&action=review This is actually a separate optimization than bug 87905. That bug is about when we *do* change the height, there are many cases where we don't need to do a layout (i.e. if there are no descendants or css properties that depend on the height). This is still a good optimization though. It just needs it's own bug. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1094 > + if (child->size().isZero() || childPreferredSize != childCurrentSize) { > + child->setChildNeedsLayout(true, MarkOnlyThis); > + child->layoutIfNeeded(); > + } I don't think we need the isZero check. Instead, we should always call child->layoutIfNeeded() here (i.e. move it out of the if-statement), but only setChildNeedsLayout if childPreferredSize != childCurrentSize. 767823 4 cgarcia 2012-11-15 01:49:35 -0800 Thank you guys for the review! I've just opened a new bug with a patch addressing your comments, see: https://bugs.webkit.org/show_bug.cgi?id=102352 css3/flexbox/inline-flex-crash.html and css3/flexbox/inline-flex-crash2.html don't fail anymore with the new patch. 2047350 5 ahmad.saleem792 2024-07-20 09:41:24 -0700 @Sammy - is it still applicable? 2047510 6 sgill26 2024-07-22 09:30:33 -0700 I think so since there is still a FIXME that references this bugzilla in RenderFlexibleBox::applyStretchAlignmentToFlexItem 174091 2012-11-14 00:13:38 -0800 2012-11-14 12:14:54 -0800 Patch css3-flexbox-layout.diff text/plain 1968 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA5MmZmZDM4Li40ZDlkMDZhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTItMTEtMTMgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIENhbiBhdm9pZCBhIHNlY29uZCBsYXlvdXQgb2YgZmxleC1pdGVtcyBpbiBz b21lIGNhc2VzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD04NzkwNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IEluIGNhc2Ugb2YgbmVzdGVkIGZsZXhib3ggY29udGFpbmVycyBpdCBjYW4gaGFwcGVuIHRoYXQg c29tZQorICAgICAgICBjaGlsZHJlbiBhcmUgbGFpZCBvdXQgbXVsdGlwbGUgdGltZXMuIEl0J3Mg cG9zc2libGUgdG8gYXZvaWQgYQorICAgICAgICBzZWNvbmQgbGF5b3V0IGluIGxheW91dEFuZFBs YWNlQ2hpbGRyZW4oKSBieSBjaGVja2luZyBpZiB0aGUKKyAgICAgICAgY2hpbGQgaGFzIGFscmVh ZHkgYSBzaXplIGFuZCBpcyB0aGUgc2FtZSB0aGFuIHRoZSBkZXNpcmVkIG9uZS4KKworICAgICAg ICAqIHJlbmRlcmluZy9SZW5kZXJGbGV4aWJsZUJveC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpS ZW5kZXJGbGV4aWJsZUJveDo6bGF5b3V0QW5kUGxhY2VDaGlsZHJlbik6CisKIDIwMTItMTEtMTIg IEFkYW0gQmFydGggIDxhYmFydGhAd2Via2l0Lm9yZz4KIAogICAgICAgICBbVjhdIFdlIHNob3Vs ZCBiZSBhYmxlIHRvIHJlY292ZXIgdGhlIFY4RE9NV2luZG93U2hlbGwgbW9yZSBxdWlja2x5CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyRmxleGlibGVCb3guY3Bw IGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckZsZXhpYmxlQm94LmNwcAppbmRleCAy ZTA4YTVjLi5lOWUyYjc0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVu ZGVyRmxleGlibGVCb3guY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJG bGV4aWJsZUJveC5jcHAKQEAgLTEwODcsOCArMTA4NywxMSBAQCB2b2lkIFJlbmRlckZsZXhpYmxl Qm94OjpsYXlvdXRBbmRQbGFjZUNoaWxkcmVuKExheW91dFVuaXQmIGNyb3NzQXhpc09mZnNldCwg Y29ucwogICAgICAgICBMYXlvdXRVbml0IGNoaWxkUHJlZmVycmVkU2l6ZSA9IGNoaWxkU2l6ZXNb aV0gKyBtYWluQXhpc0JvcmRlckFuZFBhZGRpbmdFeHRlbnRGb3JDaGlsZChjaGlsZCk7CiAgICAg ICAgIHNldExvZ2ljYWxPdmVycmlkZVNpemUoY2hpbGQsIGNoaWxkUHJlZmVycmVkU2l6ZSk7CiAg ICAgICAgIC8vIEZJWE1FOiBDYW4gYXZvaWQgbGF5aW5nIG91dCBoZXJlIGluIHNvbWUgY2FzZXMu IFNlZSBodHRwczovL3dlYmtpdC5vcmcvYi84NzkwNS4KLSAgICAgICAgY2hpbGQtPnNldENoaWxk TmVlZHNMYXlvdXQodHJ1ZSwgTWFya09ubHlUaGlzKTsKLSAgICAgICAgY2hpbGQtPmxheW91dElm TmVlZGVkKCk7CisgICAgICAgIExheW91dFVuaXQgY2hpbGRDdXJyZW50U2l6ZSA9IGlzQ29sdW1u RmxvdygpID8gY2hpbGQtPmxvZ2ljYWxIZWlnaHQoKSA6IGNoaWxkLT5sb2dpY2FsV2lkdGgoKTsK KyAgICAgICAgaWYgKGNoaWxkLT5zaXplKCkuaXNaZXJvKCkgfHwgY2hpbGRQcmVmZXJyZWRTaXpl ICE9IGNoaWxkQ3VycmVudFNpemUpIHsKKyAgICAgICAgICAgIGNoaWxkLT5zZXRDaGlsZE5lZWRz TGF5b3V0KHRydWUsIE1hcmtPbmx5VGhpcyk7CisgICAgICAgICAgICBjaGlsZC0+bGF5b3V0SWZO ZWVkZWQoKTsKKyAgICAgICAgfQogCiAgICAgICAgIHVwZGF0ZUF1dG9NYXJnaW5zSW5NYWluQXhp cyhjaGlsZCwgYXV0b01hcmdpbk9mZnNldCk7CiAK