Skip to content

Conversation

@kongbai1996
Copy link
Contributor

@kongbai1996 kongbai1996 commented Nov 4, 2025

Implement resize method for PinchZoom and update webview_renderer to call it on window size changes
Testing: No new WPT test cases passed.
Fixes: N/A

@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 4, 2025
@yezhizhen yezhizhen changed the title fix: prevent unnecessary panning during pinch zoom Compositor: Pan only when new pinch zoom is different Nov 4, 2025
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 4, 2025
@yezhizhen yezhizhen added this pull request to the merge queue Nov 4, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 4, 2025
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. Scroll can cause a pan of the viewport.

@mrobinson mrobinson removed this pull request from the merge queue due to a manual request Nov 4, 2025
@servo-highfive servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 4, 2025
@mrobinson
Copy link
Member

A scroll on the viewport should be converted into a pan because scrolling causes the pinch zoom viewport to the move to the edge edge of the un-pinch-zoomed viewport. Once the pinch zoom viewport is at the edge, a scroll happens.

@mrobinson
Copy link
Member

mrobinson commented Nov 4, 2025

You can see why this is problem if you run Servo on desktop, pinch zoom, and then scroll with the mouse wheel. Without the pan here, you cannot scroll to the edge of the page.

@yezhizhen
Copy link
Member

You can see why this is problem if you run Servo on desktop, pinch zoom, and then scroll with the mouse wheel. Without the pan here, you cannot scroll to the edge of the page.

Make sense.

…call it on window size changes

- Added a `resize` method to the `PinchZoom` struct to update the unscaled viewport size based on the new webview rectangle.
- Modified the `WebViewRenderer` to invoke the `resize` method of `PinchZoom` whenever the window size changes, ensuring the pinch zoom functionality adapts to the new dimensions.

Signed-off-by: kongbai1996 <1782765876@qq.com>
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 6, 2025
@kongbai1996 kongbai1996 changed the title Compositor: Pan only when new pinch zoom is different Compositor: Implement resize method for PinchZoom Nov 6, 2025
@kongbai1996
Copy link
Contributor Author

You can see why this is problem if you run Servo on desktop, pinch zoom, and then scroll with the mouse wheel. Without the pan here, you cannot scroll to the edge of the page.

There is indeed an issue here, but currently, the unscaled_viewport_size of PinchZoom does not update with the resize. I have added this logic.
On HarmonyOS native web components, the size of the components will be updated through resize.

@mrobinson
Copy link
Member

@kongbai1996 Is seems the PR contents and description have completely changed here?

@kongbai1996
Copy link
Contributor Author

@kongbai1996 Is seems the PR contents and description have completely changed here?

yes

@yezhizhen
Copy link
Member

@kongbai1996 Is seems the PR contents and description have completely changed here?

I also got confused earlier.

@yezhizhen yezhizhen added the T-linux-wpt Do a try run of the WPT label Nov 6, 2025
@github-actions github-actions bot removed the T-linux-wpt Do a try run of the WPT label Nov 6, 2025
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

🔨 Triggering try run (#19134577325) for Linux (WPT)

@github-actions
Copy link

github-actions bot commented Nov 6, 2025

Test results for linux-wpt from try job (#19134577325):

Flaky unexpected result (39)
  • OK /IndexedDB/idbfactory_open.any.html
    • FAIL [expected PASS] subtest: Calling open() with version argument 1.5 should not throw.

      assert_equals: version expected 1 but got 9007199254740991
      

  • OK /_mozilla/mozilla/FileAPI/file-upload.html (#40348)
    • FAIL [expected PASS] subtest: form submission of uploaded file

      assert_equals: expected "OK" but got "\n\n\n\n\n    var fileInput = document.getElementById(\"file-input\");\n    fileInput.addEventListener(\"change\", () =&gt; {\n      parent.postMessage(\"loadedAndFilesChanged\");\n    });\n    fileInput.selectFiles([\"./tests/wpt/mozilla/tests/mozilla/FileAPI/resource/upload.txt\"]);\n\n"
      

  • OK /_mozilla/webxr/create_session.https.html
    • FAIL [expected PASS] subtest: create_session

      can't access property "simulateDeviceConnection", navigator.xr.test is undefined
      

  • CRASH [expected OK] /_webgl/conformance/glsl/misc/shader-with-ivec4-return-value.frag.html
  • CRASH [expected OK] /_webgl/conformance/ogles/GL/radians/radians_001_to_006.html
  • CRASH [expected OK] /_webgl/conformance/ogles/GL/sqrt/sqrt_001_to_006.html
  • CRASH [expected OK] /_webgl/conformance/ogles/GL/step/step_001_to_006.html
  • CRASH [expected OK] /_webgl/conformance/ogles/GL/swizzlers/swizzlers_089_to_096.html
  • CRASH [expected OK] /_webgl/conformance/ogles/GL/swizzlers/swizzlers_097_to_104.html
  • CRASH [expected OK] /_webgl/conformance/rendering/gl-scissor-fbo-test.html
  • CRASH [expected OK] /_webgl/conformance/textures/misc/texture-corner-case-videos.html
  • FAIL [expected PASS] /css/css-backgrounds/background-size-041.html
  • OK /css/css-fonts/generic-family-keywords-001.html (#37467)
    • PASS [expected FAIL] subtest: @font-face matching for quoted and unquoted generic(kai)
  • CRASH [expected OK] /css/css-pseudo/parsing/marker-supported-properties.html
  • OK /custom-elements/form-associated/ElementInternals-setFormValue.html (#29174)
    • PASS [expected FAIL] subtest: Single value - empty name exists
    • PASS [expected FAIL] subtest: Null value should submit nothing
  • OK [expected TIMEOUT] /fetch/api/redirect/redirect-keepalive.https.any.html (#32153)
    • PASS [expected TIMEOUT] subtest: [keepalive][iframe][load] mixed content redirect; setting up
  • OK /fetch/fetch-later/new-window.https.window.html (#32036)
    • FAIL [expected PASS] subtest: A same-origin window[target=''][features=''] can trigger fetchLater.

      assert_equals: Number of sent beacons does not match expected count: expected 1 but got 0
      

    • FAIL [expected PASS] subtest: A same-origin window[target=''][features='popup'] can trigger fetchLater.

      assert_equals: Number of sent beacons does not match expected count: expected 1 but got 0
      

    • FAIL [expected PASS] subtest: A same-origin window[target='_blank'][features=''] can trigger fetchLater.

      assert_equals: Number of sent beacons does not match expected count: expected 1 but got 0
      

    • FAIL [expected PASS] subtest: A same-origin window[target='_blank'][features='popup'] can trigger fetchLater.

      assert_equals: Number of sent beacons does not match expected count: expected 1 but got 0
      

  • OK /fetch/metadata/generated/css-font-face.https.sub.tentative.html (#32732)
    • PASS [expected FAIL] subtest: sec-fetch-user
  • OK /html/browsers/browsing-the-web/navigating-across-documents/008.html (#24456)
    • FAIL [expected PASS] subtest: Link with onclick form submit to javascript url and href navigation

      assert_equals: expected "href" but got "click"
      

  • OK /html/browsers/history/the-history-interface/traverse_the_history_3.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • OK /html/browsers/history/the-history-interface/traverse_the_history_4.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • CRASH [expected OK] /html/browsers/the-window-object/open-close/open-features-tokenization-noreferrer.html
  • CRASH [expected TIMEOUT] /html/canvas/offscreen/layers/2d.layer.global-states.filter.no-cxt-filter.no-transform.tentative.w.html
  • OK /html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/allow-scripts-flag-changing-1.html (#39694)
    • PASS [expected FAIL] subtest: Meta refresh is blocked by the allow-scripts sandbox flag at its creation time, not when refresh comes due
  • OK /html/semantics/forms/form-submission-0/jsurl-form-submit.tentative.html (#36489)
    • PASS [expected FAIL] subtest: Verifies that form submissions scheduled inside javascript: urls take precedence over the javascript: url's return value.
  • OK [expected CRASH] /html/semantics/forms/the-fieldset-element/disabled-003.html (#31730, #39631)
  • OK /navigation-timing/test-navigation-type-reload.html (#33334)
    • PASS [expected FAIL] subtest: Reload domComplete &gt; Original domComplete
    • PASS [expected FAIL] subtest: Reload domContentLoadedEventEnd &gt; Original domContentLoadedEventEnd
    • PASS [expected FAIL] subtest: Reload domContentLoadedEventStart &gt; Original domContentLoadedEventStart
    • PASS [expected FAIL] subtest: Reload fetchStart &gt; Original fetchStart
    • PASS [expected FAIL] subtest: Reload loadEventEnd &gt; Original loadEventEnd
    • PASS [expected FAIL] subtest: Reload loadEventStart &gt; Original loadEventStart
  • PASS [expected FAIL] /png/apng/fcTL-dispose-none.html
  • ERROR [expected OK] /pointerevents/pointerevent_bubble_mousedown_mouseup_different_target.html
  • OK /preload/prefetch-document.html (#37210)
    • FAIL [expected PASS] subtest: different-site document prefetch with 'as=document' should not be consumed

      assert_equals: expected 2 but got 1
      

  • OK /preload/preload-xhr.html (#39092)
    • FAIL [expected PASS] subtest: Make an XHR request immediately after creating link rel=preload.

      assert_equals: resources/dummy.xml?token=22b95e3e-fdb8-4f00-b1f3-7e3358646517 expected 1 but got 0
      

  • CRASH [expected OK] /trusted-types/eval-csp-no-tt.html
  • CRASH [expected OK] /trusted-types/eval-function-constructor.html
  • OK /trusted-types/trusted-types-navigation.html?21-25 (#38997)
    • PASS [expected FAIL] subtest: Navigate a window via form-submission with javascript:-urls in enforcing mode.
  • TIMEOUT [expected OK] /trusted-types/trusted-types-navigation.html?26-30 (#38807)
    • TIMEOUT [expected PASS] subtest: Navigate a frame via form-submission with javascript:-urls w/ default policy in enforcing mode.

      Test timed out
      

  • CRASH [expected TIMEOUT] /uievents/mouse/cancel-mousedown-in-subframe.html
  • CRASH [expected TIMEOUT] /wasm/webapi/empty-body.any.worker.html
  • CRASH [expected OK] /webaudio/the-audio-api/the-audiobuffersourcenode-interface/ctor-audiobuffersource.html
  • OK /webdriver/tests/classic/dismiss_alert/dismiss.py (#39098)
    • FAIL [expected PASS] subtest: test_dismiss_in_popup_window

      AssertionError: no such alert (404): No user prompt is currently active.
      

Stable unexpected results that are known to be intermittent (27)
  • OK /IndexedDB/idbcursor-continuePrimaryKey-exceptions.any.worker.html (#39277)
    • FAIL [expected PASS] subtest: IDBCursor continuePrimaryKey() on object store cursor

      assert_throws_dom: continuePrimaryKey() should throw if source is not an index function "function() {
              cursor.continuePrimaryKey(2, 2);
            }" threw object "TypeError: cursor.continuePrimaryKey is not a function" that is not a DOMException InvalidAccessError: property "code" is equal to undefined, expected 15
      

  • OK /IndexedDB/idbobjectstore_getAll.any.html (#39276)
    • PASS [expected FAIL] subtest: Get all values with transaction.commit()
  • OK /IndexedDB/idbobjectstore_getAll.any.worker.html (#39400)
    • PASS [expected FAIL] subtest: Get all values with transaction.commit()
  • OK /IndexedDB/key-conversion-exceptions.any.html (#39305)
    • FAIL [expected PASS] subtest: IDBCursor continue() method with throwing/invalid keys

      assert_throws_exactly: key conversion with throwing getter should rethrow function "() =&gt; {
            receiver[method](key);
          }" threw object "TypeError: receiver[method] is not a function" but we expected it to throw object "getter: throwing from getter"
      

  • OK /IndexedDB/key-conversion-exceptions.any.worker.html (#39284)
    • FAIL [expected PASS] subtest: IDBCursor continue() method with throwing/invalid keys

      assert_throws_exactly: key conversion with throwing getter should rethrow function "() =&gt; {
            receiver[method](key);
          }" threw object "TypeError: receiver[method] is not a function" but we expected it to throw object "getter: throwing from getter"
      

    • FAIL [expected PASS] subtest: IDBCursor update() method with throwing/invalid keys

      assert_throws_exactly: throwing getter should rethrow during clone function "() =&gt; {
            cursor.update(value);
          }" threw object "TypeError: cursor.update is not a function" but we expected it to throw object "getter: throwing from getter"
      

  • FAIL [expected PASS] /_mozilla/mozilla/sslfail.html (#10760)
  • TIMEOUT [expected OK] /_mozilla/mozilla/window_resize_event.html (#36741)
    • TIMEOUT [expected PASS] subtest: Popup onresize event fires after resizeTo

      Test timed out
      

  • OK /custom-elements/form-associated/form-disabled-callback.html (#38843)
    • PASS [expected FAIL] subtest: A disabled form-associated custom element should not submit an entry for it
  • FAIL [expected TIMEOUT] /dom/xslt/large-cdata.html (#38029)
  • ERROR /fetch/metadata/generated/serviceworker.https.sub.html (#36247)
    • FAIL [expected PASS] subtest: sec-fetch-site - Same origin, no options - registration

      promise_test: Unhandled rejection with value: object "Error: Failed to query for recorded headers."
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/005.html (#27062)
    • FAIL [expected PASS] subtest: Link with onclick navigation and href navigation

      assert_equals: expected "href" but got "click"
      

  • OK /html/browsers/browsing-the-web/navigating-across-documents/refresh/same-document-refresh.html (#34597)
    • FAIL [expected PASS] subtest: Same-Document Referrer from Refresh

      assert_equals: original page loads expected "http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/refresh/resources/refresh-with-section.sub.html?url=%23section" but got "http://web-platform.test:8000/html/browsers/browsing-the-web/navigating-across-documents/refresh/resources/refresh-with-section.sub.html?url=%23section#section"
      

  • TIMEOUT /html/browsers/history/the-history-interface/001.html (#12580)
    • FAIL [expected PASS] subtest: traversing history must also traverse hash changes

      assert_equals: (this could cause other failures later on) expected "" but got "test"
      

  • OK /html/browsers/history/the-history-interface/traverse_the_history_5.html (#21383)
    • PASS [expected FAIL] subtest: Multiple history traversals, last would be aborted
  • TIMEOUT [expected OK] /html/interaction/focus/the-autofocus-attribute/document-with-fragment-nonexistent.html (#28259)
    • TIMEOUT [expected FAIL] subtest: Autofocus elements in top-level browsing context's documents with non-existent fragments should work.

      Test timed out
      

  • TIMEOUT /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
    • FAIL [expected TIMEOUT] subtest: Element with tabindex should support autofocus

      assert_equals: expected "SPAN" but got "BODY"
      

    • PASS [expected NOTRUN] subtest: Non-HTMLElement should not support autofocus
    • TIMEOUT [expected NOTRUN] subtest: Host element with delegatesFocus should support autofocus

      Test timed out
      

  • OK /html/semantics/forms/form-submission-0/multipart-formdata.window.html (#28725)
    • PASS [expected FAIL] subtest: multipart/form-data: Basic test (normal form)
    • PASS [expected FAIL] subtest: multipart/form-data: 0x00 in value (formdata event)
  • OK /html/semantics/forms/form-submission-0/text-plain.window.html (#28687)
    • PASS [expected FAIL] subtest: text/plain: Basic test (normal form)
    • PASS [expected FAIL] subtest: text/plain: \r\n in name (normal form)
  • OK /html/semantics/forms/form-submission-0/urlencoded2.window.html (#28687)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: Basic test (normal form)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: Basic test (formdata event)
    • PASS [expected FAIL] subtest: application/x-www-form-urlencoded: Basic File test (normal form)
  • OK /html/semantics/forms/historical.html (#28568)
    • PASS [expected FAIL] subtest: &lt;input name=isindex&gt; should not be supported
  • TIMEOUT [expected OK] /html/semantics/interactive-elements/the-details-element/name-attribute.html (#40355)
  • OK [expected ERROR] /html/user-activation/no-activation-thru-escape-key.html (#40343)
  • OK /html/webappapis/user-prompts/print-during-unload.html (#35944)
    • FAIL [expected PASS] subtest: print() during unload

      assert_array_equals: expected property 1 to be "destination" but got "error: window.print is not a function" (expected array ["start", "destination"] got ["start", "error: window.print is not a function"])
      

  • OK /preload/preload-error.sub.html (#37177)
    • PASS [expected FAIL] subtest: success (fetch): main
  • OK /trusted-types/trusted-types-navigation.html?01-05 (#38975)
    • FAIL [expected PASS] subtest: Navigate a window via anchor with javascript:-urls in report-only mode.

      promise_test: Unhandled rejection with value: "Unexpected message received: \"No securitypolicyviolation reported!\""
      

  • TIMEOUT [expected OK] /trusted-types/trusted-types-navigation.html?31-35 (#38034)
    • PASS [expected FAIL] subtest: Navigate a frame via form-submission with javascript:-urls in report-only mode.
    • TIMEOUT [expected PASS] subtest: Navigate a frame via form-submission with javascript:-urls w/ default policy in report-only mode.

      Test timed out
      

    • NOTRUN [expected FAIL] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy throwing an exception in enforcing mode.
    • NOTRUN [expected FAIL] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy throwing an exception in report-only mode.
    • NOTRUN [expected FAIL] subtest: Navigate a window via form-submission with javascript:-urls w/ a default policy making the URL invalid in enforcing mode.
  • TIMEOUT [expected OK] /webstorage/localstorage-about-blank-3P-iframe-opens-3P-window.partitioned.html (#29053)
    • TIMEOUT [expected PASS] subtest: StorageKey: test 3P about:blank window opened from a 3P iframe

      Test timed out
      

Stable unexpected results (1)
  • OK /html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm
    • FAIL [expected PASS] subtest: default object size after src is removed

      assert_equals: expected "300px" but got "320px"
      

@github-actions
Copy link

github-actions bot commented Nov 6, 2025

⚠️ Try run (#19134577325) failed.

@yezhizhen
Copy link
Member

This one is quite flaky today. #40457
But I'm not sure if we can add a test for this, as we don't support touch dispatch in webdriver yet.

@kongbai1996
Copy link
Contributor Author

This one is quite flaky today. #40457 But I'm not sure if we can add a test for this, as we don't support touch dispatch in webdriver yet.

I ran it locally 10 times and it passed all of them.

@kongbai1996
Copy link
Contributor Author

Can this be merged? Or do we need to rerun the WPT tests?

@mrobinson
Copy link
Member

Can this be merged? Or do we need to rerun the WPT tests?

Our of curiosity, is this something that actually happens in your use case? When I was thinking about this, before, I was thinking that any changes to size or HiDPI scale of the view should simply reset pinch zoom entirely.

@yezhizhen
Copy link
Member

What does this PR actually achieve? Can you show two videos, showing what's different after this PR?

@kongbai1996
Copy link
Contributor Author

What does this PR actually achieve? Can you show two videos, showing what's different after this PR?

The size of unscaled_viewport_size is fixed during initialization and has no update logic. When using OpenHarmony's web component, its initial value is 1*1, and it is later updated through the resize interface.
During the resize process, the unscaled_viewport_size in PinchZoom is updated synchronously.
If this value is not updated, due to the small size and precision issues with floats, the transform values in PinchZoom will be unequal when scrolling without zooming, causing a new display list to be sent again, which results in a decrease in the scrolling frame rate.

@yezhizhen
Copy link
Member

yezhizhen commented Nov 14, 2025

So it is performance improvement, no behaviour change? @kongbai1996 I believe this also changes some visual behaviour?

Copy link
Member

@yezhizhen yezhizhen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense!

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 14, 2025
Signed-off-by: kongbai1996 <1782765876@qq.com>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 14, 2025
@kongbai1996
Copy link
Contributor Author

What does this PR actually achieve? Can you show two videos, showing what's different after this PR?

The size of unscaled_viewport_size is fixed during initialization and has no update logic. When using OpenHarmony's web component, its initial value is 1*1, and it is later updated through the resize interface. During the resize process, the unscaled_viewport_size in PinchZoom is updated synchronously. If this value is not updated, due to the small size and precision issues with floats, the transform values in PinchZoom will be unequal when scrolling without zooming, causing a new display list to be sent again, which results in a decrease in the scrolling frame rate.

If unscaled_viewport_size is not updated, it can also cause the issue where the page cannot be scrolled to the bottom after zooming.

@mrobinson mrobinson changed the title Compositor: Implement resize method for PinchZoom Compositor: Resize PinchZoom::unscaled_viewport when viewport size changes Nov 18, 2025
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a chance this isn't going to work how you want it to if there is already some PinchZoom applied to the pinch zoom viewport. If possible for your usecase please just clear the entire pinch zoom (reset transform to identity and reset the zoom_factor to 1) when resizing the viewport.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 18, 2025
@yezhizhen
Copy link
Member

I think there's a chance this isn't going to work how you want it to if there is already some PinchZoom applied to the pinch zoom viewport. If possible for your usecase please just clear the entire pinch zoom (reset transform to identity and reset the zoom_factor to 1) when resizing the viewport.

To play safe yeah. @kongbai1996 tested for a while without resetting, seems ok.

@yezhizhen yezhizhen added this pull request to the merge queue Nov 19, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 19, 2025
Merged via the queue into servo:main with commit f76e5f1 Nov 19, 2025
36 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants