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.

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

Bug: calcite-radio-button: destroying throws errors #977

Closed
kevindoshier opened this issue Sep 11, 2020 · 17 comments · Fixed by #1011 or #1344
Closed

Bug: calcite-radio-button: destroying throws errors #977

kevindoshier opened this issue Sep 11, 2020 · 17 comments · Fixed by #1011 or #1344
Assignees
Labels
1 - assigned Issues that are assigned to a sprint and a team member. 2 - in development Issues that are actively being worked on. bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Milestone

Comments

@kevindoshier
Copy link
Contributor

kevindoshier commented Sep 11, 2020

Summary

When trying to remove a calcite-radio-button from the DOM within a calcite-radio-button-group, we are seeing errors throw in the console about the input for the radio button in the shadow dom not having a parent, I checked the dom structure and the input is being placed outside of the label

calciteRadioButton

And when the disconnected function it fails and throws the error since there isnt a parent

destroyButton

Actual Behavior

Expected Behavior

Reproduction Steps

Relevant Info

@kevindoshier kevindoshier added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Sep 11, 2020
@jcfranco
Copy link
Member

@eriklharper Can you triage this one?

@eriklharper
Copy link
Contributor

@kevindoshier I attempted to reproduce this bug with this code, but did not see any console errors:

<calcite-label>
    Label
    <calcite-radio-button-group name="s" scale="s">
      <calcite-radio-button value="stencil" checked>Stencil</calcite-radio-button>
      <calcite-radio-button value="react">React</calcite-radio-button>
      <calcite-radio-button value="ember">Ember</calcite-radio-button>
      <calcite-radio-button value="angular">Angular</calcite-radio-button>
    </calcite-radio-button-group>
  </calcite-label>
<script>
  setTimeout(function() {
    const btn = document.querySelector("calcite-radio-button");
    btn.parentNode.removeChild(btn);
  }, 5000)
</script>

Can you provide detailed steps including code that you're using? The <input> is rendered outside the <label> on purpose, using the for attribute to link them, which is valid html, so I don't think this is related to the issue. Also, the <input> element is not in the <calcite-radio-button>'s shadowRoot. It is placed there as a slotted child of <calcite-radio-button> just outside of the shadowRoot so that it can be picked up by <form> elements wrapping the component.

@kevindoshier
Copy link
Contributor Author

The only repro code I have is in the mapViewer, but looking at the code, this line in particular

https://github.com/Esri/calcite-components/blob/master/src/components/calcite-radio-button/calcite-radio-button.tsx#L246

If the only parent of the input is the slot this will fail correct? This is where im seeing the failure at least

If you need repro steps for the mapViewer I can provide those

@kevindoshier
Copy link
Contributor Author

image

@eriklharper
Copy link
Contributor

The slot element is not actually in the dom, the parent of input in this case is the host calcite-radio-button element. Yes, I would appreciate some repro steps since I can't reproduce this in calcite components.

@kevindoshier
Copy link
Contributor Author

kevindoshier commented Sep 11, 2020

  1. Navigate to
    https://devtesting.mapsdevext.arcgis.com/apps/mapviewer/index.html?layers=77cbb84d9fdc426f9f815f1837a744ba

  2. Click Sign in in upper right use

  3. Click Map Properties in the left action bar
    image

  4. Click Time Slider Options
    image

  5. Click the back arrow at the top of the flow item
    image

  6. Repeat step 4, click time slider options again, for every radio button in the timeSlider options flow item you should get a console error

@kevindoshier
Copy link
Contributor Author

kevindoshier commented Sep 11, 2020

We still do this is what we see

image

@eriklharper
Copy link
Contributor

Those login credentials aren't working for me.

@kevindoshier
Copy link
Contributor Author

kevindoshier commented Sep 11, 2020

I just tried them and they worked ok

emailed them to you

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 23, 2020
@kat10140 kat10140 added 1 - assigned Issues that are assigned to a sprint and a team member. and removed Stale Issues or pull requests that have not had recent activity. labels Nov 25, 2020
@kat10140
Copy link
Contributor

@eriklharper can this be closed?

@eriklharper
Copy link
Contributor

I'm working on a fix for this in #1121, so I'll link that issue to this one and close #977 once I verify the fix in #1121. Thanks for following up!

@eriklharper
Copy link
Contributor

eriklharper commented Dec 5, 2020

The fix applied by #1344 fixes the original issue reported here, but because of a discovered Stencil + Maquette bug ionic-team/stencil#2758 the issue described in #1317 is now occurring.

A temporary fix while ionic-team/stencil#2758 is being fixed is to wrap spans around radio-button label text like:

<calcite-radio-button
  value="start"
  disabled={false}
  checked={!useSavedPosition}
  afterCreate={(element: HTMLElement) =>
    this.playbackPosition = element
  }
>
  <span>{i18n.timeProperties.startPoint}</span>
</calcite-radio-button>
<calcite-radio-button
  value="position"
  checked={useSavedPosition}
>
  <span>{i18n.timeProperties.startMapPosition}</span>
</calcite-radio-button>

@eriklharper
Copy link
Contributor

Just verified this bug is fixed without the wrapping on the radio label text with Stencil's 2.4.0-1 pre-release version that includes the fix for the bug we reported to them!

@julio8a
Copy link

julio8a commented Jan 25, 2021

What's the status of this issue? Does it need verification or can it be closed?
@eriklharper, can you provide an update. Thx!

@eriklharper
Copy link
Contributor

Needs re-verification once Calcite ships the Stencil fix, which is currently in pre-release, so it is not quite prime time yet on Stencil's end.

@eriklharper
Copy link
Contributor

Closing this in favor of other open bugs that need re-verification once Stencil's bug fixes make it into their 2.4 release like #1317 and #1121. The original subject of this ticket has been resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. 2 - in development Issues that are actively being worked on. bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
6 participants