Bug 217955 - [GTK] Zooming causes page to scroll to top
Summary: [GTK] Zooming causes page to scroll to top
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2020-10-20 07:52 PDT by erusan
Modified: 2020-11-03 02:18 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.43 KB, patch)
2020-10-29 05:54 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2020-11-03 01:42 PST, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description erusan 2020-10-20 07:52:43 PDT
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
Comment 1 Adrian Perez 2020-10-28 15:35:49 PDT
This is reproducible also in the current trunk—just checked with a
recent build from yesterday.
Comment 2 Chris Lord 2020-10-29 05:54:01 PDT
Created attachment 412635 [details]
Patch
Comment 3 EWS Watchlist 2020-10-29 05:55:05 PDT
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
Comment 4 Chris Lord 2020-10-29 05:57:05 PDT
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 :)
Comment 5 Carlos Garcia Campos 2020-10-29 06:34:28 PDT
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?
Comment 6 Chris Lord 2020-10-29 06:45:34 PDT
(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.
Comment 7 Chris Lord 2020-11-02 09:12:05 PST
(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.
Comment 8 Chris Lord 2020-11-03 01:42:10 PST
Created attachment 413020 [details]
Patch
Comment 9 EWS 2020-11-03 02:18:34 PST
Committed r269290: <https://trac.webkit.org/changeset/269290>

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