Improve disabling checkbox for Picture Element on Media settings screen #1470

mukeshpanchal27 · 2024-08-14T04:46:22Z

Summary

In #1273, we improved the Settings -> Media controls for Modern Image Formats, where the Use <picture> Element (experimental) option is disabled if the fallback JPEG checkbox is not selected. However, the checkbox remains clickable even when disabled because we only added the disabled class without actually adding the disabled attribute to the checkbox.

This PR provides a quick workaround by disabling pointer events for the checkbox when the disabled class is present.

Screen.Recording.2024-08-14.at.9.55.36.AM.mov

github-actions · 2024-08-14T04:55:04Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter

LGTM

plugins/webp-uploads/settings.php

westonruter · 2024-08-14T05:01:24Z

plugins/webp-uploads/settings.php

 			opacity: 0.7;
 		}
+		#webp_uploads_picture_element_fieldset.disabled #webp_uploads_use_picture_element_label {
+			pointer-events: none;


That works! However, it will still be toggled with the keyboard, right?

Anyway, it doesn't really matter if the disabled checkbox can or can't be changed.

I don't understand this. Wouldn't toggling with the keyboard still be a problem as it would allow changing the value (since the checkbox is not technically disabled)?

I assume the reason we don't use the disabled attribute is that we still need to send the data with the form submission? If so, maybe we should conditionally add readonly, to ensure the value cannot be changed while it's "disabled"?

Wouldn't toggling with the keyboard still be a problem as it would allow changing the value (since the checkbox is not technically disabled)?

Yes. IMO, this is just a UX enhancement though. Also accounting for keyboard toggling the checkbox seems overkill. It's not like it's a problem if someone successfully checks or unchecks this "disabled" checkbox, since the state is ignored. The visual disabled treatment is just to let them know it doesn't have an effect, which the associated admin notice also makes clear.

This being said, to improve a11y then perhaps setting the aria-disabled=true attribute should be done to make this crystal clear. I've done this in 25d2c02.

I assume the reason we don't use the disabled attribute is that we still need to send the data with the form submission?

That is correct. I also noticed a bug where even though even though the webp_uploads_use_picture_element checkbox is checked, when the perflab_generate_webp_and_jpeg is not checked and the form is submitted, the resulting updated page will still show that the webp_uploads_use_picture_element checkbox is unchecked. This is in spite of the fact that the underlying DB option is still webp_uploads_use_picture_element. The reason for this is that the webp_uploads_is_picture_element_enabled() helper was being used instead of get_option( 'webp_uploads_use_picture_element' ), and the helper will always return false if the perflab_generate_webp_and_jpeg option is not true. I've fixed this in 8c8a294 (and 97dc627).

If so, maybe we should conditionally add readonly, to ensure the value cannot be changed while it's "disabled"?

Unfortunately, readonly doesn't prevent a checkbox from being toggled: https://codepen.io/westonruter/pen/ExBbzPx

It's not like it's a problem if someone successfully checks or unchecks this "disabled" checkbox, since the state is ignored.

But I thought it isn't really ignored, since the checkbox isn't actually disabled, but only has the disabled class. So as far as I understand, it'll send the value to the backend, which then IMO makes it strange if you can change it (even though that probably won't have any real behavior implications).

Unfortunately, readonly doesn't prevent a checkbox from being toggled: https://codepen.io/westonruter/pen/ExBbzPx

Ah, didn't know that, that's too bad. The other alternative to solve this would be to actually disable, while temporarily injecting an input[type="hidden"] that's just there to continue sending the previous value back to the backend - since without sending it, WordPress would update the option to null.

But I thought it isn't really ignored, since the checkbox isn't actually disabled, but only has the disabled class.

Sorry, by ignored I mean that webp_uploads_is_picture_element_enabled() will always return false when the perflab_generate_webp_and_jpeg option is false, regardless of whether the webp_uploads_use_picture_element option is true or false.

So as far as I understand, it'll send the value to the backend, which then IMO makes it strange if you can change it (even though that probably won't have any real behavior implications).

So yes, technically you could change it, but it won't have any behavioral effect.

The other alternative to solve this would be to actually disable, while temporarily injecting an input[type="hidden"] that's just there to continue sending the previous value back to the backend - since without sending it, WordPress would update the option to null.

That's right. But this seems like maybe overkill.

Actually, thinking about it further: Why don't we simply add disabled to the checkbox than doing all these workarounds? If the checkbox is disabled, nothing will be sent to the backend. This will set the webp_uploads_use_picture_element option to null - which is the same as an unchecked checkbox - which is actually what we want: If the checkbox is disabled, you can't use picture element, so it makes sense for the option value to be false-y.

