Skip to content
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

Accordion Item: Console error thrown if item is in shadow dom #6167

Closed
paulcpederson opened this issue Dec 23, 2022 · 4 comments
Closed

Accordion Item: Console error thrown if item is in shadow dom #6167

paulcpederson opened this issue Dec 23, 2022 · 4 comments
Assignees
@geospatialem
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Online Issues logged by ArcGIS Online team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 2 Small fix or update, may require updates to tests. p - low Issue is non core or affecting less that 10% of people using the library
Milestone
2023 June patch p...

Comments

@paulcpederson
Copy link
Member

Actual Behavior

If I have an accordion, and then a component that uses shadow dom inside which renders an accordion item, I see an error in the console

TypeError: Cannot read properties of null (reading 'querySelectorAll')
    at p.getItemPosition (p-3393c6b5.entry.js:6:6848)
    at p.componentDidLoad (p-3393c6b5.entry.js:6:4617)

getItemPosition calls querySelectorAll on the element's parent which is not valid here: https://github.com/Esri/calcite-components/blob/4b4d8b014665172d7e9ac08da4471af837a37b38/src/components/accordion-item/accordion-item.tsx#L295

Expected Behavior

I would expect the accordion element to be able to work across Shadow DOM boundaries. I think this would be as straight forward as using the closestElementCrossShadowBoundary utility to look up the parent accordion, rather than assuming it's there.

If that's not possible I think it would be fine to do an early return if the parent doesn't exist. The component seems to work fine (probably because we allow multiple items to be open at a time, so none of the registration seems to matter).

Reproduction Sample

https://codepen.io/paulcp/pen/MWBYdew?editors=1011

Reproduction Steps

  1. Open the console
  2. Notice the error

Reproduction Version

All

Relevant Info

All

Regression?

No response

Impact

Minor annoyance, I am happy to open a pr if the team has insufficient bandwidth and agrees with the direction outlined above.

Esri team

ArcGIS Online
@paulcpederson paulcpederson added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Dec 23, 2022
@github-actions github-actions bot added the ArcGIS Online Issues logged by ArcGIS Online team members. label Dec 23, 2022
@benelan
Copy link
Member

benelan commented Dec 23, 2022

Yeah I agree using closestElementCrossShadowBoundary is what we want. We would definitely appreciate a PR if you have the time, thanks!

@geospatialem geospatialem added estimate - 2 Small fix or update, may require updates to tests. p - low Issue is non core or affecting less that 10% of people using the library labels Feb 28, 2023
@geospatialem geospatialem added this to the 2023 June Priorities milestone Feb 28, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Feb 28, 2023
@driskull
Copy link
Member

Updating to use closestElementCrossShadowBoundary will work to get the correct itemPosition. However, if an accordion item is moved position after being loaded the first time then that index won't be updated.

Ideally, we want to refactor this to just use a mutation observer on the accordion and get item positions at that point. Issue: #6038

Probably a major refactor so I guess its fine to fix this for now. cc @jcfranco

@driskull driskull self-assigned this May 25, 2023
@driskull driskull added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels May 25, 2023
driskull added a commit that referenced this issue May 25, 2023
@driskull
fix(accordion-item): support items working across shadowDOM. #6167
733f3db
@driskull driskull mentioned this issue May 25, 2023
Merged
driskull added a commit that referenced this issue May 31, 2023
@driskull
fix(accordion-item): support items working across shadowDOM (#7035)
6378e35 
**Related Issue:** #6167

## Summary

- Uses `closestElementCrossShadowBoundary` to find the parent
`calcite-accordion` across shadow DOM.
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels May 31, 2023
@github-actions github-actions bot assigned geospatialem and unassigned driskull May 31, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Jun 1, 2023
@geospatialem
Copy link
Member

Verified in 1.4.3-next.0

benelan added a commit that referenced this issue Jun 27, 2023
@calcite-admin @github-actions @benelan
chore: release latest (#7144)
2422120 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.4.3</summary>

##
[1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components@1.4.2...@esri/calcite-components@1.4.3)
(2023-06-26)

### Bug Fixes

* **accordion-item:** support items working across shadowDOM ([#7035](#7035)) ([6378e35](6378e35)), closes [#6167](#6167)
* **alert:** Sets autoCloseDuration to "medium" by default
([#7157](#7157))
([1b9a8ed](1b9a8ed))
* **alert:** Update alert queue when an alert is removed from the DOM
([#7189](#7189))
([edd59eb](edd59eb))
* **combobox, dropdown, input-date-picker, input-time-picker, popover,
tooltip:** Prevent repositioning from affecting other floating
components
([#7178](#7178))
([1b02dae](1b02dae))
* Ensure mouse events are blocked for disabled components in Firefox
([#7107](#7107))
([271d985](271d985))
* **input-date-picker:** Fix showing the placeholder when resetting the
value ([#7156](#7156))
([8d60ffd](8d60ffd))
* **input, input-number:** Allows numeric characters.
([#7213](#7213))
([739f0af](739f0af))
* **input,input-number:** Allow typing decimal separator in firefox for
arabic locale
([#7173](#7173))
([595e6f2](595e6f2))
* **list:** No longer has incorrect border width
([a810943](a810943))
* **list:** Update selectedItems property on all item selection changes
([#7204](#7204))
([da048f6](da048f6))
* **menu-item:** Ensure correct order of rendered icons ([#7098](#7098)) ([fd344e9](fd344e9)), closes [#7097](#7097)

* **navigation:** Label is no longer a required property
([#7084](#7084))
([ba2bd4d](ba2bd4d))
* **radio-button-group:** No longer focus first radio button on label
click and adds `setFocus` method.
([#7050](#7050))
([4267b8c](4267b8c))
* **radio-button, radio-button-group:** Prevent emitting events when
selecting a checked radio button
([#7102](#7102))
([77fcc81](77fcc81))
* **radio-button:** Focuses first focusable radio-button element in
group. ([#7152](#7152))
([dd7ec60](dd7ec60))
* **scrim:** Responsively set the scale of the loading spinner
([#7182](#7182))
([72c5943](72c5943))
* **tooltip:** Improve component timing
([#7172](#7172))
([106f5d2](106f5d2))
* **tree-item:** Ensure expanded tree-item is displayed when expanded
and made visible
([#7216](#7216))
([3c0fbf5](3c0fbf5))

<details><summary>@esri/calcite-components-react: 1.4.3</summary>

##
[1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components-react@1.4.2...@esri/calcite-components-react@1.4.3)
(2023-06-26)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from 1.4.3-next.7 to 1.4.3
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben Elan <no-reply@benelan.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Online Issues logged by ArcGIS Online team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. estimate - 2 Small fix or update, may require updates to tests. p - low Issue is non core or affecting less that 10% of people using the library
Projects
None yet
Milestone
2023 June patch priorities
Development

No branches or pull requests
5 participants
@paulcpederson @driskull @alisonailea @geospatialem @benelan