Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

Rebuild widgets screen (and equivalent in Customizer) to use details and summary tags #240

Merged
merged 34 commits into from
Oct 12, 2023
Merged

Conversation

KTS915
Copy link
Member

@KTS915 KTS915 commented Oct 7, 2023

This PR follows those for the menu screen, dashboard widgets, and post/page metaboxes in replacing widgets on the admin widgets screen with HTML5 disclosure widgets, which utilize the details and summary tags.

Motivation and context

Addresses Issue #224.
Improves accessibility and reduces the use of JavaScript.

How has this been tested?

On my localhost test site

Screenshots

After

Screenshot at 2023-10-07 00-46-42
Screenshot at 2023-10-07 09-45-45

Types of changes

  • New feature

KTS915 and others added 29 commits September 29, 2023 09:48
This significantly improves accessibility while also updating the admin to use HTML5 and less JavaScript.
Enables all the media widgets (image, audio, video, etc) to work with more semantic markup and the `details` and `summary` elements.
@mattyrob
Copy link
Collaborator

mattyrob commented Oct 8, 2023

@KTS915 - I've been looking into the test failures and trying to fix what I can.

There are 2 failing PHPUnit tests, once is a simple fix, the other links to the change in src/wp-admin/includes/widgets.php from:
$params[0]['before_widget'] = "<div id='widget-{$i}_{$id}' class='widget'$hidden>";
To:
$params[0]['before_widget'] = '<'. $tag . ' id="widget-' . $i . '_' . $id . '" class="widget open">';

The test now fails for 2 reasons, a regular expression is looking for an <li> tag on a new line with a class of widget contains in single quotes, and it's the only class.

The current code changes the tag, and also the quote type from single to double quotes and it also adds the open class name. Would it still work without the open class name added and with the line as follows:
$params[0]['before_widget'] = "<{$tag} id='widget-{$i}_{$id}' class='widget'>";

@KTS915
Copy link
Member Author

KTS915 commented Oct 8, 2023

@KTS915 - I've been looking into the test failures and trying to fix what I can.

There are 2 failing PHPUnit tests, once is a simple fix, the other links to the change in src/wp-admin/includes/widgets.php from: $params[0]['before_widget'] = "<div id='widget-{$i}_{$id}' class='widget'$hidden>"; To: $params[0]['before_widget'] = '<'. $tag . ' id="widget-' . $i . '_' . $id . '" class="widget open">';

The test now fails for 2 reasons, a regular expression is looking for an <li> tag on a new line with a class of widget contains in single quotes, and it's the only class.

The current code changes the tag, and also the quote type from single to double quotes and it also adds the open class name. Would it still work without the open class name added and with the line as follows: $params[0]['before_widget'] = "<{$tag} id='widget-{$i}_{$id}' class='widget'>";

Thanks very much for doing all that, @mattyrob!

I remember I added the class open to stop the widget contents being hidden when a widget was opened. But it seems that later changes I made have rendered that unnecessary because, yes, your suggestion works perfectly!

@mattyrob
Copy link
Collaborator

mattyrob commented Oct 9, 2023

@KTS915 - could you just double check that the JavaScript coding standards changes haven't broken anything in usage and we can consider this for merge later this week.

@KTS915
Copy link
Member Author

KTS915 commented Oct 9, 2023

@mattyrob I've just pushed a commit to eradicate an error message in the console. Other than that, I can't see any problems with your fixes (and I doubt that one was caused by anything you did anyway).

However, I also found that the PHP change to using $params[0]['before_widget'] = "<{$tag} id='widget-{$i}_{$id}' class='widget'>"; in widgets.php did affect some widgets (the ones that use media, such as Image, Video, and Audio). Obviously, I just didn't test the right ones before. The issue is, indeed, the lack of the class of open.

But I've now found a better way of addressing that by checking for the open attribute instead and have pushed that fix too. So now I think we're good to go.

Copy link
Member

@xxsimoxx xxsimoxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having problems in safari. Widget section is not looking good, and in the customizer the toggles are not working.
Schermata 2023-10-10 alle 10 26 39

Looks good in Chrome and Firefox.
Even a better UX for me:
Before:
Schermata 2023-10-10 alle 10 24 01
After:

Schermata 2023-10-10 alle 10 33 50

I have just read an article by Scott O'Hara (https://www.scottohara.me/blog/2022/09/12/details-summary.html), where he says that removing the native marker for the disclosure widget can cause accessibility problems because some assistive technology looks to the marker instead of the status of the `open` attribute to know whether the widget is open or not! Yes, it's completely crazy!

So this commit goes back to using the native marker but makes it bigger. Which, really, I suppose is what I should have done in the first place. I will submit a PR to make a similar change for the dashboard widgets, post metaboxes, and menu items that we've already included.
@KTS915
Copy link
Member Author

KTS915 commented Oct 10, 2023

@xxsimoxx I haven't got a way yet of testing on Safari directly, but I have installed GNOME Web, which uses the same rendering engine (and uses the same markup for the disclosure widget marker). I found much the same as you initially but, when I did a hard refresh, it worked properly.

If a hard refresh doesn't work for you, I'll set up a site outside my desktop and test with Safari online.

@xxsimoxx
Copy link
Member

@KTS915 a (command line) cache reset made the trick on safari.

Copy link
Member

@viktorix viktorix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good on Windows.

@mattyrob mattyrob merged commit 396dabe into ClassicPress:develop Oct 12, 2023
9 checks passed
@KTS915 KTS915 deleted the better-widgets branch October 12, 2023 15:03
@mattyrob mattyrob mentioned this pull request Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants