ClassicPress / ClassicPress-v2

NOT READY FOR PRODUCTION.
GNU General Public License v2.0
13 stars 4 forks source link

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

Closed KTS915 closed 1 year ago

KTS915 commented 1 year ago

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

mattyrob commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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.

KTS915 commented 1 year ago

@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 commented 1 year ago

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