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

Disabled buttons where the 'disabled' attribute is set after the button is created are clickable in Firefox (raw Calcite) #8729

Closed
2 of 6 tasks
nCastle1 opened this issue Feb 12, 2024 · 9 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. p - high Issue should be addressed in the current milestone, impacts component or core functionality

Comments

@nCastle1
Copy link

nCastle1 commented Feb 12, 2024

Check existing issues

Actual Behavior

Calcite buttons (and other controls, like Checkbox) that have been disabled with the disabled property still fire onclick events.

Expected Behavior

Calcite buttons and other components should suppress the onclick event when the control is disabled.

Reproduction Sample

Gist (I don't have a codepen account)

It's pretty simple though:

<calcite-button id="abc" onclick="alert()">Test button</calcite-button>

<calcite-button onclick="document.getElementById('abc').setAttribute('disabled', 'true')">Disable other button</calcite-button>

Reproduction Steps

  1. Click the 'disable other button' button and notice that the other button becomes disabled
  2. Click the disabled button, and notice that the onclick event fires

Reproduction Version

2.4.0

Relevant Info

Observed in Firefox on Mac.

I believe an earlier issue with firefox buttons not honoring the 'disabled' property was partially addressed in this PR: https://github.com/Esri/calcite-design-system/pull/7107/files.

I played around with a local build and have a PR that appears to fix the problem, but this was made to satisfy my curiosity and is more of a proof of concept than a merge-worthy PR.

There is another issue filed specifically for React that I suspect shares a root cause with this issue, but I don't know. #8203

Regression?

No response

Priority impact

p3 - want for upcoming milestone

Impact

The ArcGIS Maps SDK uses Calcite Components in quite a few widgets. The workaround (check disabled state within the event handler) is straightforward, but will have to be done in several places.

Because the code for implementing the current Firefox disablement fix is shared between several components, several components are likely impacted. I only checked Checkbox and Button.

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-angular
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Maps SDK for JavaScript

@nCastle1 nCastle1 added 0 - new New issues that need assignment. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. needs triage Planning workflow - pending design/dev review. labels Feb 12, 2024
@github-actions github-actions bot added ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. p3 - want for upcoming milestone calcite-components Issues specific to the @esri/calcite-components package. labels Feb 12, 2024
@nCastle1 nCastle1 changed the title Disabled buttons where the 'disabled' attribute is set in tsx are clickable in Firefox (raw Calcite with Maquette) Disabled buttons where the 'disabled' attribute is set after the button is created are clickable in Firefox (raw Calcite) Feb 12, 2024
@jcfranco
Copy link
Member

Thanks for reporting this. It looks like we might be able to drop the FF-specific workaround as v116 is covered by FF ESR (v115+). Local testing looks good for both #7043 and this use case (codepen).

@jcfranco jcfranco added p - medium Issue is non core or affecting less that 60% of people using the library estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. and removed needs triage Planning workflow - pending design/dev review. labels Feb 13, 2024
@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Feb 13, 2024
@jcfranco jcfranco self-assigned this Feb 13, 2024
@nCastle1
Copy link
Author

I tested the fix in the linked PR and it works as expected in Maps SDK for JS with Firefox v122. Thank you!

@jcfranco
Copy link
Member

@nCastle1 No, thank you for the detailed description and quickly testing the fix. We really appreciate it! 🙌

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Feb 13, 2024
@jcfranco
Copy link
Member

Thanks for reporting this. It looks like we might be able to drop the FF-specific workaround as v116 is covered by FF ESR (v115+). Local testing looks good for both #7043 and this use case (codepen).

Apparently, I can't math in the evening. 😅 We'll have to wait until the next ESR update to install this, so 116 is covered. According to https://whattrainisitnow.com/calendar/, it looks like July 9th this year.

so-close

I'll provide an alternative solution for this. Sorry for the hassle!

@dasa
Copy link
Member

dasa commented Feb 13, 2024

According to whattrainisitnow.com/calendar, it looks like July 9th this year.

Existing ESR users won't be offered the upgrade to 128 until 128.3 on October 1st.

@jcfranco
Copy link
Member

@dasa Why you gotta kick a dev when they're down?🦿😭 lol

Serious talk, thanks for clarifying. Will update my linked PR.

@jcfranco
Copy link
Member

@dasa Sharing our convo for posterity, the ESR will be available for upgrade at version X.2.0, so our upgrade date is now September 3rd.

@jcfranco jcfranco added p - high Issue should be addressed in the current milestone, impacts component or core functionality and removed p - medium Issue is non core or affecting less that 60% of people using the library labels Feb 14, 2024
jcfranco added a commit that referenced this issue Feb 14, 2024
…ion (Firefox) (#8746)

**Related Issue:** #8729 

## Summary

This updates the interactive util to track disabled component parents
(needed for Firefox workaround) as the component is disabled and
enabled.

**Note**: test environment is Chromium-based, so no tests were updated.
@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Feb 14, 2024
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 Feb 14, 2024
@geospatialem
Copy link
Member

Verified in Firefox with 2.5.0-next.4 and https://codepen.io/geospatialem/pen/JjzwvqV.

Elijbet pushed a commit that referenced this issue Feb 15, 2024
…ion (Firefox) (#8746)

**Related Issue:** #8729 

## Summary

This updates the interactive util to track disabled component parents
(needed for Firefox workaround) as the component is disabled and
enabled.

**Note**: test environment is Chromium-based, so no tests were updated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Maps SDK for JavaScript Issues logged by ArcGIS SDK for JavaScript team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. p - high Issue should be addressed in the current milestone, impacts component or core functionality
Projects
None yet
Development

No branches or pull requests

5 participants