Bug 76834 – Relative pos. input fields in columns vanish when you start typing in them

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Oliver Kohll]

Created attachment 123566 [details]
Test case for two column relative input bug

See attachment for test case. When you click the mouse in the input field at the end of the page and start typing, it vanishes. This happens when

* it's in a div set to two columns in CSS3
* it has position: relative

Tested in Safari 5.1.2 (7534.52.7) and Google Chrome 16.0.912.75

[Oliver Kohll]

It only seems to happen when the input appears in the second column

[Pravin D]

Created attachment 145787 [details]
Patch

[Pravin D]

(In reply to comment #2)
> Created an attachment (id=145787) [details]
> Patch

About the fix:
When a layout is called for a renderer contained inside a multi column container, if it has a separate renderlayer , then updateLayerPositions() is called. Here the pagination bit is always set to false which is not proper.

The patch calls updatePagination() if the pagination bit(m_isPaginated) is already set. The logic here is that if the m_isPaginated is already set to true, then it must have been part of a multi column container. So instead of just setting it false call updatePagination().

[Julien Chaffraix]

Comment on attachment 145787 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:365
> +    if ((flags & UpdatePagination) || m_isPaginated)

I think it would be better to set the UpdatePagination flag in FrameView::layout. The issue is that we are doing a subtree relayout and the code makes the assumption that we would cross our parent with columns when doing the recursive descent and properly set this flag.

There is 2 upsides to using the flag:
* takes care of new nodes as m_isPaginated will be false on them.
* the new check is unneeded in full layout mode as m_isPaginated will be false or UpdatePagination will be properly set.

[Pravin D]

(In reply to comment #4)
> (From update of attachment 145787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145787&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:365
> > +    if ((flags & UpdatePagination) || m_isPaginated)
> 
> I think it would be better to set the UpdatePagination flag in FrameView::layout. The issue is that we are doing a subtree relayout and the code makes the assumption that we would cross our parent with columns when doing the recursive descent and properly set this flag.
> 
Thanks for the review :)

> There is 2 upsides to using the flag:
> * takes care of new nodes as m_isPaginated will be false on them.
>
1 doubt... 
 Two cases for inserting a new node:
1) A node is added to the parent(node having multiple column). Then  UpdatePagination will be set and we have no problem.
2) A node is added to the current node's subtree. If the current node does not have multiple columns then m_isPaginated is by default FALSE on its children and does not req. updatation. If on the other hand the current node has multiple columns then its UpdatePagination will be set for its children from RenderLayer::updateLayerPositions() (we cross our parent with columns senario).

So this shud take care of updating the values of m_isPaginated for all children right ? Or I'm missing something?

This being my first patch on Multi-cols, just trying to check if my understanding is correct :)

> * the new check is unneeded in full layout mode as m_isPaginated will be false or UpdatePagination will be properly set.
>
I guess it makes more sense to call set the UpdatePagination flag then use m_isPaginated.

I'll upload another patch with the changes that u have suggested.

[Pravin D]

Created attachment 146533 [details]
Proposed Patch

[Pravin D]

Comment on attachment 146533 [details]
Proposed Patch

UpdatePagination flag is added to the flag list passed to RenderLayer::updatePositions() in FrameView::layout().

[Julien Chaffraix]

> > There is 2 upsides to using the flag:
> > * takes care of new nodes as m_isPaginated will be false on them.
> >
> 1 doubt... 
>  Two cases for inserting a new node:
> 1) A node is added to the parent(node having multiple column). Then  UpdatePagination will be set and we have no problem.
> 2) A node is added to the current node's subtree. If the current node does not have multiple columns then m_isPaginated is by default FALSE on its children and does not req. updatation. If on the other hand the current node has multiple columns then its UpdatePagination will be set for its children from RenderLayer::updateLayerPositions() (we cross our parent with columns senario).
> 
> So this shud take care of updating the values of m_isPaginated for all children right ? Or I'm missing something?

Yes, if you look at updatePagination, the whole subtree defined by the renderer with columns can have m_isPaginated set. Note that updateLayerPositions makes this assumption too by setting a flag to recompute the m_isPaginated on the whole subtree. If you add a child in this subtree, there is no guarantee that you will relayout the whole subtree (this is the root cause of this bug). With the original fix, you wouldn't recompute m_isPaginated for this child and thus may forget to set it as expected.

The condition for that to happen are likely difficult to reach and very fragile which is why I didn't ask for a test case for that.

[Julien Chaffraix]

Comment on attachment 146533 [details]
Proposed Patch

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

> Source/WebCore/page/FrameView.cpp:1121
> +                                (m_doFullRepaint ? 0 : RenderLayer::CheckForRepaint | RenderLayer::UpdatePagination)

It's not totally right: m_doFullRepaint doesn't properly cover your case here. You should only set UpdatePagination if you are doing a subtree relayout (subtree) and the layer has the m_isPaginated flag set.

Please move the flag setting outside the function call (maybe in a new function) as it will start to get messy.

