Handle <use> children in clipPath collection and add unit test#432
Handle <use> children in clipPath collection and add unit test#432
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd1a310a8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (const auto* computedShadow = child.try_get<ComputedShadowTreeComponent>(); | ||
| computedShadow && computedShadow->mainBranch) { | ||
| for (const Entity shadowEntity : computedShadow->mainBranch->shadowEntities) { | ||
| appendClipPathFromEntity(EntityHandle(registry_, shadowEntity)); |
There was a problem hiding this comment.
collectClipPaths expands mainBranch->shadowEntities for every <use> child here, but this path never checks whether the <use> element itself is renderable first. In a <clipPath>, a <use display="none"> should contribute no geometry, yet its shadow entities are still appended (their own display defaults to visible), producing a non-empty clip and incorrect rendering. Gate this expansion on the <use> child’s computed display/visibility (or propagate ancestor display-none state while walking the shadow tree).
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
==========================================
- Coverage 78.77% 78.74% -0.04%
==========================================
Files 268 268
Lines 18499 18534 +35
Branches 3351 3359 +8
==========================================
+ Hits 14572 14594 +22
- Misses 3091 3097 +6
- Partials 836 843 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…t targets Shadow entities from <use> elements don't carry shape data directly — they reference a light entity via ShadowEntityComponent. The clip path collection now resolves shadow entities to their light entity for ComputedPathComponent lookup, and only checks the shadow root (not all descendants) to correctly reject <use> references to <g> or <symbol> elements.
jwmcglynn
force-pushed
the
codex/fix-bug-in-issue-238
branch
from
e3e8a6e to
94d565d
Compare
1 participant
Motivation
<use>children and simplify clip-path collection logic.Description
appendClipPathFromEntitylambda insideRenderingContext::collectClipPathsto reduce duplication and centralize visibility/display checks.<use>children by detectingElementType::Useand walking theComputedShadowTreeComponentmainBranchto collect clip paths from shadowed entities.e-clipPath-044.svgtest in the resvg test instantiation and added a new unit testSVGClipPathElementTests.RenderingUseChildthat verifies aclipPathwith a directusechild renders correctly.Testing
donner/svg/tests, includingSVGClipPathElementTests.RenderingUseChild, and they passed.resvg_test_suite) for theClipPathgroup withe-clipPath-044.svgnow enabled, and the suite passed.Resolves #238
Codex Task