Bug 99715 – [Qt][WK2] Fit-to-width broken on pages with viewport meta tag

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

[Qt][WK2] Fit-to-width broken on pages with viewport meta tag

[Andras Becsi]

Created attachment 169407 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 169407 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The initial scale from the viewport attributes should only

It's better to keep ChangeLog details only with the relevant parts, else the full details will appear twice in the commit message.

> Source/WebKit2/ChangeLog:12
> +        The initial scale from the viewport attributes should only
> +        be applied if it was explicitly specified in the viewport
> +        meta tag. If the initial scale is auto the content should
> +        initially be scaled to the minimum scale that fits the page
> +        horizontally into the view.

Might be worth saying somehow that what we want is an initialScale calculated using the final contentsSize, which might be larger than the layoutSize.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:101
> +            verify(webView.waitForLoadSucceeded())

For viewport tests it might be more reliable to use waitForViewportReady, which waits for the first DidRenderFrame and applied the initialScale. (and set "when: windowShown" on the TestCase)

> Source/WebKit2/UIProcess/PageViewportController.cpp:162
> +        applyScaleAfterRenderingContents(innerBoundedViewportScale(toViewportScale(initialScale)));

I'm not sure but I think that didChangeContentsSize will already have called applyScaleAfterRenderingContents in updateMinimumScaleToFit, and here we would normally just overwrite it here. So if I'm right in this case you could just check for m_rawAttributes.initiallyFitToWidth in the if() and avoid it.

[Andras Becsi]

Created attachment 169421 [details]
Patch

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 169407 [details])
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:101
> > +            verify(webView.waitForLoadSucceeded())
> 
> For viewport tests it might be more reliable to use waitForViewportReady, which waits for the first DidRenderFrame and applied the initialScale. (and set "when: windowShown" on the TestCase)

Both tests fail if using waitForViewportReady since it fires too early for the scale to propagate, so I left loadSucceeded.

> 
> > Source/WebKit2/UIProcess/PageViewportController.cpp:162
> > +        applyScaleAfterRenderingContents(innerBoundedViewportScale(toViewportScale(initialScale)));
> 
> I'm not sure but I think that didChangeContentsSize will already have called applyScaleAfterRenderingContents in updateMinimumScaleToFit, and here we would normally just overwrite it here. So if I'm right in this case you could just check for m_rawAttributes.initiallyFitToWidth in the if() and avoid it.

The minimum scale is only applied in updateMinimumScaleToFit() if there was no user interaction and the content is not suspended, so for example the scale is not applied when transition completes when clicking a link after pinch zoom.

[Jocelyn Turcotte]

Comment on attachment 169407 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:101
>>> +            verify(webView.waitForLoadSucceeded())
>> 
>> For viewport tests it might be more reliable to use waitForViewportReady, which waits for the first DidRenderFrame and applied the initialScale. (and set "when: windowShown" on the TestCase)
> 
> Both tests fail if using waitForViewportReady since it fires too early for the scale to propagate, so I left loadSucceeded.

Ahh, this could be related to the change that didChangeContentsSize is now only called if the size changed in didRenderFrame, which took care of emitting contentsScaleCommitted.
The thing is that there is no guarantee that loadSucceeded gets emitted after, especially on local pages. There is no order enforced between those two signals.
Anyway we can see if that breaks on the bots first, and fix it then.

>>> Source/WebKit2/UIProcess/PageViewportController.cpp:162
>>> +        applyScaleAfterRenderingContents(innerBoundedViewportScale(toViewportScale(initialScale)));
>> 
>> I'm not sure but I think that didChangeContentsSize will already have called applyScaleAfterRenderingContents in updateMinimumScaleToFit, and here we would normally just overwrite it here. So if I'm right in this case you could just check for m_rawAttributes.initiallyFitToWidth in the if() and avoid it.
> 
> The minimum scale is only applied in updateMinimumScaleToFit() if there was no user interaction and the content is not suspended, so for example the scale is not applied when transition completes when clicking a link after pinch zoom.

Ok, makes sense.

[Jocelyn Turcotte]

LGTM but it would be nice to know what Kenneth thinks about the boolean in ViewportAttributes.

[Andras Becsi]

(In reply to comment #5)
> (From update of attachment 169407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169407&action=review
> 
> >>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml:101
> >>> +            verify(webView.waitForLoadSucceeded())
> >> 
> >> For viewport tests it might be more reliable to use waitForViewportReady, which waits for the first DidRenderFrame and applied the initialScale. (and set "when: windowShown" on the TestCase)
> > 
> > Both tests fail if using waitForViewportReady since it fires too early for the scale to propagate, so I left loadSucceeded.
> 
> Ahh, this could be related to the change that didChangeContentsSize is now only called if the size changed in didRenderFrame, which took care of emitting contentsScaleCommitted.
> The thing is that there is no guarantee that loadSucceeded gets emitted after, especially on local pages. There is no order enforced between those two signals.
> Anyway we can see if that breaks on the bots first, and fix it then.

That's also what I suspected, but removing the check for the changing size does not fix the tests with waitForViewportReady.
The problem seems to be that we get a couple of DidRenderFrame messages before pageTransitionViewportReady() or one of the updateMinimumScaleToFit() calls would actually apply the initial scale.
So loadVisuallyCommitted() is emitted before the correct scale is actually applied.

Maybe loadVisuallyCommitted should actually depend on pageTransitionViewportReady?

[Jocelyn Turcotte]

(In reply to comment #7)
> Maybe loadVisuallyCommitted should actually depend on pageTransitionViewportReady?

It does indirectly, there should be no rendering between provisionalLoadStarted and the commitPageTransitionViewport response in pageTransitionViewportReady. This call is actually responsible of resuming the rendering once we requested the scale that we want to show to the user for the first glimpse of the page. Then there should be a DidRenderFrame following and loadVisuallyCommitted would be emitted.
If loadVisuallyCommitted gets emitted before pageTransitionViewportReady there is definitely something wrong.

[Andras Becsi]

(In reply to comment #8)
> (In reply to comment #7)
> > Maybe loadVisuallyCommitted should actually depend on pageTransitionViewportReady?
> 
> It does indirectly, there should be no rendering between provisionalLoadStarted and the commitPageTransitionViewport response in pageTransitionViewportReady. This call is actually responsible of resuming the rendering once we requested the scale that we want to show to the user for the first glimpse of the page. Then there should be a DidRenderFrame following and loadVisuallyCommitted would be emitted.
> If loadVisuallyCommitted gets emitted before pageTransitionViewportReady there is definitely something wrong.

Hmm, ok, in this case I have to verify my previous findings next week, since I'm pretty sure there were multiple DidRenderFrame messages before pageTransitionViewportReady actually applied the initial scale.

[Kenneth Rohde Christiansen]

Comment on attachment 169421 [details]
Patch

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

> Source/WebKit2/ChangeLog:14
> +        The initial scale from the viewport attributes should only
> +        be applied if the scale was explicitly specified in the
> +        viewport meta tag.
> +        If the initial scale is auto it should be calculated using
> +        the final contents size, which might be larger than the
> +        layout size, so that the content fits horizontally into
> +        the view.

that sounds right

> Source/WebCore/dom/ViewportArguments.cpp:56
>      result.devicePixelRatio = devicePixelRatio;
> +    result.initiallyFitToWidth = args.initialScale == ViewportArguments::ValueAuto;

Why can't you just do this check outside ViewportArguments? Are we modifying initialScale in the algorithm? I guess we shouldn't do that.

Maybe we should remove
 
118 result.initialScale = availableWidth / desktopWidth;

I think that is fine with the spec.

[Andras Becsi]

(In reply to comment #10)
> (From update of attachment 169421 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169421&action=review
> 
> > Source/WebKit2/ChangeLog:14
> > +        The initial scale from the viewport attributes should only
> > +        be applied if the scale was explicitly specified in the
> > +        viewport meta tag.
> > +        If the initial scale is auto it should be calculated using
> > +        the final contents size, which might be larger than the
> > +        layout size, so that the content fits horizontally into
> > +        the view.
> 
> that sounds right
> 
> > Source/WebCore/dom/ViewportArguments.cpp:56
> >      result.devicePixelRatio = devicePixelRatio;
> > +    result.initiallyFitToWidth = args.initialScale == ViewportArguments::ValueAuto;
> 
> Why can't you just do this check outside ViewportArguments? Are we modifying initialScale in the algorithm? I guess we shouldn't do that.
> 
The initial scale is resolved in computeViewportAttributes because the layout size depends on it being resolved.

> Maybe we should remove
> 
> 118 result.initialScale = availableWidth / desktopWidth;
> 
That would not help because in the width=device-width case args.width is already resolved to be deviceWidth. Additionally we would end up with a wrong initial scale after the initialScale is adjusted to be within valid boundaries because the resolved minimumScale is not yet the minimum scale to fit.

The problem here is that the layout size depends on the initial scale in the current implementation and the final contents size might be expanded after computeViewportAttributes.
Also clients assume that result.initialScale is within valid bounds, so leaving it as -1 would cause silent errors, which leaves us with the bool flag.

[Kenneth Rohde Christiansen]

Adding the Chrome for Android guy responsible(?) for this area

[Andras Becsi]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Maybe loadVisuallyCommitted should actually depend on pageTransitionViewportReady?
> > 
> > It does indirectly, there should be no rendering between provisionalLoadStarted and the commitPageTransitionViewport response in pageTransitionViewportReady. This call is actually responsible of resuming the rendering once we requested the scale that we want to show to the user for the first glimpse of the page. Then there should be a DidRenderFrame following and loadVisuallyCommitted would be emitted.
> > If loadVisuallyCommitted gets emitted before pageTransitionViewportReady there is definitely something wrong.
> 
> Hmm, ok, in this case I have to verify my previous findings next week, since I'm pretty sure there were multiple DidRenderFrame messages before pageTransitionViewportReady actually applied the initial scale.

The loadVisuallyCommitted signal is emitted before the initial scale is applied because during transition the content size falls back to the size set by the basic test using js. This could be related to page caching behavior.
If the basic test is removed using waitForViewportReady works for the test.

[Andras Becsi]

Created attachment 171456 [details]
Patch

[Andras Becsi]

Created attachment 172602 [details]
Patch

[Andras Becsi]

Comment on attachment 172602 [details]
Patch

Clearing flags on attachment: 172602

Committed r133624: <http://trac.webkit.org/changeset/133624>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.