Bug 109421 – [WK2][EFL][Qt] Viewport size is affected on page reload.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Mikhail Pozdnyakov]

One of scenarios to reproduce:
1) Open ./ManualTests/fixed-position.html
2) Reload.
3) See that fixed elements have "jumped"

The reason for it is that setFixedVisibleContentRect() is invoked from  WebPage::sendViewportAttributesChanged() during reload with wrong 'contentFixedSize' (scaled by initial scale) => Viewport size is affected => position of relatively positioned elements affected too.

[Mikhail Pozdnyakov]

Created attachment 187870 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 187870 [details]
patch

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

> Source/WebKit2/ChangeLog:17
> +        Frame view's fixed visible content rectangle is updated by PageViewportController
> +        (via CoordinatedLayerTreeHostProxy) when page contents size is changed, 
> +        but CoordinatedLayerTreeHostProxy did not send anything in case if the last sent rectangle
> +        was the same as the one to be sent.
> +
> +        The fixed visible content rectangle is updated also by WebPage at WebPage::sendViewportAttributesChanged()
> +        but considering the initial scale factor obtained from page viewport arguments.
> +
> +        So, we had following problem at page reload: fixed visible content rectangle was updated by WebPage
> +        (considering the initial scale) but not updated by PageViewportController (considering real contents size)

Can you try to balance the line length :-) ?

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:241
> +void CoordinatedLayerTreeHostProxy::reset()
> +{

I hate reset(). It can mean like everything.

setInitialStateForNewPageLoad() or something similar (I would look for similarly named methods) would make its use very clear

> Source/WebKit2/UIProcess/PageViewportController.cpp:155
>  
> +#if USE(COORDINATED_GRAPHICS)
> +    if (DrawingAreaProxy* drawingArea = m_webPageProxy->drawingArea())
> +        if (CoordinatedLayerTreeHostProxy* coordinatedLayerTreeHostProxy = drawingArea->coordinatedLayerTreeHostProxy())
> +            coordinatedLayerTreeHostProxy->reset();
> +#endif

Is this really the right place... This is a call back from somewhere else and you just propagate it.

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 187870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187870&action=review
> 
> > Source/WebKit2/ChangeLog:17
> > +        Frame view's fixed visible content rectangle is updated by PageViewportController
> > +        (via CoordinatedLayerTreeHostProxy) when page contents size is changed, 
> > +        but CoordinatedLayerTreeHostProxy did not send anything in case if the last sent rectangle
> > +        was the same as the one to be sent.
> > +
> > +        The fixed visible content rectangle is updated also by WebPage at WebPage::sendViewportAttributesChanged()
> > +        but considering the initial scale factor obtained from page viewport arguments.
> > +
> > +        So, we had following problem at page reload: fixed visible content rectangle was updated by WebPage
> > +        (considering the initial scale) but not updated by PageViewportController (considering real contents size)
> 
> Can you try to balance the line length :-) ?
I'll try :)
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:241
> > +void CoordinatedLayerTreeHostProxy::reset()
> > +{
> 
> I hate reset(). It can mean like everything.
> 
> setInitialStateForNewPageLoad() or something similar (I would look for similarly named methods) would make its use very clear
think didCommitLoad could be used, like PageViewportController does.
> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:155
> >  
> > +#if USE(COORDINATED_GRAPHICS)
> > +    if (DrawingAreaProxy* drawingArea = m_webPageProxy->drawingArea())
> > +        if (CoordinatedLayerTreeHostProxy* coordinatedLayerTreeHostProxy = drawingArea->coordinatedLayerTreeHostProxy())
> > +            coordinatedLayerTreeHostProxy->reset();
> > +#endif
> 
> Is this really the right place... This is a call back from somewhere else and you just propagate it.
yeah, I guess the better place shall be inside WebPageProxy::didCommitLoadForFrame

[Mikhail Pozdnyakov]

Created attachment 187885 [details]
patch v2

Took comments from Kenneth into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 187885 [details]
patch v2

Looks fine, but let noam/dongsung have a look

[Noam Rosenthal]

Comment on attachment 187885 [details]
patch v2

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

LGTM.

> Source/WebKit2/ChangeLog:23
> +        This patch introduces CoordinatedLayerTreeHostProxy::reset() method

You mean didCommitNewPageLoad?

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (From update of attachment 187885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187885&action=review
> 
> LGTM.
> 
> > Source/WebKit2/ChangeLog:23
> > +        This patch introduces CoordinatedLayerTreeHostProxy::reset() method
> 
> You mean didCommitNewPageLoad?
Indeed :)

[Mikhail Pozdnyakov]

Created attachment 188559 [details]
patch v3

fixed change log.

[Benjamin Poulain]

Comment on attachment 188559 [details]
patch v3

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

1) Why no test?
2) Despite the ChangeLog, I really don't understand why would the drawing area know anything about didCommitLoadForFrame. This looks like punching a giant hole through layers.

> Source/WebKit2/ChangeLog:10
> +        but CoordinatedLayerTreeHostProxy did not send anything in case if the

"in case if the"?

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 188559 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188559&action=review
> 
> 1) Why no test?
there is already ./ManualTests/fixed-position.html, where I found this issue.

> 2) Despite the ChangeLog, I really don't understand why would the drawing area know anything about didCommitLoadForFrame. This looks like punching a giant hole through layers.
>
well maybe didCommitLoadForFrame() is not the best name, but drawing area anyway needs a method to clear its m_lastSentVisibleRect and m_lastSentTrajectoryVector. Initially I called this method "reset()".
 
> > Source/WebKit2/ChangeLog:10
> > +        but CoordinatedLayerTreeHostProxy did not send anything in case if the
> 
> "in case if the"?

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.