Bug 116689 – clearLayoutOverflow should not be called before calling updateScrollInfoAfterLayout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Roger Fong]

See attached test case. 

The red box (just a span) starts off in the right location.
After resizing the window from the left side, the box repositions itself but is draw 5 pixels lower than it was before.
There are also a number of css properties besides -webkit-box that seem to be necessary to reproduce the issue.

[Roger Fong]

Created attachment 202738 [details]
Test Case

[Roger Fong]

        The issue here is that RenderFlexibleBox’s layoutBlock has it’s own way of dealing
        with scroll information updates.
        RenderBlock’s baselinePositioning code uses scroll information when determining
        the baselinePosition however, it does so incorrectly.
        For example, RenderBlock expects the scroll info to have been updated already
        when calculating the baselinePosition. In the case of RenderFlexibleBox however,
        layout of the flex items occurs before updating the scroll info, leading to 
        incorrect and unstable behavior.

After getting rid of the override and running the tests, there are no additional failures.
I think that the correct move here is to get rid of the overrides and if any "backwards compatibility" issues arise as a result, we can fix them as we go (or decide not to).

[Roger Fong]

Created attachment 203104 [details]
patch

Fixes the problem: yes
Causes layout test regressions: no
Confidence in patch: Little to none

[Simon Fraser (smfr)]

Comment on attachment 203104 [details]
patch

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

> Source/WebCore/rendering/RenderMenuList.h:-126
> -    // Flexbox defines baselines differently than regular blocks.
> -    // For backwards compatibility, menulists need to do the regular block behavior.
> -    virtual int baselinePosition(FontBaseline baseline, bool firstLine, LineDirectionMode direction, LinePositionMode position) const OVERRIDE
> -    {
> -        return RenderBlock::baselinePosition(baseline, firstLine, direction, position);
> -    }
> -    virtual int firstLineBoxBaseline() const OVERRIDE { return RenderBlock::firstLineBoxBaseline(); }
> -    virtual int inlineBlockBaseline(LineDirectionMode direction) const OVERRIDE { return RenderBlock::inlineBlockBaseline(direction); }
> -

I think you need to understand why these were added before removing them.

[Roger Fong]

Agreed, I was just told to upload some sort of patch to get some push on it.

[Roger Fong]

I have done a little more investigating however. Prepare for spew!!!

Let's consider the case where RenderMenuList was still a RenderDeprecatedFlexibleBox:
It's layoutBlock code has the following:
    // Update our scrollbars if we're overflow:auto/scroll/hidden now that we know if
    // we overflow or not.
    if (hasOverflowClip())
        layer()->updateScrollInfoAfterLayout();

