Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292

arturi · 2023-01-31T18:34:59Z

Before:

After:

aduh95

Using CSS for this seems weird, but if that's working, it's all good man.

arturi · 2023-01-31T18:59:13Z

:-) Seems so. Do you think it's more future-proof to set disabled on each button, link and input? Could mess something up if we use disabled somewhere and then changing to disabled: false will change it for every element and remove the intended one. Unless we store the disabled condition before removing it, like we do with tabindex.

arturi · 2023-01-31T19:03:48Z

Fixes #4223 (comment), @ahardin

aduh95 · 2023-01-31T19:08:20Z

Do you think it's more future-proof to set disabled on each button, link and input?

Alternatively, we could use a <fieldset> to wrap the dashboard, and apply the disabled attribute on it, which would cascade down to all the Uppy form elements.

arturi · 2023-01-31T19:11:08Z

Good point. So a conditional fieldset that only wraps when disabled: true, right? Wouldn't want to always have it there.

aduh95 · 2023-01-31T22:09:52Z

Wouldn't want to always have it there.

Why not?

mifi · 2023-02-01T00:12:22Z

packages/@uppy/dashboard/src/style.scss

@@ -198,6 +198,13 @@
  fill: #9f9f9f;
 }

+// Disallow clicking on all interactive elements
+.uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap {
+  button, a, input, select, textarea {


why only button, a, input, select, textarea ? can't we just disable pointer-events for everything, e.g the whole dashboard or all children?

alternatively if we want to make it more fine-grained (e.g. still allow some things to be clicked), then I think it's cleaner to do it in javascript (e.g. if (disabled) return) so that it won't have unintended consequences

Thought it’s more performant to target interactive elements, those that support the disabled attribute, than doing .uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap *.

what about .uppy-Dashboard--isDisabled .uppy-Dashboard-innerWrap?

That is currently the case

uppy/packages/@uppy/dashboard/src/style.scss

Lines 176 to 195 in 3e796ee

.uppy-Dashboard-innerWrap {

position: relative;

display: flex;

flex-direction: column;

height: 100%;

overflow: hidden;

border-radius: 5px;

opacity: 0;

.uppy-Dashboard--isInnerWrapVisible & {

opacity: 1;

}

.uppy-Dashboard--isDisabled & {

opacity: 0.6;

filter: grayscale(100%);

user-select: none;

pointer-events: none;

}

}

, doesn't propagate to children.

ah ok! The only problem with doing this selective disabling as I see it is that if someone were to add a different button type, e.g. <div role="button"> in the future, then this disabling functionality would stop working and nobody will notice.

arturi · 2023-02-01T00:16:17Z

Why not always have fieldset?

Because sometimes our views might have no buttons/inputs, otherwise not a form, and it’s weird (semantically incorrect) to have everything be wrapped in an element meant to group things in a form?

mifi · 2023-02-01T00:28:45Z

Because sometimes our views might have no buttons/inputs, otherwise not a form, and it’s weird (semantically incorrect) to have everything be wrapped in an element meant to group things in a form?

then i would argue it's also semantically wrong and a bit hacky to suddenly "appear" a fieldset on demand just to be able to disable all form elements.

arturi · 2023-02-01T00:32:40Z

So the CSS solution seams like the least problematic? We set tabindex=-1 with JS, so no keyboards / voice input, and disable clicking with CSS.

Otherwise we can make a rule to wrap all buttons, inputs etc in a fieldset, but seems a bit redundant for just this one option.

aduh95 · 2023-02-01T10:16:51Z

So the CSS solution seams like the least problematic?

Do we know how accessibility software respond to it vs the use of the disabled attribute? I would expect that disabled is the most accessible solution.

packages/@uppy/dashboard/src/Dashboard.jsx

arturi · 2023-02-03T14:40:52Z

Changed to disabled, thrown away tabindex=-1. Seems to work well.

packages/@uppy/dashboard/src/Dashboard.jsx

… into fix-disabled-mode

| Package | Version | Package | Version | | -------------------- | ------- | -------------------- | ------- | | @uppy/audio | 1.0.4 | @uppy/screen-capture | 3.0.2 | | @uppy/companion | 4.3.0 | @uppy/transloadit | 3.1.1 | | @uppy/core | 3.0.6 | @uppy/xhr-upload | 3.1.0 | | @uppy/dashboard | 3.2.2 | uppy | 3.5.0 | | @uppy/locales | 3.0.6 | | | - @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / #4316) - meta: Remove Robodog advice, since it is deprecated (Artur Paikin) - @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / #4306) - @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / #4292) - @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / #4302) - @uppy/locales: Update de_DE.js (Jörn Velten / #4297) - meta: use load balancer for companion in e2e tests (Mikael Finstad / #4228) - @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / #4286) - @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / #4247) - @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / #4282)

| Package | Version | Package | Version | | -------------------- | ------- | -------------------- | ------- | | @uppy/audio | 1.0.4 | @uppy/screen-capture | 3.0.2 | | @uppy/companion | 4.3.0 | @uppy/transloadit | 3.1.1 | | @uppy/core | 3.0.6 | @uppy/xhr-upload | 3.1.0 | | @uppy/dashboard | 3.2.2 | uppy | 3.5.0 | | @uppy/locales | 3.0.6 | | | - @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / transloadit#4316) - meta: Remove Robodog advice, since it is deprecated (Artur Paikin) - @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / transloadit#4306) - @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / transloadit#4292) - @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / transloadit#4302) - @uppy/locales: Update de_DE.js (Jörn Velten / transloadit#4297) - meta: use load balancer for companion in e2e tests (Mikael Finstad / transloadit#4228) - @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / transloadit#4286) - @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / transloadit#4247) - @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / transloadit#4282)

Disallow clicking on buttons and links in Dashboard disabled mode

3e796ee

arturi requested a review from aduh95 January 31, 2023 18:39

arturi changed the title ~~Disallow clicking on buttons and links in Dashboard disabled mode~~ Dashboard: disallow clicking on buttons and links in Dashboard disabled mode Jan 31, 2023

aduh95 approved these changes Jan 31, 2023

View reviewed changes

arturi requested a review from mifi January 31, 2023 18:59

arturi mentioned this pull request Jan 31, 2023

@uppy/react: deprecate useUppy #4223

Merged

mifi reviewed Feb 1, 2023

View reviewed changes

arturi added 2 commits February 2, 2023 23:53

Add disabled attr to all interactive elements instead of tabindex=-1

27838d6

Merge branch 'main' into fix-disabled-mode

ce4761e

aduh95 reviewed Feb 3, 2023

View reviewed changes

packages/@uppy/dashboard/src/Dashboard.jsx Outdated Show resolved Hide resolved

aduh95 reviewed Feb 3, 2023

View reviewed changes

packages/@uppy/dashboard/src/Dashboard.jsx Outdated Show resolved Hide resolved

arturi added 4 commits February 3, 2023 20:11

Test and apply Antoine’s suggestions

d0855f8

Merge branch 'fix-disabled-mode' of https://github.com/transloadit/uppy…

d6bc109

… into fix-disabled-mode

Account for links, use aria-disabled and pointer-events

2edbf35

Rename disableAllFocusableElements --> disableInteractiveElements

871133d

arturi requested a review from aduh95 February 3, 2023 21:00

Add role="button" just in case for the future

4457986

arturi merged commit a602f1e into main Feb 11, 2023

arturi deleted the fix-disabled-mode branch February 11, 2023 14:53

github-actions bot mentioned this pull request Feb 13, 2023

Release: uppy@3.5.0 #4318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292

Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292

arturi commented Jan 31, 2023 •

edited

aduh95 left a comment

arturi commented Jan 31, 2023

arturi commented Jan 31, 2023

aduh95 commented Jan 31, 2023

arturi commented Jan 31, 2023

aduh95 commented Jan 31, 2023

mifi Feb 1, 2023

arturi Feb 1, 2023

mifi Feb 1, 2023

arturi Feb 1, 2023

mifi Feb 1, 2023

arturi commented Feb 1, 2023

mifi commented Feb 1, 2023

arturi commented Feb 1, 2023

aduh95 commented Feb 1, 2023

arturi commented Feb 3, 2023 •

edited

	.uppy-Dashboard-innerWrap {
	position: relative;
	display: flex;
	flex-direction: column;
	height: 100%;
	overflow: hidden;
	border-radius: 5px;
	opacity: 0;

	.uppy-Dashboard--isInnerWrapVisible & {
	opacity: 1;
	}

	.uppy-Dashboard--isDisabled & {
	opacity: 0.6;
	filter: grayscale(100%);
	user-select: none;
	pointer-events: none;
	}
	}

Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292

Dashboard: disallow clicking on buttons and links in Dashboard disabled mode #4292

Conversation

arturi commented Jan 31, 2023 • edited

aduh95 left a comment

Choose a reason for hiding this comment

arturi commented Jan 31, 2023

arturi commented Jan 31, 2023

aduh95 commented Jan 31, 2023

arturi commented Jan 31, 2023

aduh95 commented Jan 31, 2023

mifi Feb 1, 2023

Choose a reason for hiding this comment

arturi Feb 1, 2023

Choose a reason for hiding this comment

mifi Feb 1, 2023

Choose a reason for hiding this comment

arturi Feb 1, 2023

Choose a reason for hiding this comment

mifi Feb 1, 2023

Choose a reason for hiding this comment

arturi commented Feb 1, 2023

mifi commented Feb 1, 2023

arturi commented Feb 1, 2023

aduh95 commented Feb 1, 2023

arturi commented Feb 3, 2023 • edited

arturi commented Jan 31, 2023 •

edited

arturi commented Feb 3, 2023 •

edited