[Pravin D]

(In reply to comment #9)
> (From update of attachment 146533 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146533&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1121
> > +                                (m_doFullRepaint ? 0 : RenderLayer::CheckForRepaint | RenderLayer::UpdatePagination)
> 
> It's not totally right: m_doFullRepaint doesn't properly cover your case here. You should only set UpdatePagination if you are doing a subtree relayout (subtree) and the layer has the m_isPaginated flag set.
> 
> Please move the flag setting outside the function call (maybe in a new function) as it will start to get messy.
>

I'll upload another patch with the suggested changes....

[Pravin D]

Created attachment 146699 [details]
Patch

[Pravin D]

(In reply to comment #9)
> (From update of attachment 146533 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146533&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1121
> > +                                (m_doFullRepaint ? 0 : RenderLayer::CheckForRepaint | RenderLayer::UpdatePagination)
> 
> It's not totally right: m_doFullRepaint doesn't properly cover your case here. You should only set UpdatePagination if you are doing a subtree relayout (subtree) and the layer has the m_isPaginated flag set.
> 

Inside FrameView::layout m_layoutRoot valid only if its a subtree relayout. Also the local variable bool subtree is true if m_layoutRoot is valid. So the variable subtree is used to determine it its a subtree relayout or not. Also RenderLayer* is passed to updateLayerPositionsFlagsForLayer() to determine if the layer is paginated or not. 

Reason for passing RenderLayer* 1) keep the logic for setting UpdatePagination flag withing updateLayerPositionsFlagsForLayer() and also in future if more flags are added to RenderLayer, then this layer pointer will be useful.

> Please move the flag setting outside the function call (maybe in a new function) as it will start to get messy.
> 

static helper function updateLayerPositionsFlagsForLayer() added to prepare the UpdateLayerPositionsFlags.

[Julien Chaffraix]

Comment on attachment 146699 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:125
> +static RenderLayer::UpdateLayerPositionsFlags updateLayerPositionsFlagsForLayer(RenderLayer* layer, bool subtreeRelayout, bool fullRepaint)

Not a huge fan of updateLayerPositionsFlagsForLayer, alternative names: flagsForUpdateLayerPosition, updateLayerPositionFlags. I would pick the second as it's shorter.

Note that WebKit style guide says that boolean should have a verb in them so the naming could be enhanced:
* I would rename |fullRepaint| to |didFullRepaint| for completeness
* |subtreeRelayout| -> isRelayoutingSubtree, isLayoutingSubtree, didSubtreeRelayout

> Source/WebCore/page/FrameView.cpp:130
> +    if (subtreeRelayout && layer && layer->isPaginated())

layer cannot be NULL.

[Pravin D]

(In reply to comment #13)
> (From update of attachment 146699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146699&action=review
> 
> > Source/WebCore/page/FrameView.cpp:125
> > +static RenderLayer::UpdateLayerPositionsFlags updateLayerPositionsFlagsForLayer(RenderLayer* layer, bool subtreeRelayout, bool fullRepaint)
> 
> Not a huge fan of updateLayerPositionsFlagsForLayer, alternative names: flagsForUpdateLayerPosition, updateLayerPositionFlags. I would pick the second as it's shorter.
> 
UpdateLayerPositionsFlags is a typedef used in RenderLayer class. So is it ok to use updateLayerPositionFlags() ??

> Note that WebKit style guide says that boolean should have a verb in them so the naming could be enhanced:
> * |subtreeRelayout| -> isRelayoutingSubtree, isLayoutingSubtree, didSubtreeRelayout
> 
Is it ok if I use isInSubtreeRelayout instead ?

[Julien Chaffraix]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 146699 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146699&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:125
> > > +static RenderLayer::UpdateLayerPositionsFlags updateLayerPositionsFlagsForLayer(RenderLayer* layer, bool subtreeRelayout, bool fullRepaint)
> > 
> > Not a huge fan of updateLayerPositionsFlagsForLayer, alternative names: flagsForUpdateLayerPosition, updateLayerPositionFlags. I would pick the second as it's shorter.
> > 
> UpdateLayerPositionsFlags is a typedef used in RenderLayer class. So is it ok to use updateLayerPositionFlags() ??

Not sure if the compiler would let you do that. You have other names in case.

> > Note that WebKit style guide says that boolean should have a verb in them so the naming could be enhanced:
> > * |subtreeRelayout| -> isRelayoutingSubtree, isLayoutingSubtree, didSubtreeRelayout
> > 
> Is it ok if I use isInSubtreeRelayout instead ?

SGTM.

[Pravin D]

Created attachment 146869 [details]
Patch

[Pravin D]

(In reply to comment #16)
> Created an attachment (id=146869) [details]
> Patch

Patch contains the suggested changes in review comment #13

[WebKit Review Bot]

Comment on attachment 146869 [details]
Patch

Clearing flags on attachment: 146869

Committed r119996: <http://trac.webkit.org/changeset/119996>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Kohll]

Thanks guys, nice one.