Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix injection of script in Categories block when dropdown displayed #25026

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

westonruter
Copy link
Member

Description

Unit tests for the AMP plugin started failing once Guenberg v8.9.0: ampproject/amp-wp#5319 (comment). The issue was traced back to 7fbdda2 as part of #25020.

Currently, when a Categories block is added and the "Display as dropdown" option is enabled, selecting a category in the dropdown has no effect. This is because the script constructed by build_dropdown_script_block_core_categories() is no longer making it into the page. The reason is that gutenberg_render_block_core_categories() is appending the script to the $wrapper_markup so it is outside of the block wrapper, but after 7fbdda2 only the block wrapper ($block_root) is returned by gutenberg_apply_block_supports().

This PR fixes that problem by ensuring that <script> returned by build_dropdown_script_block_core_categories() is inserted immediately after the </select> closing tag and is placed on the inside of the $items_markup rather than being inserted after the $wrapper_markup.

This should also prevent the sprintf() call from failing if someone has a category containing string formats.

How has this been tested?

Add a Categories block to the content and enable the "Display as dropdown" option. Before this PR, no <script> is added with the block. With this PR, a <script> is added and selecting a category causes navigation to the category.

Screenshots

Before

<div class="wp-block-categories-dropdown wp-block-categories">
	<select name="cat" id="wp-block-categories-1" class="postform">
		<option value="-1">Select Category</option>
		<option class="level-0" value="3">aciform</option>
		<!-- ... -->
	</select>
</div>

After

<div class="wp-block-categories-dropdown wp-block-categories">
	<select name="cat" id="wp-block-categories-1" class="postform">
		<option value="-1">Select Category</option>
		<option class="level-0" value="3">aciform</option>
		<!-- ... -->
	</select>
	<script type="text/javascript">
	/* <![CDATA[ */
	( function() {
		var dropdown = document.getElementById( 'wp-block-categories-1' );
		function onCatChange() {
			if ( dropdown.options[ dropdown.selectedIndex ].value > 0 ) {
				location.href = "https://wordpressdev.lndo.site/?cat=" + dropdown.options[ dropdown.selectedIndex ].value;
			}
		}
		dropdown.onchange = onCatChange;
	})();
	/* ]]&gt; */
	</script>
</div>

Types of changes

Bug fix (a regression)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Sep 2, 2020

Size Change: 0 B

Total Size: 1.17 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 8.55 kB 0 B
build/block-library/editor.css 8.55 kB 0 B
build/block-library/index.js 137 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 305 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.24 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 12 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.4 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@sirreal
Copy link
Member

sirreal commented Sep 2, 2020

It may be best to revert the change that introduced the regression rather than work around the block root limitation: #25028

It was not intended to limit the output in such a way.

Thanks for the detailed report!

@westonruter
Copy link
Member Author

In any case, this PR should still be merged because the location for the JS injection was not ideal.

@sirreal sirreal self-requested a review September 4, 2020 20:56
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I've confirmed the regression (still present in current master) and the fix. This is working well, thanks!

@sirreal sirreal added [Block] Categories Affects the Categories Block [Type] Regression Related to a regression in the latest release labels Sep 7, 2020
@sirreal sirreal added this to the Gutenberg 8.9 milestone Sep 7, 2020
@youknowriad
Copy link
Contributor

While I think this is a good fix, ideally the block-supports hook shouldn't drop the extra markup if not added under the wrapper.

@sirreal
Copy link
Member

sirreal commented Sep 7, 2020

While I think this is a good fix, ideally the block-supports hook shouldn't drop the extra markup if not added under the wrapper.

I agree, is a workaround for a regression but should be considered complementary to the fix in #25028 and not a replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Categories Affects the Categories Block [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants