8000 add implementation for Path2D addPath method by arthmis · Pull Request #37838 · servo/servo · GitHub
[go: up one dir, main page]
Skip to content

add implementation for Path2D addPath method #37838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 21 commits into from
Aug 9, 2025
Merged

add implementation for Path2D addPath method #37838

merged 21 commits into from
Aug 9, 2025

Conversation

arthmis
Copy link
Contributor
@arthmis arthmis commented Jul 2, 2025 •
edited
Loading

Add implementation for Path2D addPath method
Spec: https://html.spec.whatwg.org/multipage/canvas.html#dom-path2d-addpath

Testing: WPT test - tests/wpt/tests/css/geometry/DOMMatrix2DInit-validate-fixup.html
Fixes: #37695

arthmis
arthmis commented Jul 2, 2025
View reviewed changes
arthmis
arthmis commented Jul 2, 2025
View reviewed changes
8000
@arthmis
Copy link
Contributor Author
arthmis commented Jul 3, 2025

@sagudev I'm trying to run the WPT for the path2d functionality found here tests\wpt\tests\html\canvas\element\path-objects in the repo. They all crash though, is that expected at this stage? I use this command ./mach test-wpt tests\wpt\tests\html\canvas\element\path-objects -r.

I also get a message of Unrecognized option: 'enable-experimental-web-platform-features'. I'm on Windows 10 if that matters. I also noticed that most of the tests don't have corresponding ini files in this meta folder tests\wpt\meta\html\canvas\element\path-objects. Do I need to add the ini files?

@sagudev
Copy link
Member
sagudev commented Jul 3, 2025

Not sure about status of WPT runs on windows, but WPT runs in CI should work (uses ubuntu runners): https://book.servo.org/hacking/testing.html?highlight=mach%20try#running-web-platform-tests-on-your-github-fork

@arthmis arthmis force-pushed the lmassiah/issue-37695 branch 3 times, most recently from 9eaa17a to 536a463 Compare July 3, 2025 19:32
@arthmis
Copy link
Contributor Author
arthmis commented Jul 3, 2025 •
edited
Loading

Not sure about status of WPT runs on windows, but WPT runs in CI should work (uses ubuntu runners): https://book.servo.org/hacking/testing.html?highlight=mach%20try#running-web-platform-tests-on-your-github-fork

Seems running the WPT tests on windows locally doesn't work out of the box. I had to switch to my macbook for the tests to run.

@arthmis arthmis marked this pull request as ready for review July 3, 2025 21:24
@arthmis arthmis requested a review from gterzian as a code owner July 3, 2025 21:24
@arthmis arthmis requested a review from sagudev July 3, 2025 21:24
sagudev
sagudev reviewed Jul 4, 2025
View reviewed changes
@sagudev sagudev added the T-linux-wpt Do a try run of the WPT label Jul 4, 2025
@github-actions github-actions bot removed the T-linux-wpt Do a try run of the WPT label Jul 4, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Jul 4, 2025

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

@github-actions GitHub Actions
Copy link
github-actions bot commented Jul 4, 2025

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