This is the code that updates the scroll information before all that baselinePositioning code gets called.
It finds the select element to have overflowTop.y = 1 and border.top.y = 1 (from the user agent style sheet, maybe this is why chrome isn't seeing issues), thus setting scroll origin to 0, thus setting ignoreBaseline to false, thus setting the baseline correctly.

Now,
Let's consider when RenderMenuList is a RenderFlexibleBox.
    // Update our scroll information if we're overflow:auto/scroll/hidden now that we know if
    // we overflow or not.
    updateScrollInfoAfterLayout();
(which seems fishy for starters.) Anyways, no layer scroll updates happen.

SIDE NOTE: if I change this code to match RenderDeprecatedFLexibleBox, we get the correct behaviour. It even sets scrollOrigin correctly!

The containing element (in this case the body which is a RenderDeprecatedFlexibleBox) continues on to call all that baselinePosition code. 
Then we get to RenderBlock::finishDelayUpdateScrollInfo(); in RenderDeprecatedFlexibleBox::layoutHorizontalBox which finds that the DelayedUpdateScrollInfoSet is not empty and proceeds to updates the scroll info, but incorrectly as it sets scroll origin to -1 because it thinks that overflowTop() is 0 and borderTop is 1.

[Roger Fong]

Thus what we need to figure out here is 
1. Why does the layer's scroll info get set incorrectly from the when it's the containing RenderDeprecatedFlexibleBox that updates the layer's scroll info, but not when it's getting updated directly from the element that the layer represents.

2. Why doesn't the RenderFlexibleBox code update the layer's scroll info? What about it makes it different from RenderDeprecatedFlexibleBox in that regard? Or is it actually a typo?

3. Why is there scroll stuff happening at all for a select element. (Perhaps it's not really the fact that it's a select element but just the fact that it has it's own RenderLayer?)

My brain hurts.

[Roger Fong]

(In reply to comment #7)
> 1. Why does the layer's scroll info get set incorrectly from the when it's the containing RenderDeprecatedFlexibleBox that updates the layer's scroll info, but not when it's getting updated directly from the element that the layer represents.

!! We call RenderBlock::clearLayoutOverflow which zeros out the overflowRect, but I think it's expected that the scroll origin of the layer has been computed by then via updateScrollInfoAfterLayout(). Because RenderFlexibleBox doesn't do that however, we then fall back to the parent's DelayedUpdateScrollInfoSet, which tries to update the scroll info incorrectly since we've already cleared the overflow rect.

> 2. Why doesn't the RenderFlexibleBox code update the layer's scroll info? What about it makes it different from RenderDeprecatedFlexibleBox in that regard? Or is it actually a typo?

!! I'm becoming more convinced that RenderFlexibleBox just needs to update the RenderLayer's scroll info like RenderDeprecatedFlexibleBox does.

> 3. Why is there scroll stuff happening at all for a select element. (Perhaps it's not really the fact that it's a select element but just the fact that it has it's own RenderLayer?)

All this scrollorigin/info updating stuff is on RenderLayer not on RenderMenuList, so perhaps it's not surprising scroll stuff is happening, it's just general RenderLayer behavior?

[Roger Fong]

> > 2. Why doesn't the RenderFlexibleBox code update the layer's scroll info? What about it makes it different from RenderDeprecatedFlexibleBox in that regard? Or is it actually a typo?
> 
> !! I'm becoming more convinced that RenderFlexibleBox just needs to update the RenderLayer's scroll info like RenderDeprecatedFlexibleBox does.

Simon pointed out that the code to do this is there, just not getting called.
It's not getting called because of some delayUpdateScrollInfo stuff.
A few more of the dots are starting to connect now.

Upon entering RenderFlexibleBox::layoutBlock, gDelayUpdateScrollInfo is already 1 from a previous call to RenderDeprecatedFlexibleBox::layoutHorizontalBox (this is the select element's containing parent)

It remains at 1 by the time we leave call updatescrollinfoafterlayout, and thus we ignore updating the render layer's scroll info.

We then eventually call clearLayoutOverflow(), (still while gDelayUpdateScrollInfo is 1) and then when we finally return to RenderDeprecatedFlexibleBox::layoutHorizontalBox and call finishDelayUpdateScrollInfo, we set gDelayUpdateScrollInfo back down to 0 and perform all the delayed scroll updates, including on the layer that was ignored earlier.
However, since clearLayoutOverflow has already been called at this point and layoutOverFlowRect has been 0'ed out, the scroll origin gets improperly set.

I will look into these delayscrollinfo changes but perhaps what needs to happen is that the clearLayoutOverflow must also be delayed until finishDelayUpdateScrollInfo, that way the scollinfo will be updated properly.

This is fun.

[Roger Fong]

Created attachment 203284 [details]
patch

A better, more thought out patch.
Although I'm not clear on the ramifications on delaying the call to clearOverflowRect().

[Roger Fong]

> Although I'm not clear on the ramifications on delaying the call to clearOverflowRect().

Again, no layout test regressions.

[Ojan Vafai]

Comment on attachment 203284 [details]
patch

This patch looks correct to me for the most part. It definitely seems wrong to me that we clearLayoutOverflow in the case where we've delayed updating scrollinfo.

-I'd prefer to see the clearLayoutOverflow call in RenderBlock::updateScrollInfoAfterLayout in the !gDelayUpdateScrollInfo and style()->isFlippedBlocksWritingMode() cases and removed from RenderBlock::layout entirely. Keeps all the delayed update logic together.
-Would be good to add a ASSERT(!gDelayUpdateScrollInfo) to clearLayoutOverflow. Related to this, as best I can tell, we only ever call this on RenderBlocks, so we could move this function from RenderBox into RenderBlock to avoid adding gDelayUpdateScrollInfo knowledge to RenderBox.
-Not directly related to your patch, but while we're in this code...it doesn't seem like the hasControlClip check provides any value. clearLayoutOverflow already early returns if !m_overflow. I might be missing something here though.
-Needs a test obviously. :)

Also, reading the backthread a bit, I'm surprised that removing the baseline code in RenderFlexibleBox didn't cause any test failures. The patch that added those definitely had tests and there are a bunch of baseline tests in css3/flexobx. Maybe the relevant test is skipped?

[Roger Fong]

<rdar://problem/13595525>

[Roger Fong]

(In reply to comment #12)
> (From update of attachment 203284 [details])
> -I'd prefer to see the clearLayoutOverflow call in RenderBlock::updateScrollInfoAfterLayout in the !gDelayUpdateScrollInfo and style()->isFlippedBlocksWritingMode() cases and removed from RenderBlock::layout entirely. Keeps all the delayed update logic together.

Sounds good!

> -Would be good to add a ASSERT(!gDelayUpdateScrollInfo) to clearLayoutOverflow. Related to this, as best I can tell, we only ever call this on RenderBlocks, so we could move this function from RenderBox into RenderBlock to avoid adding gDelayUpdateScrollInfo knowledge to RenderBox.

Hmm yeah and it also builds fine after moving it, I'll do that.

> -Not directly related to your patch, but while we're in this code...it doesn't seem like the hasControlClip check provides any value. clearLayoutOverflow already early returns if !m_overflow. I might be missing something here though.

Just looking at the method, hasControlClip seems to be unrelated to m_overflow, it's overridden in various Render classes and simply either returns true or false (in RenderMenuLists's case the implementation is just: return true;) I guess it doesn't matter if I'm removing the call from layout() altogether though :)

> -Needs a test obviously. :)

Will do.

> Also, reading the backthread a bit, I'm surprised that removing the baseline code in RenderFlexibleBox didn't cause any test failures. The patch that added those definitely had tests and there are a bunch of baseline tests in css3/flexobx. Maybe the relevant test is skipped?

Humm they are not skipped. Perhaps something else progressed them. Or maybe it has to do with Safari's user agent style sheet again? Which may have been why my test case wasn't repro'ing on chrome earlier.

[Roger Fong]

(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 203284 [details] [details])
> > -I'd prefer to see the clearLayoutOverflow call in RenderBlock::updateScrollInfoAfterLayout in the !gDelayUpdateScrollInfo and style()->isFlippedBlocksWritingMode() cases and removed from RenderBlock::layout entirely. Keeps all the delayed update logic together.
> 
> Sounds good!
> 

Hmm one concern I have is if anything else needs the layoutOverflowRect between the time we call updateScrolInfoAfterLayout and we exit the RenderBlock::layout. (where it was called before).
Although I guess updateScrollInfoAFTERlayout should imply that all the layout stuff has already happened and we're good to go?

[Roger Fong]

One more thing is that RenderDeprecatedFlexibleBox and RenderGrid both call
layer()->updateScrollInfoAfterLayout();
directly, not via RenderBlock::udpateScrollInfoAfterLayout() ... should I add clearOverflowRect to those as well? Otherwise it'll never be called for those elements right? (unless their scroll info update is delayed)

[Ojan Vafai]

(In reply to comment #15)
> Hmm one concern I have is if anything else needs the layoutOverflowRect between the time we call updateScrolInfoAfterLayout and we exit the RenderBlock::layout. (where it was called before).
> Although I guess updateScrollInfoAFTERlayout should imply that all the layout stuff has already happened and we're good to go?

Yeah. It's a valid concern. It wouldn't surprise me if there's other bugs like this one hiding here. You could add an ASSERT(!gDelayUpdateScrollInfo) to layoutOverflowRect as well.

(In reply to comment #16)
> One more thing is that RenderDeprecatedFlexibleBox and RenderGrid both call
> layer()->updateScrollInfoAfterLayout();
> directly, not via RenderBlock::udpateScrollInfoAfterLayout() ... should I add clearOverflowRect to those as well? Otherwise it'll never be called for those elements right? (unless their scroll info update is delayed)

I think those should be calling RenderBlock's udpateScrollInfoAfterLayout.

(In reply to comment #14)
> (In reply to comment #12)
> > -Not directly related to your patch, but while we're in this code...it doesn't seem like the hasControlClip check provides any value. clearLayoutOverflow already early returns if !m_overflow. I might be missing something here though.
> 
> Just looking at the method, hasControlClip seems to be unrelated to m_overflow, it's overridden in various Render classes and simply either returns true or false (in RenderMenuLists's case the implementation is just: return true;) I guess it doesn't matter if I'm removing the call from layout() altogether though :)

Hmmm...I'm not sure about this. Might want to get Hyatt to take a look at this first. Thinking about this more, it might be wrong to clear m_overflow if !hasControlClip(). I'm not sure.

[Roger Fong]

> Yeah. It's a valid concern. It wouldn't surprise me if there's other bugs like this one hiding here. You could add an ASSERT(!gDelayUpdateScrollInfo) to layoutOverflowRect as well.

Okay, yeah, I'll try that and see if I hit it anywhere.


> I think those should be calling RenderBlock's udpateScrollInfoAfterLayout.

Sounds good.

> Hmmm...I'm not sure about this. Might want to get Hyatt to take a look at this first. Thinking about this more, it might be wrong to clear m_overflow if !hasControlClip(). I'm not sure.

I added :
    if (!hasControlClip())
        return;
to the top of clearLayoutOverflow() to maintain old behaviour.

Without that there, we get some failing tests, like, fast/overflow/height-during-simplified-layout.html

[Roger Fong]

(In reply to comment #18)
> > Yeah. It's a valid concern. It wouldn't surprise me if there's other bugs like this one hiding here. You could add an ASSERT(!gDelayUpdateScrollInfo) to layoutOverflowRect as well.

I realized that gDelayUpdateScrollInfo can really be any value when calling layoutOverflowRect since calling layoutOverflowRect is independent of the delayed scroll stuff. But I did do a test where I set a flag on RenderBlock to true when clearOverflowRect is called, and toggled to false when we reach the place in layout() where clearOverflowRect was being called before. Then asserted in layoutOverflowRect that the flag was false.
Ran this test with a bunch of sites including my attached test case and I never hit the assert. So it seems fine? Maybe hyatt can give a more concrete answer.

[Roger Fong]

Created attachment 203463 [details]
patch

Still no test but I thought I'd upload the actual more finalized code changes first just in case someone spots something really wrong with it.

[Ojan Vafai]

Comment on attachment 203463 [details]
patch

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

> Source/WebCore/rendering/RenderBlock.cpp:1812
> +void RenderBlock::clearLayoutOverflow()

Maybe move the layer()->updateScrollInfoAfterLayout(); call into this function to avoid someone in the future doing one without the other.

[Roger Fong]

> Maybe move the layer()->updateScrollInfoAfterLayout(); call into this function to avoid someone in the future doing one without the other.

Should the clearLayoutOverflow be renamed then? Say to updateScrollAndClearLayoutOverflow or something?

[Ojan Vafai]

(In reply to comment #22)
> > Maybe move the layer()->updateScrollInfoAfterLayout(); call into this function to avoid someone in the future doing one without the other.
> 
> Should the clearLayoutOverflow be renamed then? Say to updateScrollAndClearLayoutOverflow or something?

Yeah. I don't feel strongly about the specific name. updateScrollAndClearLayoutOverflow seems reasonable to me.

[Roger Fong]

Created attachment 203638 [details]
with test case

[Roger Fong]

Thanks for all the help!
Committed here: http://trac.webkit.org/changeset/151146

[Tim Horton]

Roger, you broke fast/forms/control-clip-overflow.html: http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r151174%20(13036)/results.html

[Tim Horton]

And caused some assertion failures: http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r151146%20(9923)/results.html

Also updateScrollAndclearLayoutOverflow is capitalized wrong.

[Roger Fong]

Rolled out r

The assert shouldn't be there because there is a case where gDelayedScrollInfo is not expected to be 0 when calling the code (as indicated by the comment right above that case...)

I'm not sure why that test is failing but I suspect it has to do with moving the clearLayoutOverflow code.
I still believe that the fix is conceptually correct but perhaps it is safer to just keep clearLayoutOverflow where it was.

[Roger Fong]

Created attachment 203741 [details]
safer patch

This patch is safer. It moves clearLayoutOverflow back to layout() so that the only time it's called at a different time is when finishdelayedscrolling is called.
Thus clearLayoutOverflow will never get called earlier than its supposed to be.

The delayed logic is a little more spread out because of this but it's at least still all in RenderBlock :).

In addition, I ran the failing test and it does not fail locally, which apparently doesn't mean much to the bots, but it's something.

[Roger Fong]

Ojan, when you have a moment, could you take another look?
I'd like to take another stab at this and see if the bots get angry at this patch.

[Roger Fong]

Created attachment 203988 [details]
safer patch v2

same patch, updated tree

[Roger Fong]

I can download this uploaded patch, right now, apply it locally on ToT, and build just fine...

[Roger Fong]

Reopening to attach new patch.

[Roger Fong]

Created attachment 204068 [details]
Patch

[Build Bot]

Comment on attachment 204068 [details]
Patch

Attachment 204068 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/723807

New failing tests:
fast/flexbox/clear-overflow-before-scroll-update.html

[Build Bot]

Created attachment 204077 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Roger Fong]

Created attachment 204082 [details]
Patch

[Build Bot]

Comment on attachment 204082 [details]
Patch

Attachment 204082 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/807036

New failing tests:
fast/flexbox/clear-overflow-before-scroll-update.html

[Build Bot]

Created attachment 204087 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Roger Fong]

Created attachment 204093 [details]
Patch

[Darin Adler]

Comment on attachment 204093 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        clearLayoutOverflow should never be called before calling layer()->updateScrollInfoAfterLayout().

Bug title should describe the symptom, not the structural problem in the code.

> Source/WebCore/ChangeLog:15
> +        (WebCore::RenderBlock::layout): Only call clearLayoutOverflow here if we're scrolling isn't being delayed.

extra word "we're" here

> Source/WebCore/ChangeLog:20
> +        * rendering/RenderDeprecatedFlexibleBox.cpp: Should call updateScrollInfoAfterLayout, not layer()->updateScrollInfoAfterLayout()

Why? Comment needs to say why, not what is changed. And a deeper level of why than “should”.

[Roger Fong]

Committed with Changelog fixes: http://trac.webkit.org/changeset/151360