Bug 82337 – [Qt] Add desktop zooming support for QQuickWebView

• Component: Platform
• Status: RESOLVED FIXED

[Balazs Kelemen]

Currently there is no way for zooming the view in desktop mode (for mobile there is pinch zoom).
It's a question for me whether we should implement this for mobile as well but desktop mode definitely needs an API for zoom.

[Balazs Kelemen]

Created attachment 134062 [details]
Patch

[Balazs Kelemen]

Only implemented for desktop mode. I'm looking forward for comments.

[Balazs Kelemen]

Created attachment 134302 [details]
Patch

Removed the js file as a var property is enought.

[Andras Becsi]

Comment on attachment 134302 [details]
Patch

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

We might need to support this API for the mobile version as well, which we could hook up with the pinch zoom scaling inside QQuickWebPage. Not sure programmatical scaling makes sense in that case though.
The patch looks good, just a few comments.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:514
> +void QQuickWebViewLegacyPrivate::setZoomFactor(qreal factor)
> +{
> +    Q_Q(QQuickWebView);

The q pointer looks like a left-over, and is not used in this function.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:973
> +void QQuickWebViewExperimental::setZoomFactor(qreal zoomFactor)
> +{
> +    Q_Q(QQuickWebView);

Same here.

[Balazs Kelemen]

Created attachment 134535 [details]
Patch

Fixed review comments.

[Andras Becsi]

Comment on attachment 134535 [details]
Patch

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

LGTM now.
Simon, Kenneth, could you review?

> Source/WebKit2/ChangeLog:8
> +        Support zooming with for desktop mode.

Something is not right with the english here. This should probably be something line "zooming in desktop mode".

[Balazs Kelemen]

(In reply to comment #6)
> (From update of attachment 134535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134535&action=review
> 
> LGTM now.
> Simon, Kenneth, could you review?
> 
> > Source/WebKit2/ChangeLog:8
> > +        Support zooming with for desktop mode.
> 
> Something is not right with the english here. This should probably be something line "zooming in desktop mode".

Sure, I will fix it before landing.

[Simon Hausmann]

Comment on attachment 134535 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:256
> +    Q_PROPERTY(qreal zoomFactor READ zoomFactor WRITE setZoomFactor NOTIFY zoomFactorChanged)

By making this part of "experimental" I wonder: How confident are we that this "experiment" is going to succeed, i.e. that we'll move this from experimental to public API?

Perhaps this should just remain private C++ API?

[Balazs Kelemen]

(In reply to comment #8)
> (From update of attachment 134535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134535&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:256
> > +    Q_PROPERTY(qreal zoomFactor READ zoomFactor WRITE setZoomFactor NOTIFY zoomFactorChanged)
> 
> By making this part of "experimental" I wonder: How confident are we that this "experiment" is going to succeed, i.e. that we'll move this from experimental to public API?
> 
> Perhaps this should just remain private C++ API?

Well, than everybody who wants a webview on desktop with zooming support have to use private C++ API? It's quite basic feature, I think.

[Balazs Kelemen]

Comment on attachment 134535 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:974
> +    d_ptr->setZoomFactor(zoomFactor);
> +    emit zoomFactorChanged();
> +}

Forgot to check if the value differs from the current, going to fix.

[Balazs Kelemen]

Created attachment 134622 [details]
Patch

[Balazs Kelemen]

The problem with this API is that it doesn't fit well with mobile. Even if we want to add a way for programatic zooming for mobile it won't be the same thing because the fixed layout only allows scaling which is not the traditional behavior of zoom. I'm going to upload a WIP patch (based on the first) where I implemented it that way, so you can see how it looks like.

[Balazs Kelemen]

Created attachment 134624 [details]
WIP patch - zoom in touch mode

[Kenneth Rohde Christiansen]

Comment on attachment 134622 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Support zooming in desktop mode. The API has no effect
> +        in touch mode.

There is no such thing as touch mode... though we have a preferred mode (non-legacy) which supports touch interaction

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:256
> +    Q_PROPERTY(qreal zoomFactor READ zoomFactor WRITE setZoomFactor NOTIFY zoomFactorChanged)

Does changing this, modify the -webkit-pixel-ratio ? Should it? it probably should.

Keeping that in mind, it probably makes a lot more sense not to use this and instead just increase the pixel ratio. Then the zoom levels should be calculated from the actual device pixel ratio. This would work the exactly same way, and work with the viewport meta tag.

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:300
> +        function nextZoomLevel() {

weird method name, it doesnt return the next zoom level, but actually increases the zoom level value. Why not make it do what exactly it says?

[Balazs Kelemen]

Comment on attachment 134622 [details]
Patch

that's enough for an r- now. going to investigate in the device pixel ratio approach

[Balazs Kelemen]

I have tried to implement a consistent behavior on mobile but I was not really successful with that. I have problems with applying the zoom factor together with the scaling that comes from user interaction and with centering the view (sometimes an offset is applying to the view with an animation which mess up my logic). Definitely I would have to get a better understanding of these things before I could finish, but I'm going to postpone it for now.

[Balazs Kelemen]

Created attachment 137522 [details]
Patch

[Andras Becsi]

Comment on attachment 137522 [details]
Patch

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

Much better. Some comments, before you update to ToT.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:575
> +    // This is used by programatic zooming.
> +    if (m_contentSizeUpdateDisabled)
> +        return;
> +

I think this is not needed, and if it is, this is definetely the wrong place for this, since if the user adds other items to the Flickable in QML (search bar), then he will bind the contentWidth and contentHeight to the summarized size of the page and the additional item in QML. In this case this here has no effect, since JavaScript will resize the content automatically.
I believe, this is not needed, since the resizing here has no effect on the position, if you guard your scaling properly in the interaction engine.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:582
> +    }
> +
> +    ensureContentWithinViewportBoundary(/* immediate */ true);

I think this could cause asserts in QtViewportInteractionEngine::ensureContentWithinViewportBoundary since you put the guard inside its own block.
ensureContentWithinViewportBoundary expects the content being suspended.

[Balazs Kelemen]

Created attachment 137532 [details]
Patch

[Balazs Kelemen]

(In reply to comment #18)
> (From update of attachment 137522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137522&action=review
> 
> Much better. Some comments, before you update to ToT.
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:575
> > +    // This is used by programatic zooming.
> > +    if (m_contentSizeUpdateDisabled)
> > +        return;
> > +
> 
> I think this is not needed, and if it is, this is definetely the wrong place for this, since if the user adds other items to the Flickable in QML (search bar), then he will bind the contentWidth and contentHeight to the summarized size of the page and the additional item in QML. In this case this here has no effect, since JavaScript will resize the content automatically.
> I believe, this is not needed, since the resizing here has no effect on the position, if you guard your scaling properly in the interaction engine.

Right, seems like I don't need it. I guess I created this before I added a call to ensureContentWithinViewportBoundary but the latter solved my issues.
Fixed.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:582
> > +    }
> > +
> > +    ensureContentWithinViewportBoundary(/* immediate */ true);
> 
> I think this could cause asserts in QtViewportInteractionEngine::ensureContentWithinViewportBoundary since you put the guard inside its own block.
> ensureContentWithinViewportBoundary expects the content being suspended.

Yep, it was a silly idea.
Fixed.

[Andras Becsi]

Comment on attachment 137532 [details]
Patch

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

LGTM. Just a last nitpick I realized now:

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:581
> +    QPointF oldContentCenter = m_content->boundingRect().center();
> +    m_content->setContentsScale(itemScaleFromCSS(targetCSSScale));
> +    QPointF newContentCenter = m_content->boundingRect().center();
> +    m_viewport->setContentPos(m_viewport->contentPos() + (newContentCenter - oldContentCenter));

I think instead of this you could call scaleContent(m_viewport->mapToWebContent(m_viewport->boundingRect().center()), targetCSSScale), which has the same positioning logic as you duplicated here.

[Balazs Kelemen]

Created attachment 137542 [details]
Patch

[Balazs Kelemen]

(In reply to comment #21)
> (From update of attachment 137532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137532&action=review
> 
> LGTM. Just a last nitpick I realized now:
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:581
> > +    QPointF oldContentCenter = m_content->boundingRect().center();
> > +    m_content->setContentsScale(itemScaleFromCSS(targetCSSScale));
> > +    QPointF newContentCenter = m_content->boundingRect().center();
> > +    m_viewport->setContentPos(m_viewport->contentPos() + (newContentCenter - oldContentCenter));
> 
> I think instead of this you could call scaleContent(m_viewport->mapToWebContent(m_viewport->boundingRect().center()), targetCSSScale), which has the same positioning logic as you duplicated here.

Absolutely true. Fixed.

[Andras Becsi]

Comment on attachment 137542 [details]
Patch

The patch looks good to me now.
Simon, could you take a look too?

[Simon Hausmann]

(In reply to comment #24)
> (From update of attachment 137542 [details])
> The patch looks good to me now.
> Simon, could you take a look too?

Technically the patch is fine, but I don't see the use-case for the zoom factor on the public WebView API like that.

I do see a use-case for the "traditional" zoom factor that resembles the zooming you can see on Chrome or Safari on the desktop. I think such a property is essential in this context.

Until we have a clear public API for that I think it should be private C++ API that exposes the WebPageProxy's zoom factor to MiniBrowser and anyone who wants to develop a "desktop" style webview component.

The experimental QML component is for properties where we can be reasonably optimistic that the "experiment will succeed" and we can make it public in the future, but we need further testing. I don't see that for a zoom-in-center zoom factor property.

In terms of use-cases for the current WebView for zooming I could imagine things like this:

    (1) Load a particular web page
    (2) Send a CSS selector query to the web process
    (3) Retrieve back a list of geometries of the elements that match the provided selector
    (4) Pick a particular geometry and instruct the webview through public API to center & zoom on a particular element.

Naturally this is an entirely different approach, but I mention it to illustrate that I don't think a zoom-on-center property is very powerful.

I'd be happy to r+ a patch that makes the WebPageProxy properties available via private C++ Qt API and changes MiniBrowser to use it.

[Balazs Kelemen]

Created attachment 138343 [details]
Patch

[Balazs Kelemen]

Comment on attachment 138343 [details]
Patch

Clearing flags on attachment: 138343

Committed r114900: <http://trac.webkit.org/changeset/114900>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.