Flaky unexpected result (18)
  • OK /FileAPI/url/url-with-fetch.any.html (#21517)
    • FAIL [expected PASS] subtest: Revoke blob URL after calling fetch, fetch should succeed

      promise_test: Unhandled rejection with value: object "TypeError: Network error occurred"

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

      Test timed out

  • OK /css/css-fonts/variations/at-font-face-font-matching.html (#20684)
    • FAIL [expected PASS] subtest: Matching font-weight: '400' should prefer '351 398' over '501 550'

      assert_equals: Unexpected font on test element expected 487 but got 532

    • FAIL [expected PASS] subtest: Matching font-weight: '501' should prefer '390 410' over '300 350'

      assert_equals: Unexpected font on test element expected 487 but got 532

    • FAIL [expected PASS] subtest: Matching font-style: 'italic' should prefer 'oblique 30deg 60deg' over 'oblique 40deg 50deg'

      assert_equals: Unexpected font on test element expected 487 but got 532

  • OK /css/css-grid/alignment/grid-content-alignment-with-abspos-001.html (#34339)
    • FAIL [expected PASS] subtest: .grid 1

      assert_equals: 
<div class="grid" data-expected-width="800" data-expected-height="600">
    <div class="a" id="item" data-offset-x="329" data-offset-y="269" data-expected-width="142" data-expected-height="62" style="place-self: center;"></div>
  </div>
offsetLeft expected 329 but got 0

  • OK [expected ERROR] /fetch/fetch-later/quota/same-origin-iframe/empty-payload.tentative.https.window.html (#35176)
  • TIMEOUT [expected ERROR] /fetch/fetch-later/quota/same-origin-iframe/max-payload.tentative.https.window.html (#35210)
  • OK /html/browsers/browsing-the-web/overlapping-navigations-and-traversals/cross-document-nav-cross-document-nav.html (#29181)
    • PASS [expected FAIL] subtest: cross-document navigation then cross-document navigation
  • 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/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-nav-link-click.html (#32664)
    • FAIL [expected PASS] subtest: Navigating iframe loading='lazy' before it is loaded: link click

      uncaught exception: Error: assert_equals: expected "http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/support/blank.htm?nav" but got "http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/support/blank.htm?src"

  • OK /html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-nav-location-assign.html (#32863)
    • FAIL [expected PASS] subtest: Navigating iframe loading='lazy' before it is loaded: location.assign

      uncaught exception: Error: assert_equals: expected "http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/support/blank.htm?nav" but got "http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/support/blank.htm?src"

  • OK /html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url.any.html (#33948)
    • FAIL [expected PASS] subtest: Revoking a blob URL immediately after calling import will not fail

      promise_test: Unhandled rejection with value: object "TypeError: Dynamic import failed"

  • 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/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

  • ERROR /service-workers/idlharness.https.any.html (#36250)
    • PASS [expected TIMEOUT] subtest: ServiceWorkerContainer interface: operation register((TrustedScriptURL or USVString), optional RegistrationOptions)
    • PASS [expected TIMEOUT] subtest: NavigationPreloadManager interface: operation enable()
    • PASS [expected TIMEOUT] subtest: NavigationPreloadManager interface: operation disable()
    • PASS [expected TIMEOUT] subtest: NavigationPreloadManager interface: operation setHeaderValue(ByteString)
    • PASS [expected TIMEOUT] subtest: NavigationPreloadManager interface: operation getState()
  • OK [expected TIMEOUT] /webmessaging/with-ports/017.html (#24486)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, about:blank
  • OK [expected TIMEOUT] /webmessaging/without-ports/017.html (#24486)
    • PASS [expected TIMEOUT] subtest: origin of the script that invoked the method, about:blank
  • OK /workers/dedicated-worker-from-blob-url.window.html (#22286)
    • PASS [expected FAIL] subtest: Creating a dedicated worker from a blob URL works immediately before revoking.
  • OK /xhr/open-url-multi-window-5.htm (#23360)
    • FAIL [expected PASS] subtest: XMLHttpRequest: open() resolving URLs (multi-Window; 5)

      assert_throws_dom: function "function() {client.open("GET", "...") }" did not throw

Stable unexpected results that are known to be intermittent (21)
  • FAIL [expected PASS] /_mozilla/css/stacked_layers.html (#15988)
  • FAIL [expected PASS] /_mozilla/mozilla/sslfail.html (#10760)
  • FAIL [expected PASS] /css/css-grid/grid-items/grid-auto-margin-and-replaced-item-001.html (#37162)
  • OK /fetch/metadata/generated/css-font-face.https.sub.tentative.html (#32732)
    • PASS [expected FAIL] subtest: sec-fetch-user
  • OK /fetch/metadata/generated/css-font-face.sub.tentative.html (#34624)
    • PASS [expected FAIL] subtest: sec-fetch-storage-access - Not sent to non-trustworthy same-origin destination
    • PASS [expected FAIL] subtest: sec-fetch-storage-access - Not sent to non-trustworthy same-site destination
    • FAIL [expected PASS] subtest: sec-fetch-storage-access - Not sent to non-trustworthy cross-site destination

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

  • TIMEOUT [expected OK] /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/iframe-src-aboutblank-navigate-immediately.html (#29048)
    • TIMEOUT [expected FAIL] subtest: Navigating to a different document with link click

      Test timed out

    • NOTRUN [expected FAIL] subtest: Navigating to a different document with form submission
  • OK /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html (#28681)
    • PASS [expected FAIL] subtest: load & pageshow events do not fire on contentWindow of <iframe> element created with src=''
  • OK /html/browsers/browsing-the-web/navigating-across-documents/navigation-unload-same-origin-fragment.html (#20768)
    • PASS [expected FAIL] subtest: Tests that a fragment navigation in the unload handler will not block the initial navigation
  • PASS [expected FAIL] /html/canvas/element/manual/text/canvas.2d.disconnected.html (#30063)
  • 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

  • OK [expected TIMEOUT] /html/interaction/focus/the-autofocus-attribute/supported-elements.html (#24145)
    • PASS [expected NOTRUN] subtest: Non-HTMLElement should not support autofocus
    • FAIL [expected NOTRUN] subtest: Host element with delegatesFocus should support autofocus

      assert_equals: expected Element node <div autofocus=""></div> but got Element node <body></body>

    • FAIL [expected NOTRUN] subtest: Host element with delegatesFocus including no focusable descendants should be skipped

      assert_equals: expected Element node <input autofocus=""></input> but got Element node <body><div autofocus=""></div><input autofocus=""></body>

    • FAIL [expected NOTRUN] subtest: Area element should support autofocus

      promise_test: Unhandled rejection with value: object "TypeError: w.document.querySelector(...) is null"

  • TIMEOUT [expected CRASH] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-1.html (#22647)
  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html (#22667)
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3.html (#24057)
    • FAIL [expected TIMEOUT] subtest: Check that popups from a sandboxed iframe escape the sandbox if allow-popups-to-escape-sandbox is used

      assert_equals: It came from a sandboxed iframe expected "null" but got "http://web-platform.test:8000"

  • CRASH [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-1.html (#24066)
  • TIMEOUT /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html (#22154)
    • NOTRUN [expected FAIL] subtest: Check that popups from a sandboxed iframe do not escape the sandbox
  • OK [expected TIMEOUT] /html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3.html (#24066)
  • OK /html/semantics/scripting-1/the-script-element/execution-timing/077.html (#22139)
    • FAIL [expected PASS] subtest: adding several types of scripts through the DOM and removing some of them confuses scheduler

      assert_array_equals: expected property 1 to be "Script #1 ran" but got "Script #3 ran" (expected array ["Script #2 ran", "Script #1 ran", "Script #3 ran", "Script #4 ran"] got ["Script #2 ran", "Script #3 ran", "Script #4 ran", "Script #1 ran"])

  • OK /preload/preload-error.sub.html (#37177)
    • FAIL [expected PASS] subtest: success (style): main

      assert_greater_than: http://web-platform.test:8000/preload/resources/dummy.css?label=style should be loaded expected a number greater than 0 but got 0

    • PASS [expected FAIL] subtest: CORS (style): main
    • PASS [expected FAIL] subtest: success (script): main
    • FAIL [expected PASS] subtest: Decode-error (style): main

      assert_greater_than: http://web-platform.test:8000/preload/resources/dummy.xml?pipe=header%28Content-Type%2Ctext%2Fcss%29&label=style should be loaded expected a number greater than 0 but got 0

  • OK [expected TIMEOUT] /resource-timing/tentative/document-initiated.html (#37785)
  • OK [expected ERROR] /workers/constructors/Worker/Worker-constructor.html (#22991)

@github-actions GitHub Actions
Copy link
github-actions bot commented Jul 4, 2025

✨ Try run (#16066642903) succeeded.

sagudev
sagudev requested changes Jul 4, 2025
View reviewed changes
@arthmis arthmis force-pushed the lmassiah/issue-37695 branch 3 times, most recently from 5a999cd to a5836f8 Compare July 4, 2025 14:50
@arthmis arthmis marked this pull request as draft July 6, 2025 04:06
@arthmis arthmis force-pushed the 8000 lmassiah/issue-37695 branch from a5836f8 to 8ba8b51 Compare July 8, 2025 02:24
arthmis
arthmis commented Jul 8, 2025
View reviewed changes
@arthmis arthmis force-pushed the lmassiah/issue-37695 branch from 1c283cd to 22d954a Compare July 8, 2025 17:52
arthmis
arthmis commented Jul 8, 2025
View reviewed changes
@arthmis arthmis force-pushed the lmassiah/issue-37695 branch 2 times, most recently from d2f8c0a to 27fc3a5 Compare July 9, 2025 12:43
@arthmis arthmis marked this pull request as ready for review July 9, 2025 12:43
arthmis added 15 commits August 8, 2025 14:34
@arthmis
implement transformations on path segments
605d0da 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
update idl for addPath
b5cd8f8 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
update webidl for canvas rendering to have one addPath method and update
5f86a50 
implementation of it for Path2D

Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
add most of the implementation for step 6 and 8 of addPath
a17692b 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
add implementation for last point and complete addPath steps
25e8ad2 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
refactor last_point function
0c41149 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
only push last point if it is a pathsegment that isn't ClosePath
26d59b4 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
remove redundant empty check
80c7a72 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
update DOMMatrix2DInit-validate-fixup.html.ini to expect PASS when
6D40
564350c 
running test

Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
run ./mach update-wpt for now passing tests
84cf9b8 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
add implementation to compute last point when adding a path
ca23201 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
add some test cases when computing last point and updating
61e7bb3 
implementation to fix test cases that fail

Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
update ClosePath implementation to be closer to the spec by preventing
55f7e87 
insertion of ClosePath segments in an empty path or having consecutive
ClosePath segments

Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
add more test cases and adjust implementation to pass tests
729037e 
- remove unnecessary comments and commented code - remove dead code

Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis
address nit
1b545ae 
Signed-off-by: Lloyd Massiah <artmis9@protonmail.com>
@arthmis arthmis marked this pull request as draft August 8, 2025 18:36
@arthmis
make changes to use kurbo::BezPath instead of the removed PathSegment
ae3f8af 
Signed-off-by: arthmis <artmis9@protonmail.com>
@arthmis arthmis force-pushed the lmassiah/issue-37695 branch from af9ce08 to ae3f8af Compare August 8, 2025 19:55
@arthmis arthmis marked this pull request as ready for review August 8, 2025 20:00
@arthmis
make tweak to comment
4bccc63 
Signed-off-by: arthmis <artmis9@protonmail.com>
sagudev
sagudev reviewed Aug 8, 2025
View reviewed changes
@arthmis
Copy link
Contributor Author
arthmis commented Aug 8, 2025

@sagudev sorry this took a bit, I had to pause working on this. I've updated add_path to use kurbo::BezPath. I think the implementation was straightforward but pls let me know if I've missed anything glaring.

Oh actually, what did you mean by BezPath already uses transform?

@arthmis
remove unnecessary tests after integrating kurbo::BezPath
5063e92 
Signed-off-by: arthmis <artmis9@protonmail.com>
@sagudev
Copy link
Member
sagudev commented Aug 8, 2025

Oh actually, what did you mean by BezPath already uses transform?

Kurbo has https://docs.rs/kurbo/latest/kurbo/struct.BezPath.html#method.apply_affine which applies transform on path.

arthmis added 2 commits August 8, 2025 16:13
@arthmis
remove redundant borrows and simplify path transformation
6c57eef 
Signed-off-by: arthmis <artmis9@protonmail.com>
@arthmis
fix some lint warnings
af3cc0b 
Signed-off-by: arthmis <artmis9@protonmail.com>
@sagudev sagudev added this pull request to the merge queue Aug 9, 2025
Merged via the queue into servo:main with commit 86d7f4c Aug 9, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagudev sagudev sagudev approved these changes

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement optional transform argument in addPath
2 participants
@arthmis @sagudev
0