Bug 217955 – [GTK] Zooming causes page to scroll to top

• Component: WebKitGTK
• Status: RESOLVED FIXED

Zooming on a a page (Ctrl+- or Ctrl++ or Ctrl+0) immediately "scrolls" (doesn't actually trigger a scroll animation) to the top of the page, losing the user's place.
This can be tested on probably any page, including planet.gnome.org

1) Scroll down a page
2) Ctrl++ to zoom in
Result: You are now back at the top of the page

epiphany-browser 3.38.1
webkitgtk 2.30.1

[Adrian Perez]

This is reproducible also in the current trunk—just checked with a
recent build from yesterday.

[Chris Lord]

Created attachment 412635 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Chris Lord]

This is pretty weird, it seems the behaviour was intentional...? Either way, here's the patch that reverses it, this certainly isn't desirable behaviour in any situation as far as I can tell, but maybe we'll find out in review :)

[Carlos Garcia Campos]

Comment on attachment 412635 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:-3604
> -    page.scalePage(1.0, IntPoint()); // Reset page scale when zoom level is changed

This was added for pinch to zoom in r235529, are you sure this is no longer needed?

[Chris Lord]

(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 412635 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412635&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:-3604
> > -    page.scalePage(1.0, IntPoint()); // Reset page scale when zoom level is changed
> 
> This was added for pinch to zoom in r235529, are you sure this is no longer
> needed?

Not certain at all, was it actually needed in the first place? Is there a platform or method that would make this easy to test?

I've cc'd Justin in case they're available to comment on this.

[Chris Lord]

(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 412635 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412635&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:-3604
> > -    page.scalePage(1.0, IntPoint()); // Reset page scale when zoom level is changed
> 
> This was added for pinch to zoom in r235529, are you sure this is no longer
> needed?

Reading that patch and reading what scalePage does, I don't see why this is necessary. It seems to be a user experience thing, but currently, it just has a negative effect. I don't see any situation where you'd really want the scroll position to suddenly reset and text zoom and page scale are two different things, I don't see why changing text zoom would necessarily imply that page scale should be reset.

My suggestion is to just check in this change, and if there's some situation we're not seeing here, it ought to become evident soon.

[Chris Lord]

Created attachment 413020 [details]
Patch

[EWS]

Committed r269290: <https://trac.webkit.org/changeset/269290>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413020 [details].