Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

Set disabled according to new value of attribute #323

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Set disabled according to new value of attribute #323

merged 1 commit into from
Feb 7, 2022

Conversation

yuyichao
Copy link
Contributor

Rather than force disabling it.

Link to relevant issue

Description of changes

Without this change, setting the disabled attribute using setAttribute or removeAttribute doesn't work on firefox. This can be demostrated using the following html

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <script type="module" src="dist/toolkit.js"></script>
</head>
<body>
  <vscode-button id="btn">btn-0</vscode-button>
  <vscode-button id="btn2">btn2</vscode-button>
  <script>
      enabled = true;
      counter = 0;
      btn = document.getElementById('btn');
      btn2 = document.getElementById('btn2');
      function onclick() {
          btn.innerHTML = `btn-${++counter}`;
          enabled = !enabled;
          if (enabled) {
              btn.removeAttribute('disabled');
          }
          else {
              btn.setAttribute('disabled', '');
          }
      }
      btn.addEventListener('click', onclick);
      btn2.addEventListener('click', onclick);
  </script>
</body>
</html>

Clicking on either button should toggle the disable state of the first button. This works in chromium (and therefore the native version of vscode). However, on firefox, as soon as this happens, even when enabled, the first button isn't clickable anymore (clicking has no effect) (which happens for online version of vscode when used in firefox).

I'm not sure how this was supposed to work but the code here looks wrong to me. The callback is called (on both firefox and chromium) when the attribute is changed but it seems to just assume any change in the value is adding the attribute which is only the case for the initial setup.

Even though this is setting the wrong value, it works in chromium because formDisabledCallback from FormAssociated is called with the right disabled value. It appears that this callback isn't called on firefox and I'm not sure why (I assume some expected browser differences...)...

Link to forked docs site

@yuyichao
Set disabled according to new value of attribute
a59d252 
Rather than force disabling it.
yuyichao added a commit to yuyichao/digitaljs_code that referenced this pull request Jan 28, 2022
@yuyichao
Apply fix for vscode button disabling issue on firefox
2d76b22 
Ref microsoft/vscode-webview-ui-toolkit#323
@daviddossett
Copy link
Collaborator

Hey @yuyichao, thanks for bringing this up. To be clear, are you using these components in FF outside of a VS Code extension?

@yuyichao
Copy link
Contributor Author

No. I'm using this in an extension running on the web version of the vs code (first noticing on github.dev). It works correctly when the browser is chromium but not when I run it on firefox.

@yuyichao
Copy link
Contributor Author

And that html is a reduction of the webview from the extension.

@hawkticehurst
Copy link
Member

hawkticehurst commented Feb 4, 2022 •
edited
Loading

Hey @yuyichao! Just chiming in to say thank you so much for filing this PR (and apologies for the late response)!

It's been on my list all week to give it a proper review/testing but I was just hit with back-to-back high priority issues/bugs all week long. My hope is to give it a review by the end of today but if I don't get to it I promise it'll be the first thing I tackle on Monday.

@yuyichao
Copy link
Contributor Author

yuyichao commented Feb 4, 2022

No worry. I’m already shipping a patched version myself so this is not affecting me too much…

@hawkticehurst
Copy link
Member

@yuyichao Alright finally got around to reviewing/testing and this looks great! Thank you again for bringing this up/creating a PR for it!

For some added context, this code was originally added as a temporary fix to an upstream issue with the framework we use to build the components (i.e. if you were to remove the if block altogether it would completely break the disabled attribute which is not expected behavior). I meant to add this as a stop-gap fix and then evidently forgot to file the issue (so thanks again for opening this PR).

Also yeah, during the time when this was implemented vscode.dev and github.dev were still not public, so we didn't yet have plans to support anything but a Chromium-based VS Code environment, thus the fact that this specific bug was missed.

Regardless, I'm happy to merge this PR now to address your issue but will open a PR upstream to see if the underlying issue can be resolved as well.

@hawkticehurst hawkticehurst merged commit 28b087c into microsoft:main Feb 7, 2022
@yuyichao yuyichao deleted the disable branch February 7, 2022 19:25
daviddossett added a commit that referenced this pull request Feb 18, 2022
@daviddossett
Text area: update scrollbar styling to match VS Code (#338)
2a8cf79 
* Update button `disabled` attribute logic (#323)

Description of changes

Update logic for setting the `disabled` attribute value in the button component to fix a cross-browser bug.

* Add basic component usage guidelines (#322)

* Add badge and button guidelines without images

* Add checkbox guidelines

* Add divider guidelines

* Add badge artwork

* Update badge sections

* Fix image

* Update badge image

* Add hero assets for all components

* More guideline updates

* More do/don'ts

* Add radio and tag guidance

* Add text field guidelines

* Polish docs

* Remove unused image

* Remove unused image

* Update assets structure

* Fix progress ring corner radius

* Fix images paths

* Update button guidelines

* Add link to button guidelines

* Grammar fixes

* Clarify text field guidelines

* Grammar

* PR feedback

* Fix button images resolution

* Update badge artwork. Add button artwork

* Finish button artwork

* Add checkbox artwork

* Fix button size in table

* More image res fixes

* Add data grid and divider artwork

* Add dropdown artwork

* Add link artwork

* Add panels guidelines

* Add progress ring artwork

* Add radio group artwork

* Add missing alt text to radio image markdown

* Add tag artwork

* Add text area artwork

* Add text field artwork

* Fix missing badge artwork and alt text

* Fix badge border radius

* Add icon button example

* PR feedback: update checkbox, data grid and dropdown

* More PR feedback

* Update basic example titles

* Fix panel size

* Initial commit

* Try forcing scrollbar background with !important

* FIx high contrast theme
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@hawkticehurst hawkticehurst hawkticehurst approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yuyichao @daviddossett @hawkticehurst