Bug 72852 – Scroll position is lost after hide/show element

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Naveen Bobbili]

Created attachment 116027 [details]
Scroll Bug

What steps will reproduce the problem?
1. add element with active vertical scroll (big div inside small div)
2. scroll to any position
3. hide element (set style.display='none')
4. show element (set style.display='block')

What is the expected result?
 scroll position was not changed

What happens instead?
 scroll position is reset

Use attached html to reproduce.

[Naveen Bobbili]

To achieve this we need to maintain the old scroll position. As renderer and the associated layer gets dropped after hide operation, where can we maintain the old scroll position?

[Rakesh]

Created attachment 116563 [details]
Proposed patch

We maintain a map of node and scroll point in Page(similar to ScrollableAreaSet). When the RenderLayer is getting dropped (for example display none), we store the location with node. When the same node get a renderer, we set scrolloffset to previous value stored.

[Alexey Proskuryakov]

What if the element gets moved to another page while hidden?

[Rakesh]

(In reply to comment #3)
> What if the element gets moved to another page while hidden?
In this case I think it should be rendered to the default scroll position than the stored prev. position as I guess it would be altogether another DOM itself(I may be wrong, please correct me if so).

[Alexey Proskuryakov]

One not so great thing that would happen in this case is that m_scrollableNodeMap will have abandoned data for the node.

[Rakesh]

(In reply to comment #5)
> One not so great thing that would happen in this case is that m_scrollableNodeMap will have abandoned data for the node.

Yes, you are right, I think we need to remove the data when node is detached but with this any style change will lead to losing the scroll position. What do you  suggest, we can maintain scroll position till the time node is detached?

[Rakesh]

(In reply to comment #5)
> One not so great thing that would happen in this case is that m_scrollableNodeMap will have abandoned data for the node.

After some debugging, I think following two approaches are possible: 

1. Instead of page, document should hold this map(ScrollableNodePoint). We can remove the reference of Node in node's destructor.

2. Have a raredata to hold the scroll position in the node itself which can be directly used.

In both the approaches, to handle the case of node being moved to another page, we can reset the scroll position in setDocument() of Node.

Please let me know your thoughts.

[Rakesh]

Just for information, this issue works fine on IE and Firefox for display:none and visibility:hidden as well.

[Simon Fraser (smfr)]

I'm not sure that it's appropriate to store the scroll position across a display:none change. Authors should use visibility:hidden if they want the scroll position to be remembered.

[Simon Fraser (smfr)]

Comment on attachment 116563 [details]
Proposed patch

There's no guarantee that the style will be the same after the display:none -> display:block change, so the point may be just wrong. For example, imagine that all the rows got taller.

[Alexey Proskuryakov]

> I'm not sure that it's appropriate to store the scroll position across a display:none change

I was also unsure, but if it works in both IE and Firefox, maybe we should comply. Also, there is quite a number of complaints in <http://code.google.com/p/chromium/issues/detail?id=36428>, which Rakesh pointed me to on IRC.

[Rakesh]

Created attachment 116900 [details]
Test case if height/rows are increased.

[Rakesh]

(In reply to comment #10)
> (From update of attachment 116563 [details])
> There's no guarantee that the style will be the same after the display:none -> display:block change, so the point may be just wrong. For example, imagine that all the rows got taller.

For this case, both IE and firefox restore to the old position even though height is increased as long as the stored position + height is less than the scrollable height.

This can be verified in the new test case attached. May be we can have this behavior to match other browsers.

[Rakesh]

(In reply to comment #13)
> (In reply to comment #10)
> > (From update of attachment 116563 [details] [details])
> > There's no guarantee that the style will be the same after the display:none -> display:block change, so the point may be just wrong. For example, imagine that all the rows got taller.
> 
> For this case, both IE and firefox restore to the old position even though height is increased as long as the stored position + height is less than the scrollable height.
> 
> This can be verified in the new test case attached. May be we can have this behavior to match other browsers.

Also this change of row height does apply for an visibility:hidden also, it does not restore to the previous position but a changed scroll position when it was hidden after scrolling to end of scrollable area and height is increased and  visibility:visible is applied.

[Rakesh]

Created attachment 117176 [details]
Proposed patch

As the scrollable node can be removed from current DOM, now removing the stored scroll position when the node is getting deleted or moving to a new document. Maintaining the ScrollableNodePosition map in Document. We can as well store the position in Node rareData but will increase the size of Node

[WebKit Review Bot]

Comment on attachment 117176 [details]
Proposed patch

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

New failing tests:
fast/layers/inline-dirty-z-order-lists.html
fast/flexbox/line-clamp-crash.html

[Rakesh]

Created attachment 117355 [details]
Updated patch

I think the tree was red and was getting failures on this patch.

[WebKit Review Bot]

Comment on attachment 117355 [details]
Updated patch

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

New failing tests:
fast/layers/inline-dirty-z-order-lists.html
fast/flexbox/line-clamp-crash.html

[Hajime Morrita]

Comment on attachment 117355 [details]
Updated patch

- How about <iframe> ? In general, scroll relate change should have tests which cover both overflowed-div and iframe.
- Is it possible to use Element instead of Node? it would be clarify the intention.
- Is RenderLayer::scrollToOffset() really right timing to do it? My naive thought is that we can just save/restore it on the construction/destruction of the layer or the scrollbar.

[Rakesh]

(In reply to comment #19)
> (From update of attachment 117355 [details])
> - How about <iframe> ? In general, scroll relate change should have tests which cover both overflowed-div and iframe.

Yes, will cover iframe test case too.

> - Is it possible to use Element instead of Node? it would be clarify the intention.

Element is fine.

> - Is RenderLayer::scrollToOffset() really right timing to do it? My naive thought is that we can just save/restore it on the construction/destruction of the layer or the scrollbar.

Seems the correct place to me (I may be wrong) as the ScrollableArea::scrollToOffsetWithoutAnimation() gets called from here which set the scrolloffset.

[Rakesh]

Created attachment 117883 [details]
Updated patch

Using element to store the scroll position.

[Hajime Morrita]

Comment on attachment 117883 [details]
Updated patch

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

> Source/WebCore/dom/Element.cpp:2035
> +    return ensureRareData()->scrollableElement();

Please be conservative about instantiating ElementRareData object. you can check its existence using hasRareData().

> Source/WebCore/dom/Element.cpp:2040
> +    return ensureRareData()->scrolledPosition();

Same as above.

> Source/WebCore/dom/Element.cpp:2050
> +    ensureRareData()->setScrollPosition(IntPoint());

Same as above.

> Source/WebCore/dom/Element.h:369
> +    IntPoint scrolledPosition();

The method should be const

> Source/WebCore/dom/Element.h:370
> +    void setScrollPosition(IntPoint);

The parameter should be a const reference.

> Source/WebCore/dom/ElementRareData.h:55
> +    IntPoint scrollPosition;

Please follow the naming convention.
Having this in ElementRareData seems good idea for me.

> Source/WebCore/dom/ElementRareData.h:57
> +    inline IntPoint scrolledPosition() { return scrollPosition; }

Should return const reference. the method should be const too.

> Source/WebCore/dom/ElementRareData.h:58
> +    inline void setScrollPosition(IntPoint point) { scrollPosition = point; } 

the paramter should be const.

> Source/WebCore/dom/Node.cpp:461
> +        if (toElement(this)->scrollableElement())

You can do this check inside resetScrollPosition().

> Source/WebCore/rendering/RenderLayer.cpp:1382
> +            element->resetScrollPosition();

This reset should happen outside the if (box) clause.

> LayoutTests/ChangeLog:10
> +

Could you add another test case that verify the style recalculation invalidate the scroll postion cache?
(That is for covering the change on RenderLayer::styleChanged())

[Hajime Morrita]

> > - Is RenderLayer::scrollToOffset() really right timing to do it? My naive thought is that we can just save/restore it on the construction/destruction of the layer or the scrollbar.
> 
> Seems the correct place to me (I may be wrong) as the ScrollableArea::scrollToOffsetWithoutAnimation() gets called from here which set the scrolloffset.
It's called really often from many places.
I guess there are more natural place to do it.
For example, RenderLayer::updateScrollInfoAfterLayout() is possible candidate.

Anyway you'd better to ask an expert like smfr@. you can catch him at IRC, I think.

[Simon Fraser (smfr)]

(In reply to comment #23)
> > > - Is RenderLayer::scrollToOffset() really right timing to do it? My naive thought is that we can just save/restore it on the construction/destruction of the layer or the scrollbar.
> > 
> > Seems the correct place to me (I may be wrong) as the ScrollableArea::scrollToOffsetWithoutAnimation() gets called from here which set the scrolloffset.
> It's called really often from many places.
> I guess there are more natural place to do it.
> For example, RenderLayer::updateScrollInfoAfterLayout() is possible candidate.

That's probably OK, but does that get called after the user scrolls?

[Hajime Morrita]

> > It's called really often from many places.
> > I guess there are more natural place to do it.
> > For example, RenderLayer::updateScrollInfoAfterLayout() is possible candidate.
> 
> That's probably OK, but does that get called after the user scrolls?
No.
In my understanding, restoring the position from associated element isn't required after the user scrolls.
It has to happen just after the element becomes visible.

For me, calling scrollTo() looks more straightforward than hooking it.
But I'm not so confident. I agree that hooking scrollTo() will work
because it's a gateway of the scrolls.

[Rakesh]

(In reply to comment #24)
> > For example, RenderLayer::updateScrollInfoAfterLayout() is possible candidate.
> 
> That's probably OK, but does that get called after the user scrolls?

Yes, RenderLayer::updateScrollInfoAfterLayout looks better. It does not get called when user scrolls but only when layout occurs.

(In reply to comment #25)
> No.
> In my understanding, restoring the position from associated element isn't required after the user scrolls.
> It has to happen just after the element becomes visible.

If we hook in at RenderLayer::updateScrollInfoAfterLayout which is called on layout then I think we are restoring as soon as the element becomes visible.

> 
> For me, calling scrollTo() looks more straightforward than hooking it.
> But I'm not so confident. I agree that hooking scrollTo() will work
> because it's a gateway of the scrolls.

updateScrollInfoAfterLayout does call scrollTo() internally.

[Rakesh]

Created attachment 118183 [details]
Updated patch

Changes as per review comments and calling restoring the scroll position in RenderLayer::updateScrollInfoAfterLayout(). Updated test case to include test for overflow style change also.

[Julien Chaffraix]

Comment on attachment 118183 [details]
Updated patch

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

You really need more testing for us to see that you won't break scrolling after resetting your visibility.

> Source/WebCore/dom/Element.cpp:2035
> +    return hasRareData() ? rareData()->scrollableElement() : false;

It should be:

return hasRareData() && rareData()->scrollableElement();

> Source/WebCore/dom/Element.cpp:2045
> +    ElementRareData* data = hasRareData() ? rareData() : ensureRareData();

No need for checking hasRareData here. Just do:

ensureRareData()->setScrollPosition(point);

> Source/WebCore/dom/Element.cpp:2052
> +        rareData()->setScrollPosition(IntPoint());

The whole logic is flawed. If you set up scrolledPosition() once, you will always reset the scrollOffset in updateScrollInfoAfterLayout to something (IntPoint() if you have called reset in the meantime) thus overriding a scroll offset set by JS or the user. This is not covered by your tests unfortunately.

> Source/WebCore/dom/Node.cpp:461
> +        toElement(this)->resetScrollPosition();

There is nothing that warranties that |this| is an Element AFAICT. This will crash pretty badly if you move a node from one Document to another one.

> Source/WebCore/rendering/RenderLayer.cpp:2275
> +    }

It really looks like an overkill to check for that at every layout. I would feel better if we restored it in the constructor of RenderLayer as to reset the scrollOffset once.

> Source/WebCore/rendering/RenderLayer.cpp:4297
> +        toElement(m_renderer->node())->resetScrollPosition();

Again what guarantees our node to be an Element?

> LayoutTests/fast/overflow/scroll-div-hide-show-expected.txt:19
> +FAIL e.scrollTop should be 100. Was 0.

This is failing, you need to mention why and we *need* a bug tracking this failure at some point as it will go unnoticed in our tree.

[Rakesh]

Comment on attachment 118183 [details]
Updated patch


> return hasRareData() && rareData()->scrollableElement();

Agreed.

> No need for checking hasRareData here. Just do:
> 
> ensureRareData()->setScrollPosition(point);

Agreed.

>> Source/WebCore/dom/Element.cpp:2052
>> +        rareData()->setScrollPosition(IntPoint());
> 
> The whole logic is flawed. If you set up scrolledPosition() once, you will always reset the scrollOffset in updateScrollInfoAfterLayout to something (IntPoint() if you have called reset in the meantime) thus overriding a scroll offset set by JS or the user. This is not covered by your tests unfortunately.

I did not get "(IntPoint() if you have called reset in the meantime) thus overriding a scroll offset set by JS or the user". We want to reset the scrolloffset once we have restored as we will store a new position if user scrolls to new offset and then layer is dropped due to display:none. May be you meant some thing else.

>> Source/WebCore/dom/Node.cpp:461
>> +        toElement(this)->resetScrollPosition();
> 
> There is nothing that warranties that |this| is an Element AFAICT. This will crash pretty badly if you move a node from one Document to another one.

Yes, a check is needed here.

>> Source/WebCore/rendering/RenderLayer.cpp:2275
>> +    }
> 
> It really looks like an overkill to check for that at every layout. I would feel better if we restored it in the constructor of RenderLayer as to reset the scrollOffset once.

Will set scrolloffset in constructor.

>> Source/WebCore/rendering/RenderLayer.cpp:4297
>> +        toElement(m_renderer->node())->resetScrollPosition();
> 
> Again what guarantees our node to be an Element?

Will add a check.

>> LayoutTests/fast/overflow/scroll-div-hide-show-expected.txt:19
>> +FAIL e.scrollTop should be 100. Was 0.
> 
> This is failing, you need to mention why and we *need* a bug tracking this failure at some point as it will go unnoticed in our tree.

My mistake, expected.txt got generated on wrong build, I compiled for gtk and ran new-run-webkit-tests for chromium.

[Rakesh]

Created attachment 118338 [details]
Updated patch

Changes as per review comments and restore scrolloffset in constructor.

[Julien Chaffraix]

Comment on attachment 118183 [details]
Updated patch

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

>>> Source/WebCore/dom/Element.cpp:2052
>>> +        rareData()->setScrollPosition(IntPoint());
>> 
>> The whole logic is flawed. If you set up scrolledPosition() once, you will always reset the scrollOffset in updateScrollInfoAfterLayout to something (IntPoint() if you have called reset in the meantime) thus overriding a scroll offset set by JS or the user. This is not covered by your tests unfortunately.
> 
> I did not get "(IntPoint() if you have called reset in the meantime) thus overriding a scroll offset set by JS or the user". We want to reset the scrolloffset once we have restored as we will store a new position if user scrolls to new offset and then layer is dropped due to display:none. May be you meant some thing else.

Looking again, I missed a bit - that you checked scrolledPosition against IntPoint() in scrollableElement() before resetting m_scrollOffset - and the logic was fine. My bad.

[Julien Chaffraix]

Comment on attachment 118338 [details]
Updated patch

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

You *really* need more testing here. Each review brings up some bugs in the implementation that are not caught by any test. It would be neat to come up with some new test case covering the pointed bug but at least *more* testing would give reviewers some confidence that this works outside a very basic example (which is your current test case). Here is some ideas:

* toggling several times visibility should work. Changing the value in between should also work. Make sure m_scrollOffset.x() != m_scrollOffset.y() as this would fine flipping issues.
* look at the different code path and come with a test case (example: the Node::setDocument change is not tested AFAICT, RenderLayer::styleChanged too).
* use some non default writing modes and direction in a test.
* vary your scroll origin (see RenderLayer::computeScrollDimensions to see what impacts it).

> Source/WebCore/ChangeLog:8
> +        Maintain the scroll position of overflow element when the scrollable renderlayer is being dropped,

overflowing element?

s/renderlayer/RenderLayer/

dropped -> destroyed?

> Source/WebCore/ChangeLog:9
> +        which can be used to restore the scroll position if same node is again added to render tree

node -> element

added to *the* render tree

> Source/WebCore/ChangeLog:19
> +        Helper functions for accessing scroll position from rare data.

It would help readability if you added an empty line between the different groups covered by a single comment.

> Source/WebCore/dom/Element.cpp:2040
> +    return hasRareData() ? rareData()->scrolledPosition() : IntPoint();

Looking at the rest of the code, it is not fine to call scrolledPosition if we don't have some rareData. If that's the case, then this should just be:

ASSERT(hasRareData());
return rareData()->scrolledPosition();

> Source/WebCore/dom/Element.h:370
> +    void setScrollPosition(const IntPoint& = IntPoint());

Please remove the default argument, it is confusing and a source of badness in this case.

> Source/WebCore/dom/ElementRareData.h:56
> +    inline bool scrollableElement() { return (m_scrolledPosition != IntPoint()); }

You can use IntPoint::zero() for the comparison here.

> Source/WebCore/dom/ElementRareData.h:58
> +    inline void setScrollPosition(const IntPoint& point) { m_scrolledPosition = point; } 

You don't have to use the |inline| keyword in the previous functions. Any function defined in a header is automatically inlined by the compiler.

> Source/WebCore/rendering/RenderLayer.cpp:201
> +            m_scrollOffset = toSize(element->scrolledPosition());

I think this will break if scrollOrigin() is different from (0, 0) (and any non-English combinaison of writing mode and directions). You are saving scrollPosition() into your rare data field which included the scrollOrigin() *and* m_scrollOffset. However you restore everything into m_scrollOffset. This means that now scrollPosition will include scrollOrigin() twice.

[Rakesh]

> * look at the different code path and come with a test case (example: the Node::setDocument change is not tested AFAICT, RenderLayer::styleChanged too).

Will add a test for Node::setDocument change. I think change in RenderLayer::styleChanged is not required as even if overflow is not there then setting a scrolloffset does not have any effect.

> * use some non default writing modes and direction in a test.
> * vary your scroll origin (see RenderLayer::computeScrollDimensions to see what impacts it).
> 

Yes, with change in direction will change scrollOrigin also. Will add that test too.

> > Source/WebCore/ChangeLog:19
> > +        Helper functions for accessing scroll position from rare data.
> 
> It would help readability if you added an empty line between the different groups covered by a single comment.

I am ok to add empty line but I haven't seen that in ChangeLog's so is that fine?

> > Source/WebCore/rendering/RenderLayer.cpp:201
> > +            m_scrollOffset = toSize(element->scrolledPosition());
> 
> I think this will break if scrollOrigin() is different from (0, 0) (and any non-English combinaison of writing mode and directions). You are saving scrollPosition() into your rare data field which included the scrollOrigin() *and* m_scrollOffset. However you restore everything into m_scrollOffset. This means that now scrollPosition will include scrollOrigin() twice.

I think saving m_scrollOffset instead of scrollPosition() will solve this issue. Scrolling to appropriate point wrt to origin will be taken care in scrollTo(), what do you think? Also renaming the functions as setScrolledOffset if we are saving scrollOffset will be good.

[Julien Chaffraix]

(In reply to comment #33)
> I think change in RenderLayer::styleChanged is not required as even if overflow is not there then setting a scrolloffset does not have any effect.

Good catch, I thought it may be needed in case your style makes you not scrollable anymore. As we save / restore on layer's creation / deletion, it shouldn't be needed.

> > > Source/WebCore/ChangeLog:19
> > > +        Helper functions for accessing scroll position from rare data.
> > 
> > It would help readability if you added an empty line between the different groups covered by a single comment.
> 
> I am ok to add empty line but I haven't seen that in ChangeLog's so is that fine?

Look at other people's ChangeLog entries, most people don't feel the details like you do. Among those who do, they usually add some empty lines for readability. ChangeLog entries vary wildly by people so there is not a lot of official rules as to what to put in it and how to present it.

> > > Source/WebCore/rendering/RenderLayer.cpp:201
> > > +            m_scrollOffset = toSize(element->scrolledPosition());
> > 
> > I think this will break if scrollOrigin() is different from (0, 0) (and any non-English combinaison of writing mode and directions). You are saving scrollPosition() into your rare data field which included the scrollOrigin() *and* m_scrollOffset. However you restore everything into m_scrollOffset. This means that now scrollPosition will include scrollOrigin() twice.
> 
> I think saving m_scrollOffset instead of scrollPosition() will solve this issue. Scrolling to appropriate point wrt to origin will be taken care in scrollTo(), what do you think? 

I would think so, hoping that computeScrollDimensions reset all the other m_*scroll* members to the original dimensions. Note that this is an optimistic approach: if any of those changes, it's potentially game over. I think this trade-off is fine for a first step.

> Also renaming the functions as setScrolledOffset if we are saving scrollOffset will be good.

I don't think I follow this renaming. Seeing the change would make me see what you mean.

[Rakesh]

Created attachment 118694 [details]
Updated patch

More test coverage and suggested changes.

[Julien Chaffraix]

Comment on attachment 118694 [details]
Updated patch

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

More comments / questions / nits but you have the hang of it and I think the next patch should be good to land.

> Source/WebCore/ChangeLog:8
> +        Maintain the scroll position of overflowing element when the scrollable RenderLayer is being destroyed,

s/is being destroyed/is destroyed/ (I am not a native English speaker but the 'being' seems unnecessary)

> Source/WebCore/dom/Element.cpp:2050
> +    ASSERT(hasRareData());

Looking further, it looks like it is bad to scrolledPosition if scrollableElement() is false. Thus the ASSERT could be:

ASSERT(scrollableElement());

which covers and broadens the existing ASSERT.

> Source/WebCore/dom/ElementRareData.h:58
> +    bool scrollableElement() { return (m_scrolledPosition != IntPoint::zero()); }
> +    IntPoint scrolledPosition() const { return m_scrolledPosition; }
> +    void setScrollPosition(const IntPoint& point) { m_scrolledPosition = point; } 

It looks like all those functions should just be removed and inlined in the only user Element.

> Source/WebCore/dom/Node.cpp:463
> +            toElement(this)->resetScrollPosition();

I don't see that exercised in your test, am I missing something?

> Source/WebCore/rendering/RenderLayer.cpp:201
> +            m_scrollOffset = toSize(element->scrolledPosition());

Could you put a comment here about your approach being optimistic when storing only the scrolled position? (it would be really better would be to enforce this an ASSERT but a FIXME will do)

> LayoutTests/fast/overflow/scroll-div-hide-show.html:43
> +e.scrollTop = 50;
> +e.scrollLeft = 50;

Nit: Those 2 values should be different.

> LayoutTests/fast/overflow/scroll-div-hide-show.html:129
> +debug("Scrolling to 50,150");

Nit: Space after the comma.

> LayoutTests/fast/overflow/scroll-div-hide-show.html:151
> +debug("Scrolling Iframe to (50,50)");
> +frame.contentWindow.scrollTo(50,50);

Nit: Those 2 values should be different. Also space after the comma.

> LayoutTests/fast/overflow/scroll-div-hide-show.html:168
> +debug("Scrolling div to (50,50)");
> +e.scrollTop = 50;
> +e.scrollLeft = 50;

Nit: Ditto.

[Rakesh]

Comment on attachment 118694 [details]
Updated patch

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

>> Source/WebCore/dom/ElementRareData.h:58
>> +    void setScrollPosition(const IntPoint& point) { m_scrolledPosition = point; } 
> 
> It looks like all those functions should just be removed and inlined in the only user Element.

I think we may have to include ElementRareData.h in Element.h to make these inline. Better we can have these defined in Element.cpp.

>> Source/WebCore/dom/Node.cpp:463
>> +            toElement(this)->resetScrollPosition();
> 
> I don't see that exercised in your test, am I missing something?

Test for this is added, it has following comments: "Testing scroll offset getting reset when moved to other document"

>> LayoutTests/fast/overflow/scroll-div-hide-show.html:43
>> +e.scrollLeft = 50;
> 
> Nit: Those 2 values should be different.

It is one of the test with same values, the other tests below have different values.

[Julien Chaffraix]

Comment on attachment 118694 [details]
Updated patch

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

>>> Source/WebCore/dom/ElementRareData.h:58
>>> +    void setScrollPosition(const IntPoint& point) { m_scrolledPosition = point; } 
>> 
>> It looks like all those functions should just be removed and inlined in the only user Element.
> 
> I think we may have to include ElementRareData.h in Element.h to make these inline. Better we can have these defined in Element.cpp.

I was following this exact line of thoughts. It looks like we don't define any function on ElementRareData and the previous functions can easily be implemented in Element.cpp without losing some abstraction.

>>> Source/WebCore/dom/Node.cpp:463
>>> +            toElement(this)->resetScrollPosition();
>> 
>> I don't see that exercised in your test, am I missing something?
> 
> Test for this is added, it has following comments: "Testing scroll offset getting reset when moved to other document"

OK, sorry I missed it.

>>> LayoutTests/fast/overflow/scroll-div-hide-show.html:43
>>> +e.scrollLeft = 50;
>> 
>> Nit: Those 2 values should be different.
> 
> It is one of the test with same values, the other tests below have different values.

FYI nits can usually be ignored. They just represents comments the reviewer does not feel strongly about (in this case, I am fine with the test case having the same values).

[Rakesh]

Created attachment 118957 [details]
Updated patch

[Julien Chaffraix]

Comment on attachment 118957 [details]
Updated patch

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

> Source/WebCore/dom/Element.cpp:2035
> +    return hasRareData() && (rareData()->m_scrolledPosition != IntPoint::zero());

Extra parenthesis here.

> Source/WebCore/rendering/RenderLayer.cpp:202
> +            // We save and restore only the scrollOffset as the other scroll
> +            // values are recalulated.

That comment is good. I would also like to see one about us being optimistic somewhere as the recalculated values may not match the old ones and thus scrollOffset would be wrong (for example, would need to be clamped). Here is an updated comment:

// We save and restore only the scrollOffset as the other scroll values are recalulated.
// FIXME: We are assuming those scroll values did not change. If they do, our cached scrollOffset may be wrong.

[Rakesh]

Created attachment 119057 [details]
Updated patch

Made necessary changes

[Julien Chaffraix]

Comment on attachment 119057 [details]
Updated patch

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

> LayoutTests/ChangeLog:9
> +        Maintain the scroll position of overflowing element when the scrollable RenderLayer is destroyed,
> +        which can be used to restore the scroll position if the same element is again added to the render tree. 

Damn super close: this should be in the WebCore ChangeLog as it is pretty much useless in the LayoutTest one. I also really think this should be moved before landing :(

[Rakesh]

Created attachment 119240 [details]
Updated patch

modified ChangeLogs

[Rakesh]

> Damn super close: this should be in the WebCore ChangeLog as it is pretty much useless in the LayoutTest one. I also really think this should be moved before landing :(

How did I miss it :(. Uploaded a new patch.

Thanks for your suggestions and taking time to review.

[Darin Adler]

Comment on attachment 119240 [details]
Updated patch

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

> Source/WebCore/dom/Element.cpp:2045
> +    return hasRareData() && rareData()->m_scrolledPosition != IntPoint::zero();

This does not answer the question of whether an element is scrollable. It answers the question of whether is is scrolled, which is not the same thing.

> Source/WebCore/dom/Element.cpp:2052
> +IntPoint Element::scrolledPosition() const
> +{
> +    ASSERT(scrollableElement());
> +    return rareData()->m_scrolledPosition;
> +}

This seems wrong. Should just return IntPoint::zero() if there is no rare data.

> Source/WebCore/dom/Element.cpp:2056
> +    ensureRareData()->m_scrolledPosition = point;

This should not create new rare data if called with a zero point on an object that doesn’t already have rare data.

> Source/WebCore/dom/Element.cpp:2062
> +    if (scrollableElement())
> +        rareData()->m_scrolledPosition = IntPoint();

This should just check hasRareData(). No reason to check the position before resetting it to zero.

In fact, I think it should just call setScrollPosition(IntPoint::zero()) rather than having its own logic.

> Source/WebCore/dom/Element.h:367
> +    bool scrollableElement() const;

A function named “scrollable element” would return an element, not an answer to the question “is this scrollable”. I suggest just dropping this function.

> Source/WebCore/dom/Element.h:368
> +    IntPoint scrolledPosition() const;

It doesn’t make sense to call the getter “scrolled position” and then setter “scroll position”. Please choose one term and stick to it.

I think it’s misleading to store something called the “scroll position” that is only accurate when an element has no renderer. Any code that got at this to try to ask what the current scroll position of an element was would get a bad result. If the use of the data is so specific, then I suggest we make the name specific as well. Maybe “saved layer scroll position” is a good name.

> Source/WebCore/dom/Node.cpp:464
> +
> +        if (isElementNode())
> +            toElement(this)->resetScrollPosition();
> +

This is too low level for code like this and there is no need to put Element-specific code into Node here. There is already a virtual function called willMoveToNewOwnerDocument, and if this is needed, this code can go there.

But I don’t understand why moving to a new document is a time to do this. Is that really right? Why would adoptNode be the one thing that would cause us to reset the scroll position when moving a node from one part of the document to another would not?

> Source/WebCore/rendering/RenderLayer.cpp:200
> +        if (element->scrollableElement()) {

This if is unnecessary. It should be save to save off a zero scroll offset and then reset the scroll offset to zero.

[Rakesh]

> > Source/WebCore/dom/Element.cpp:2045
> > +    return hasRareData() && rareData()->m_scrolledPosition != IntPoint::zero();
> 
> This does not answer the question of whether an element is scrollable. It answers the question of whether is is scrolled, which is not the same thing.

Yes, you are right, "scrolled" would be better.

> 
> > Source/WebCore/dom/Element.cpp:2052
> > +IntPoint Element::scrolledPosition() const
> > +{
> > +    ASSERT(scrollableElement());
> > +    return rareData()->m_scrolledPosition;
> > +}
> 
> This seems wrong. Should just return IntPoint::zero() if there is no rare data.

Thinking was that this function should not get called if the scrolled position has not been cached.

> 
> > Source/WebCore/dom/Element.cpp:2056
> > +    ensureRareData()->m_scrolledPosition = point;
> 
> This should not create new rare data if called with a zero point on an object that doesn’t already have rare data.

Yes, we should return if called with zero point.

> 
> > Source/WebCore/dom/Element.cpp:2062
> > +    if (scrollableElement())
> > +        rareData()->m_scrolledPosition = IntPoint();
> 
> This should just check hasRareData(). No reason to check the position before resetting it to zero.

Wanted to reset only if some non zero point has been cached. Otherwise we will be resetting to zero every time which too is fine but can be avoided.

> 
> In fact, I think it should just call setScrollPosition(IntPoint::zero()) rather than having its own logic.

Yes. 

> > Source/WebCore/dom/Element.h:367
> > +    bool scrollableElement() const;
> 
> A function named “scrollable element” would return an element, not an answer to the question “is this scrollable”. I suggest just dropping this function.
> 
> > Source/WebCore/dom/Element.h:368
> > +    IntPoint scrolledPosition() const;
> 
> It doesn’t make sense to call the getter “scrolled position” and then setter “scroll position”. Please choose one term and stick to it.
> 
> I think it’s misleading to store something called the “scroll position” that is only accurate when an element has no renderer. Any code that got at this to try to ask what the current scroll position of an element was would get a bad result. If the use of the data is so specific, then I suggest we make the name specific as well. Maybe “saved layer scroll position” is a good name.

Agreed, it should be in the lines of saveLayerScrollPosition.

> 
> > Source/WebCore/dom/Node.cpp:464
> > +
> > +        if (isElementNode())
> > +            toElement(this)->resetScrollPosition();
> > +
> 
> This is too low level for code like this and there is no need to put Element-specific code into Node here. There is already a virtual function called willMoveToNewOwnerDocument, and if this is needed, this code can go there.
> 
> But I don’t understand why moving to a new document is a time to do this. Is that really right? Why would adoptNode be the one thing that would cause us to reset the scroll position when moving a node from one part of the document to another would not?

Can Element::removedFromDocument be the place for resetting?

> 
> > Source/WebCore/rendering/RenderLayer.cpp:200
> > +        if (element->scrollableElement()) {
> 
> This if is unnecessary. It should be save to save off a zero scroll offset and then reset the scroll offset to zero.

Yes, if the previous suggested changes are taken then this will not be necessary.

[Rakesh]

Created attachment 119833 [details]
Updated patch

Modified function names and handling the case of scrollable layer moved to different location in the same page. Updated the test for the same.

[Darin Adler]

Comment on attachment 119833 [details]
Updated patch

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

> Source/WebCore/dom/Element.cpp:950
> +    resetsavedLayerScrolledPosition();

This should just be:

    saveLayerScrolledPosition(IntPoint::zero());

To me, this is still a bit suspect. I don’t understand why removal from the document is the key moment to reset this state. Does testing with other browsers confirm they follow that rule?

> Source/WebCore/dom/Element.cpp:2043
> +    if (point == IntPoint::zero())
> +        return;

This is incorrect for overwriting stored scroll position with zero. Instead it should read:

    if (point == IntPoint::zero() && !hasRareData())

> Source/WebCore/dom/Element.cpp:2051
> +void Element::resetsavedLayerScrolledPosition()
> +{
> +    if (hasRareData())
> +        rareData()->m_scrolledPosition = IntPoint::zero();
> +}

Please delete this poorly named and unneeded function.

> Source/WebCore/dom/Element.h:369
> +    IntPoint savedLayerScrolledPosition() const;
> +    void saveLayerScrolledPosition(const IntPoint&);

I don’t think “scrolled position” is as good as “scroll position”. I think a more traditional “set” naming here would be slightly better:

    IntPoint savedLayerScrollPosition() const;
    void setSavedLayerScrollPosition(const IntPoint&);

Another option is to do something closer to what RenderLayer actually needs:

    IntSize savedLayerScrollOffset() const;
    void setSavedLayerScrollOffset(const IntSize&);

Either would be OK.

> Source/WebCore/dom/Element.h:370
> +    void resetsavedLayerScrolledPosition();

The s in “saved” should be capitalized, but it would be better to just delete this function.

> Source/WebCore/dom/ElementRareData.h:55
> +    IntPoint m_scrolledPosition;

This should be named:

    IntPoint m_savedLayerScrollPosition;

Or:

    IntSize m_savedLayerScrollOffset;

> Source/WebCore/rendering/RenderLayer.cpp:200
> +        // We save and restore only the scrollOffset as the other scroll
> +        // values are recalulated.

Would read better on a single line.

> Source/WebCore/rendering/RenderLayer.cpp:1396
> -    
> +

Remove this unneeded whitespace change. For one thing, it will cause a merge conflict that otherwise would not occur.

[Rakesh]

(In reply to comment #48)
Thanks for reviewing this.

> This should just be:
> 
>     saveLayerScrolledPosition(IntPoint::zero());
> 
> To me, this is still a bit suspect. I don’t understand why removal from the document is the key moment to reset this state. Does testing with other browsers confirm they follow that rule?

Firefox and IE does reset the scroll position when moved within document or to other document. Opera does only when element is moved to other document.

> This is incorrect for overwriting stored scroll position with zero. Instead it should read:
> 
>     if (point == IntPoint::zero() && !hasRareData())
> 
Agreed.

>     IntSize savedLayerScrollOffset() const;
>     void setSavedLayerScrollOffset(const IntSize&);
> 
> Either would be OK.

> This should be named:
> 
>     IntPoint m_savedLayerScrollPosition;
> 
> Or:
> 
>     IntSize m_savedLayerScrollOffset;
> 

Using scroll offset looks better.

> > Source/WebCore/rendering/RenderLayer.cpp:200
> > +        // We save and restore only the scrollOffset as the other scroll
> > +        // values are recalulated.
> 
> Would read better on a single line.

Ok.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:1396
> > -    
> > +
> 
> Remove this unneeded whitespace change. For one thing, it will cause a merge conflict that otherwise would not occur.

Ok.

[Rakesh]

Created attachment 120026 [details]
Updated patch

Changes as per comments

[Rakesh]

(In reply to comment #50)
> Created an attachment (id=120026) [details]
> Updated patch
> 
> Changes as per comments

Darin, can you please have a look at this patch.

[Julien Chaffraix]

Comment on attachment 120026 [details]
Updated patch

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

Unfortunately Darin didn't comment on the latest patch and it fell off my radar. The patch has unfortunately rotten and would need to be rebaselined on ToT.

> Source/WebCore/ChangeLog:12
> +        Resetting the saved scroll offset if node is moved to other location in same document or other document.

Darin asked about that in his latest review. Does it match the other browsers here?

> Source/WebCore/rendering/RenderLayer.cpp:199
> +        // We save and restore only the scrollOffset as the other scroll values are recalulated.

recalculated

[Rakesh]

Created attachment 132823 [details]
patch_rebaselined_tot

Updated patch on tot with typo fixed.

[Rakesh]

(In reply to comment #52)
> (From update of attachment 120026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120026&action=review
> 
> Unfortunately Darin didn't comment on the latest patch and it fell off my radar. The patch has unfortunately rotten and would need to be rebaselined on ToT.
> 

Thanks for having a look.

> > Source/WebCore/ChangeLog:12
> > +        Resetting the saved scroll offset if node is moved to other location in same document or other document.
> 
> Darin asked about that in his latest review. Does it match the other browsers here?

Yes, other browsers do reset, I mentioned about that in my comment. Was something else expected?

> 
> > Source/WebCore/rendering/RenderLayer.cpp:199
> > +        // We save and restore only the scrollOffset as the other scroll values are recalulated.
> 
> recalculated

Fixed.

[Rakesh]

Julien, can you please review this patch.

[Julien Chaffraix]

Comment on attachment 132823 [details]
patch_rebaselined_tot

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

(In reply to comment #55)
> Julien, can you please review this patch.

Sorry, the patch slipped through my emails. I redid the testing and found quite different supports (using the attached test case):
* IE9 doesn't support resetting the scroll offset.
* Opera doesn't clear the scroll position when moved between documents or inside the same document.
* FF matches our implementation.

It seems like a desirable change but I would like the ChangeLog updated to underline that we are chosing to match Firefox here.

> Source/WebCore/rendering/RenderLayer.cpp:221
> +    Node* node = m_renderer->node();
> +    if (node && node->isElementNode())
> +        toElement(node)->setSavedLayerScrollOffset(m_scrollOffset);

This should be guarded by !m_renderer->documentBeingDestroyed() as we don't want to allocate extra rare data on document destruction.

[Rakesh]

Created attachment 134258 [details]
Updated patch

Modified changelogs to specify behaviour similar to Firefox, Added documentBeingDestroyed check.

[Julien Chaffraix]

Comment on attachment 134258 [details]
Updated patch

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

Very close but the ChangeLog really doesn't cut it.

> Source/WebCore/ChangeLog:7
> +        Maintain the scrolled position of overflowing element when the scrollable RenderLayer is destroyed,
> +        which can be used to restore the scroll position if the same element is again added to the render tree.

Explaining where you store the offset would be neat (in the ElementRareData as we don't expect this to happen a lot).

> Source/WebCore/ChangeLog:8
> +        We are matching Firefox's behaviour.

This would definitely use more context here as this doesn't give much. For example, what's the difference with the other browsers? Think of someone watching the commit at home who doesn't know the gory details.

> Source/WebCore/ChangeLog:16
> +        Resetting the saved scroll offset if node is moved to other location in same document or other document.

We usually prefer ChangeLog to be written in good English. Here it feels weird even to my foreigner's ear due to missing some words. A better writing would be:

Reset the saved scroll offset if the node is moved to another location in the same document or another one.

> Source/WebCore/ChangeLog:21
> +        Helper functions for accessing layer scroll offset from rare data.

Add helper functions to access the layer scroll offset from the element's rare data.

> Source/WebCore/ChangeLog:25
> +        Maintain the scroll offset.

Add the scroll offset book-keeping.

[Rakesh]

Created attachment 134793 [details]
Updated patch

Addressing review comments.

[Rakesh]

Created attachment 134794 [details]
Test moving of element

Test for element moving element with same document and another one. Works with IE too.

[Rakesh]

(In reply to comment #56)
> (From update of attachment 132823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132823&action=review
> 
> (In reply to comment #55)
> > Julien, can you please review this patch.
> 
> Sorry, the patch slipped through my emails. I redid the testing and found quite different supports (using the attached test case):
> * IE9 doesn't support resetting the scroll offset.

It does, I think some timing issues in IE when the layout test is run. I tested with "Test moving of element " uploaded and it resets in both the cases.

> * Opera doesn't clear the scroll position when moved between documents or inside the same document.
Opera reset when moved to another document.


(In reply to comment #58)
> (From update of attachment 134258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134258&action=review

Thanks for r+

> 
> Very close but the ChangeLog really doesn't cut it.
Modified with your inputs.

[Julien Chaffraix]

Comment on attachment 134793 [details]
Updated patch

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

I will fix the ChangeLog this time as it will be faster.

> Source/WebCore/ChangeLog:8
> +        RenderLayer is destroyed, which can be used to restore the scroll position if the same element is 
> +        again added to the render tree.

That's not exact: a RenderObject can lose its RenderLayer without being removed from the tree. The example is toggling overflow: hidden to overflow: visible (as in your test).

> Source/WebCore/ChangeLog:10
> +        Webkit behaviour will be same as Firefox and IE. It differs from Opera in a way that Opera does 

Web*K*it.

> Source/WebCore/ChangeLog:34
> +        Storing the scroll offset if document is not being destroyed.

Normally we want the ChangeLog to be consistent. So here it should be:

Store the scroll offset if *the* document is not being destroyed.

[Julien Chaffraix]

Created attachment 135127 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135127 [details]
Patch for landing

Clearing flags on attachment: 135127

Committed r112919: <http://trac.webkit.org/changeset/112919>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rakesh]

(In reply to comment #62)

Thanks for your inputs and efforts for landing this.

[Simon Fraser (smfr)]

This caused bug 89114.