Bug 82337 - [Qt] Add desktop zooming support for QQuickWebView
Summary: [Qt] Add desktop zooming support for QQuickWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 07:29 PDT by Balazs Kelemen
Modified: 2012-04-23 07:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.03 KB, patch)
2012-03-27 07:48 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2012-03-28 08:25 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2012-03-29 02:54 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2012-03-29 11:22 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
WIP patch - zoom in touch mode (3.02 KB, patch)
2012-03-29 11:26 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2012-04-17 06:01 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (10.07 KB, patch)
2012-04-17 07:22 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (9.88 KB, patch)
2012-04-17 08:34 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2012-04-23 07:43 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-03-27 07:29:59 PDT
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 Balazs Kelemen 2012-03-27 07:48:58 PDT
Created attachment 134062 [details]
Patch
Comment 2 Balazs Kelemen 2012-03-27 07:51:16 PDT
Only implemented for desktop mode. I'm looking forward for comments.
Comment 3 Balazs Kelemen 2012-03-28 08:25:44 PDT
Created attachment 134302 [details]
Patch

Removed the js file as a var property is enought.
Comment 4 Andras Becsi 2012-03-28 08:57:18 PDT
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.
Comment 5 Balazs Kelemen 2012-03-29 02:54:22 PDT
Created attachment 134535 [details]
Patch

Fixed review comments.
Comment 6 Andras Becsi 2012-03-29 05:03:12 PDT
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".
Comment 7 Balazs Kelemen 2012-03-29 05:07:11 PDT
(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.
Comment 8 Simon Hausmann 2012-03-29 07:23:49 PDT
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?
Comment 9 Balazs Kelemen 2012-03-29 11:19:20 PDT
(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.
Comment 10 Balazs Kelemen 2012-03-29 11:20:34 PDT
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.
Comment 11 Balazs Kelemen 2012-03-29 11:22:14 PDT
Created attachment 134622 [details]
Patch
Comment 12 Balazs Kelemen 2012-03-29 11:24:14 PDT
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 Balazs Kelemen 2012-03-29 11:26:02 PDT
Created attachment 134624 [details]
WIP patch - zoom in touch mode
Comment 14 Kenneth Rohde Christiansen 2012-04-10 02:05:01 PDT
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?
Comment 15 Balazs Kelemen 2012-04-10 02:27:27 PDT
Comment on attachment 134622 [details]
Patch

that's enough for an r- now. going to investigate in the device pixel ratio approach
Comment 16 Balazs Kelemen 2012-04-12 06:01:34 PDT
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 Balazs Kelemen 2012-04-17 06:01:22 PDT
Created attachment 137522 [details]
Patch
Comment 18 Andras Becsi 2012-04-17 06:42:15 PDT
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.
Comment 19 Balazs Kelemen 2012-04-17 07:22:02 PDT
Created attachment 137532 [details]
Patch
Comment 20 Balazs Kelemen 2012-04-17 07:26:45 PDT
(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.
Comment 21 Andras Becsi 2012-04-17 08:09:10 PDT
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.
Comment 22 Balazs Kelemen 2012-04-17 08:34:20 PDT
Created attachment 137542 [details]
Patch
Comment 23 Balazs Kelemen 2012-04-17 08:34:42 PDT
(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.
Comment 24 Andras Becsi 2012-04-18 04:34:16 PDT
Comment on attachment 137542 [details]
Patch

The patch looks good to me now.
Simon, could you take a look too?
Comment 25 Simon Hausmann 2012-04-18 12:05:30 PDT
(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.
Comment 26 Balazs Kelemen 2012-04-23 07:43:51 PDT
Created attachment 138343 [details]
Patch
Comment 27 Balazs Kelemen 2012-04-23 07:56:50 PDT
Comment on attachment 138343 [details]
Patch

Clearing flags on attachment: 138343

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