Bug 82337

Summary: [Qt] Add desktop zooming support for QQuickWebView
Product: WebKit Reporter: Balazs Kelemen <kbalazs@webkit.org>
Component: PlatformAssignee: Balazs Kelemen <kbalazs@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi@webkit.org, alexis@webkit.org, hausmann@webkit.org, kenneth@webkit.org, webkit.review.bot@gmail.com, zoltan@webkit.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
WIP patch - zoom in touch mode
none
Patch
none
Patch
none
Patch
none
Patch none

Description From 2012-03-27 07:29:59 PST
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.
------- Comment #1 From 2012-03-27 07:48:58 PST -------
Created an attachment (id=134062) [details]
Patch
------- Comment #2 From 2012-03-27 07:51:16 PST -------
Only implemented for desktop mode. I'm looking forward for comments.
------- Comment #3 From 2012-03-28 08:25:44 PST -------
Created an attachment (id=134302) [details]
Patch

Removed the js file as a var property is enought.
------- Comment #4 From 2012-03-28 08:57:18 PST -------
(From update of attachment 134302 [details])
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.
------- Comment #5 From 2012-03-29 02:54:22 PST -------
Created an attachment (id=134535) [details]
Patch

Fixed review comments.
------- Comment #6 From 2012-03-29 05:03:12 PST -------
(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".
------- Comment #7 From 2012-03-29 05:07:11 PST -------
(In reply to comment #6)
> (From update of attachment 134535 [details] [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.
------- Comment #8 From 2012-03-29 07:23:49 PST -------
(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?
------- Comment #9 From 2012-03-29 11:19:20 PST -------
(In reply to comment #8)
> (From update of attachment 134535 [details] [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.
------- Comment #10 From 2012-03-29 11:20:34 PST -------
(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.cpp:974
> +    d_ptr->setZoomFactor(zoomFactor);
> +    emit zoomFactorChanged();
> +}

Forgot to check if the value differs from the current, going to fix.
------- Comment #11 From 2012-03-29 11:22:14 PST -------
Created an attachment (id=134622) [details]
Patch
------- Comment #12 From 2012-03-29 11:24:14 PST -------
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.
------- Comment #13 From 2012-03-29 11:26:02 PST -------
Created an attachment (id=134624) [details]
WIP patch - zoom in touch mode
------- Comment #14 From 2012-04-10 02:05:01 PST -------
(From update of attachment 134622 [details])
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?
------- Comment #15 From 2012-04-10 02:27:27 PST -------
(From update of attachment 134622 [details])
that's enough for an r- now. going to investigate in the device pixel ratio approach
------- Comment #16 From 2012-04-12 06:01:34 PST -------
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.
------- Comment #17 From 2012-04-17 06:01:22 PST -------
Created an attachment (id=137522) [details]
Patch
------- Comment #18 From 2012-04-17 06:42:15 PST -------
(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.

> 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.
------- Comment #19 From 2012-04-17 07:22:02 PST -------
Created an attachment (id=137532) [details]
Patch
------- Comment #20 From 2012-04-17 07:26:45 PST -------
(In reply to comment #18)
> (From update of attachment 137522 [details] [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.
------- Comment #21 From 2012-04-17 08:09:10 PST -------
(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.
------- Comment #22 From 2012-04-17 08:34:20 PST -------
Created an attachment (id=137542) [details]
Patch
------- Comment #23 From 2012-04-17 08:34:42 PST -------
(In reply to comment #21)
> (From update of attachment 137532 [details] [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.
------- Comment #24 From 2012-04-18 04:34:16 PST -------
(From update of attachment 137542 [details])
The patch looks good to me now.
Simon, could you take a look too?
------- Comment #25 From 2012-04-18 12:05:30 PST -------
(In reply to comment #24)
> (From update of attachment 137542 [details] [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.
------- Comment #26 From 2012-04-23 07:43:51 PST -------
Created an attachment (id=138343) [details]
Patch
------- Comment #27 From 2012-04-23 07:56:50 PST -------
(From update of attachment 138343 [details])
Clearing flags on attachment: 138343

Committed r114900: <http://trac.webkit.org/changeset/114900>
------- Comment #28 From 2012-04-23 07:56:59 PST -------
All reviewed patches have been landed.  Closing bug.