Bug 76834 - Relative pos. input fields in columns vanish when you start typing in them
Summary: Relative pos. input fields in columns vanish when you start typing in them
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Major
Assignee: Pravin D
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-23 08:50 PST by Oliver Kohll
Modified: 2012-06-11 13:46 PDT (History)
6 users (show)

See Also:


Attachments
Test case for two column relative input bug (882 bytes, text/html)
2012-01-23 08:50 PST, Oliver Kohll
no flags Details
Patch (4.48 KB, patch)
2012-06-05 07:28 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Proposed Patch (4.68 KB, patch)
2012-06-08 03:27 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (7.12 KB, patch)
2012-06-09 06:26 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (7.07 KB, patch)
2012-06-11 10:05 PDT, Pravin D
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Kohll 2012-01-23 08:50:41 PST
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
Comment 1 Oliver Kohll 2012-01-23 09:05:47 PST
It only seems to happen when the input appears in the second column
Comment 2 Pravin D 2012-06-05 07:28:17 PDT
Created attachment 145787 [details]
Patch
Comment 3 Pravin D 2012-06-05 07:35:07 PDT
(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().
Comment 4 Julien Chaffraix 2012-06-07 16:33:05 PDT
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.
Comment 5 Pravin D 2012-06-08 01:30:46 PDT
(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.
Comment 6 Pravin D 2012-06-08 03:27:24 PDT
Created attachment 146533 [details]
Proposed Patch
Comment 7 Pravin D 2012-06-08 03:29:37 PDT
Comment on attachment 146533 [details]
Proposed Patch

UpdatePagination flag is added to the flag list passed to RenderLayer::updatePositions() in FrameView::layout().
Comment 8 Julien Chaffraix 2012-06-08 09:36:25 PDT
> > 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.
Comment 9 Julien Chaffraix 2012-06-08 09:43:03 PDT
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.
Comment 10 Pravin D 2012-06-08 21:13:33 PDT
(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....
Comment 11 Pravin D 2012-06-09 06:26:16 PDT
Created attachment 146699 [details]
Patch
Comment 12 Pravin D 2012-06-09 06:38:05 PDT
(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.
Comment 13 Julien Chaffraix 2012-06-11 09:06:42 PDT
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.
Comment 14 Pravin D 2012-06-11 09:28:25 PDT
(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 ?
Comment 15 Julien Chaffraix 2012-06-11 09:50:12 PDT
(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.
Comment 16 Pravin D 2012-06-11 10:05:05 PDT
Created attachment 146869 [details]
Patch
Comment 17 Pravin D 2012-06-11 10:06:25 PDT
(In reply to comment #16)
> Created an attachment (id=146869) [details]
> Patch

Patch contains the suggested changes in review comment #13
Comment 18 WebKit Review Bot 2012-06-11 11:49:04 PDT
Comment on attachment 146869 [details]
Patch

Clearing flags on attachment: 146869

Committed r119996: <http://trac.webkit.org/changeset/119996>
Comment 19 WebKit Review Bot 2012-06-11 11:49:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Oliver Kohll 2012-06-11 13:46:05 PDT
Thanks guys, nice one.