Bug 93547 – [CSS Exclusions] Enable shape-inside for percentage lengths based on logical height

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

Several features need to resolve the logical height to use for percentage-based vertical measurements.
We should only really do this once.

Currently some code lives in computeInitialRegionRangeForBlock, and 89259 was going to add computeShapeSizeForBlock.
This should really only need to be done once, and update multiple items there.

[Bear Travis]

Created attachment 157815 [details]
Initial patch, based on 89259

Proposed patch. Diff is based off 89259, so it will not be ready until that patch lands.

[Bear Travis]

Created attachment 160472 [details]
Updated patch

Including changes from main branch, now that bug 89259 has been fixed.

[WebKit Review Bot]

Comment on attachment 160472 [details]
Updated patch

Attachment 160472 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13594283

New failing tests:
fast/exclusions/shape-inside/shape-inside-percentage-auto.html

[WebKit Review Bot]

Created attachment 160488 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bear Travis]

Created attachment 160491 [details]
Simplify percentage-auto test

[Bear Travis]

Created attachment 162594 [details]
Fixing min/max-height edge case

Logical height was not always being calculated correctly when max-height was set

[Levi Weintraub]

Comment on attachment 162594 [details]
Fixing min/max-height edge case

View in context: https://bugs.webkit.org/attachment.cgi?id=162594&action=review

> Source/WebCore/rendering/RenderBlock.cpp:1416
> +        bool percentageLogicalHeightResolvable = percentageLogicalHeightIsResolvableFromBlock(this, document()->inQuirksMode(), false);

Why is outOfFlowPositioned hardcoded to false here?

> Source/WebCore/rendering/RenderBlock.cpp:1512
> -    computeInitialRegionRangeForBlock();
> -#if ENABLE(CSS_EXCLUSIONS)
> -    // FIXME: Bug 93547: Resolve logical height for percentage based vertical lengths
> -    if (WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo())
> -        wrapShapeInfo->computeShapeSize(logicalWidth(), 0);
> -#endif
> +    computeInitialLogicalSizeBasedMeasurements();

I'm happy to see this code go into a helper.

> Source/WebCore/rendering/RenderBox.cpp:3735
> +bool RenderBox::percentageLogicalHeightIsResolvableFromBlock(const RenderBlock* containingBlock, bool quirksMode, bool outOfFlowPositioned)

boolean params :(

We appear to ask the document if it's in quirksMode in all cases, so what do we gain by making it a parameter?

[Bear Travis]

Created attachment 162899 [details]
Incorporating feedback

Fixing up patch.
Removing quirksMode argument & changing outOfFlowPositioned to an enum
The call in RenderBlock has a hardcoded InFlowPosition because percentage lengths for shape-inside are resolved as if they were in flow

[Dave Hyatt]

Comment on attachment 162899 [details]
Incorporating feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=162899&action=review

Comments below.

> Source/WebCore/rendering/RenderBlock.cpp:1397
> -void RenderBlock::computeInitialRegionRangeForBlock()
> +void RenderBlock::computeInitialLogicalSizeBasedMeasurements()

My suggestion here would be to retain computeInitialRegionRangeForBlock and have it be called by computeInitialLogicalSizeBasedMeasurements. Then have your own additional function that computeInitialLogicalSizeBasedMeasurements calls to handle exclusions.

Note the term "size" is wrong here, since all you care about is logical height, so you should rename to computeLogicalHeightBasedMeasurements. I think you can drop the term "initial" since that really only applied to region ranges and was implying that the region range might be corrected later on.

> Source/WebCore/rendering/RenderBox.h:40
> +enum PositionType { InFlowPosition, OutOfFlowPosition };

I'm not following why this needs to be added. I don't like it.

[Bear Travis]

Created attachment 163216 [details]
Incorporating feedback

Fixing up the patch. computeInitialLogicalSizeBasedMeasurements computes the maximum possible logical height, then passes it, along with the current logical width, along to regions and exclusions. With the calculations removed, computeInitialRegionRangeForBlock was obsoleted, and I added a call to computeRegionRangeForBlock instead.
Removed the term initial
Modified the code to call the new const versions of computeLogicalHeight.
Removed PositionType

[Bear Travis]

Created attachment 163233 [details]
Updating patch

Merging with more recent changes

[Build Bot]

Comment on attachment 163233 [details]
Updating patch

Attachment 163233 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13810400

[Bear Travis]

Created attachment 163249 [details]
Fixing windows build

[WebKit Review Bot]

Comment on attachment 163249 [details]
Fixing windows build

Attachment 163249 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13822124

New failing tests:
fast/repaint/region-painting-via-layout.html

[Bear Travis]

Created attachment 163425 [details]
Incorporating feedback

Responding to feedback from Mihnea.
Updating changelog to be more descriptive.
Caching inQuirksMode boolean in RenderBox::percentageLogicalHeightIsResolvable.
Renaming computeLogicalSizeBasedMeasurements -> updateRegionsAndExclusionsLogicalSize, and removed the logical width/height parameters.

[WebKit Review Bot]

Comment on attachment 163425 [details]
Incorporating feedback

Attachment 163425 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13825380

New failing tests:
fast/repaint/region-painting-via-layout.html

[Bear Travis]

Created attachment 163753 [details]
Updating patch

[WebKit Review Bot]

Comment on attachment 163753 [details]
Updating patch

Attachment 163753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13826979

New failing tests:
fast/repaint/region-painting-via-layout.html

[Bear Travis]

Created attachment 164013 [details]
Use old-style updateLogicalHeight

Simplify patch to use updateLogicalHeight, rather than the const computeLogicalHeight. The code should eventually use computeLogicalHeight, but several areas of the code need to be modified to not use the side effects of updateLogicalHeight.

[Bear Travis]

Adding bug 96724 to track the const computeLogicalHeight issue.

[Tony Chang]

(In reply to comment #19)
> Created an attachment (id=164013) [details]
> Use old-style updateLogicalHeight
> 
> Simplify patch to use updateLogicalHeight, rather than the const computeLogicalHeight. The code should eventually use computeLogicalHeight, but several areas of the code need to be modified to not use the side effects of updateLogicalHeight.

BTW, one of the know problems with computeLogicalHeight right now is that it's not virtual so if you call it on a pointer and your pointer overrides updateLogicalHeight (e.g., it's an iframe), it'll do the wrong thing.

I think long term, we want to make computeLogicalHeight virtual and move code from the subclass updateLogicalHeight into it and then make updateLogicalHeight non-virtual.  I've filed bug 96804 for this.

[Levi Weintraub]

Comment on attachment 164013 [details]
Use old-style updateLogicalHeight

View in context: https://bugs.webkit.org/attachment.cgi?id=164013&action=review

Otherwise this looks about done.

> Source/WebCore/rendering/RenderBlock.cpp:1397
> -void RenderBlock::computeInitialRegionRangeForBlock()
> +void RenderBlock::updateRegionsAndExclusionsLogicalSize()

Rehashing Hyatt's comment, this only deals with top and height, so the name seems wrong.

[Levi Weintraub]

Comment on attachment 164013 [details]
Use old-style updateLogicalHeight

View in context: https://bugs.webkit.org/attachment.cgi?id=164013&action=review

LGTM.

>> Source/WebCore/rendering/RenderBlock.cpp:1397
>> +void RenderBlock::updateRegionsAndExclusionsLogicalSize()
> 
> Rehashing Hyatt's comment, this only deals with top and height, so the name seems wrong.

computeExlucsionShapeSize does indeed need and use width, so I take this back.

[WebKit Review Bot]

Comment on attachment 164013 [details]
Use old-style updateLogicalHeight

Clearing flags on attachment: 164013

Committed r128786: <http://trac.webkit.org/changeset/128786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.