Because let's say you had PICTURE element enabled, but then you temporarily turned off JPEG fallback. When you go back to turn on JPEG fallback, you'd then find that your PICTURE elements aren't automatically being served again. Keeping a non-disabled input for webp_uploads_use_picture_element ensures that the option isn't mutated unnecessarily. The webp_uploads_is_picture_element_enabled() helper function takes care of whether or not you can use PICTURE element based on the state of the two checkboxes.

Okay, that makes sense. In that case though, I think my previous suggestion of using the disabled attribute on the checkbox and temporarily injecting input[type="hidden"] is a cleaner solution.

We'd only need to inject that additional element if the checkbox was previously checked anyway / the option was true. Otherwise the missing element will lead to the same behavior as the unchecked checkbox would.

Implemented in 42b1129. I found it simpler to not have to inject/remove the hidden input, but to just leave it all the time.

Screen.recording.2024-08-14.10.44.47.webm

Thanks, @westonruter, for updating the PR. The latest changes look good to me.

Clever way to exclude the checkbox in form submission.

felixarntz

@mukeshpanchal27 One point of feedback in relation to @westonruter's comment.

felixarntz · 2024-08-14T15:22:43Z

plugins/webp-uploads/settings.php

 			opacity: 0.7;
 		}
+		#webp_uploads_picture_element_fieldset.disabled #webp_uploads_use_picture_element_label {
+			pointer-events: none;


I don't understand this. Wouldn't toggling with the keyboard still be a problem as it would allow changing the value (since the checkbox is not technically disabled)?

I assume the reason we don't use the disabled attribute is that we still need to send the data with the form submission? If so, maybe we should conditionally add readonly, to ensure the value cannot be changed while it's "disabled"?

…cture_element_enabled() helper

… checkbox() input

plugins/webp-uploads/settings.php

felixarntz · 2024-08-15T20:13:44Z

plugins/webp-uploads/settings.php

+			<input
+				type="checkbox"
+				id="webp_uploads_use_picture_element"
+				aria-describedby="webp_uploads_use_picture_element_description"
+				<?php checked( get_option( 'webp_uploads_use_picture_element', false ) ); // Option intentionally used instead of webp_uploads_is_picture_element_enabled() to persist when perflab_generate_webp_and_jpeg is updated. ?>
+				<?php disabled( ! $jpeg_fallback_enabled ); ?>
+				onchange="document.getElementById('webp_uploads_use_picture_element_value').value = this.checked ? 1 : 0"
+			>


Does the lack of a name attribute on this have any unintended side effects? Makes sense we don't send it with the form submission in favor of the hidden input, but I wonder if this leads to other problems (is the name attribute used for anything else?).

Alternatively, we avoid tampering with this (user-facing) element completely and inject the hidden input only when needed (if the checkbox is disabled). This could then happen in the above event listener for #perflab_generate_webp_and_jpeg, which IMO is cleaner in terms of code organization too as it colocates all the dynamic logic in one place based on the condition (rather than the onchange attribute here that feels a bit out of place).

Does the lack of a name attribute on this have any unintended side effects? Makes sense we don't send it with the form submission in favor of the hidden input, but I wonder if this leads to other problems (is the name attribute used for anything else?).

No, AFAIK the name attribute is only relevant here in that it includes the value in the form submission.

Alternatively, we avoid tampering with this (user-facing) element completely and inject the hidden input only when needed (if the checkbox is disabled). This could then happen in the above event listener for #perflab_generate_webp_and_jpeg, which IMO is cleaner in terms of code organization too as it colocates all the dynamic logic in one place based on the condition (rather than the onchange attribute here that feels a bit out of place).

IMO, adding and removing a hidden input makes the code more complicated. But if you'd like to see that, feel free to amend the PR.

IMO it only makes the code more, but not more complicated. Happy to add this.

Co-authored-by: Felix Arntz <[email protected]>

Co-authored-by: felixarntz <[email protected]>

…into fix/disable-input

… exclusively concerns this part of the UI.

felixarntz · 2024-08-15T20:57:50Z

@westonruter I implemented the placing and removing of the hidden input in a039791. Please review that commit individually as it makes it easier.

In 9e5d40f I moved the <script> to the other PHP callback since IMO it makes more sense there. While it's about listening to the other element, everything that is part of the logic concerns the UI in that callback, so I think it makes more sense to colocate. We could even use PHP variables for the name attributes etc and share them between the PHP and JS code to make it more error-proof, though I didn't do that for now. Can be added if you think it's valuable.

westonruter

@felixarntz LGTM, just a few minor suggestions

plugins/webp-uploads/settings.php

westonruter · 2024-08-15T21:24:25Z

plugins/webp-uploads/settings.php

+				hiddenInput.setAttribute( 'type', 'hidden' );
+				hiddenInput.setAttribute( 'id', 'webp_uploads_use_picture_element_hidden' );
+				hiddenInput.setAttribute( 'name', checkbox.getAttribute( 'name' ) );
+				hiddenInput.setAttribute( 'value', checkbox.getAttribute( 'value' ) );


Less verbose if properties are used instead:

Suggested change

hiddenInput.setAttribute( 'type', 'hidden' );

hiddenInput.setAttribute( 'id', 'webp_uploads_use_picture_element_hidden' );

hiddenInput.setAttribute( 'name', checkbox.getAttribute( 'name' ) );

hiddenInput.setAttribute( 'value', checkbox.getAttribute( 'value' ) );

hiddenInput.type = 'hidden';

hiddenInput.id = 'webp_uploads_use_picture_element_hidden';

hiddenInput.name = checkbox.name;

hiddenInput.value = checkbox.value;

Does this do the same? I believe I had run into weird issues in the past, where one is not like the other, i.e. not in sync. But maybe that was a specific situation. Are you aware of any quirks with attributes vs properties in that sense?

Yeah, although in this case they will be equivalent. We're using the properties elsewhere in this file (e.g. hidden), so I think also good for consistency.

Done in ae2f0c7

westonruter · 2024-08-15T21:25:13Z

plugins/webp-uploads/settings.php

+				// The hidden input is only needed if the value was originally set (i.e. the checkbox enabled).
+				var hiddenInput = document.createElement( 'input' );
+				hiddenInput.setAttribute( 'type', 'hidden' );
+				hiddenInput.setAttribute( 'id', 'webp_uploads_use_picture_element_hidden' );


This string is duplicated three times. Should be added to a new constant that is reused?

We could do that, although then it makes me think we should rather do it across PHP and JS (i.e. define variables in PHP and pass them even to JS), to share more code between server and client. Either way works.

Yeah, defining it in PHP once and then reusing in PHP in JS sounds good to me.

Done in fa311f9

Co-authored-by: Weston Ruter <[email protected]>

westonruter · 2024-08-16T01:18:27Z

@adamsilverstein This should be ready for merge. Please review and we'll get this out the door for the Monday release!

Remove pointer event when disable

1e3520c

mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Aug 14, 2024

mukeshpanchal27 added this to the webp-uploads 2.1.0 milestone Aug 14, 2024

mukeshpanchal27 self-assigned this Aug 14, 2024

mukeshpanchal27 marked this pull request as ready for review August 14, 2024 04:54

mukeshpanchal27 requested review from adamsilverstein, felixarntz and westonruter as code owners August 14, 2024 04:54

Correct inline comment

c32e463

mukeshpanchal27 requested a review from joemcgill August 14, 2024 04:58

westonruter approved these changes Aug 14, 2024

View reviewed changes

felixarntz reviewed Aug 14, 2024

View reviewed changes

westonruter added 4 commits August 14, 2024 09:25

Add aria-disabled attribute

25d2c02

Use option value directly in checked() rather than webp_uploads_is_pi…

8c8a294

…cture_element_enabled() helper

Improve readability of checkbox input and add explanatory comment for…

97dc627

… checkbox() input

Use a disabled checkbox with a hidden input to preserve state

42b1129

westonruter requested a review from felixarntz August 15, 2024 18:23

felixarntz reviewed Aug 15, 2024

View reviewed changes

westonruter and others added 6 commits August 15, 2024 13:16

Put attributes on separate lines

d84831c

Co-authored-by: Felix Arntz <[email protected]>

Improve variable naming

7a50264

Co-authored-by: felixarntz <[email protected]>

Reuse variable instead of calling get_option() again

67485b4

Co-authored-by: felixarntz <[email protected]>

Place hidden input only when needed.

a039791

Merge branch 'fix/disable-input' of github.com:WordPress/performance …

0509452

…into fix/disable-input

Move script to use_picture_element checkbox PHP callback as it almost…

9e5d40f

… exclusively concerns this part of the UI.

westonruter reviewed Aug 15, 2024

View reviewed changes

felixarntz and others added 2 commits August 15, 2024 15:05

Use const instead of var.

f9a7517

Co-authored-by: Weston Ruter <[email protected]>

Define hidden element ID once in PHP

fa311f9

Use element properties instead of attributes

ae2f0c7

westonruter merged commit d6c1b0d into trunk Aug 16, 2024

westonruter deleted the fix/disable-input branch August 16, 2024 23:16

westonruter changed the title ~~Modern Image Formats: Checkbox disable is not working for Settings->Media controls~~ Improve disabling checkbox for Picture Element on Media settings screen Aug 16, 2024

Improve disabling checkbox for Picture Element on Media settings screen #1470

Improve disabling checkbox for Picture Element on Media settings screen #1470

Uh oh!

Conversation

mukeshpanchal27 commented Aug 14, 2024

Summary

Uh oh!

github-actions bot commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Aug 15, 2024

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

github-actions bot commented Aug 14, 2024 •

edited

Loading

felixarntz Aug 14, 2024 •

edited

Loading