[Webkit-unassigned] [Bug 116689] Regression(r145959) Convert <select> to new flex box causes strange positioning problems

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116689


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203284|review?                     |review-
               Flag|                            |




--- Comment #12 from Ojan Vafai <ojan at chromium.org>  2013-05-29 19:49:26 PST ---
(From update of attachment 203284)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.