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.

Sign up for GitHub

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

Jump to bottom

Fix invalid HTML structure on the widgets screen #24866

Merged
merged 6 commits into from
Aug 28, 2020
Merged

Fix invalid HTML structure on the widgets screen #24866

merged 6 commits into from
Aug 28, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Aug 27, 2020 •
edited by draganescu
Loading

Description

By default every widget on the legacy screen is wrapped with a <form>.

https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/widgets.php#L205

This means that you can sometimes end up with invalid HTML.

  1. Navigate to Widgets (beta) and add a Search block to one of the block areas.
  2. Navigate to Widgets (the old screen).
  3. View the page source and search for wp-block-search.
  4. You'll see that there's a <form> inside a <form>. Firefox helpfully highlights the invalid HTML.

91242962-190de480-e78c-11ea-9214-4f7b9dff7edc

Courtesy of @noisysocks #24290 (comment)

This solution is not awesome, but it's okay. I believe that's what this filter and dynamic options were invented for anyway. Since there is no real form when the widget is also a block, this PR also hides the save button for such widgets.

How has this been tested?

  1. Navigate to Widgets (beta) and add a Search block to one of the block areas.
  2. Navigate to Widgets (the old screen).
  3. View the page source and search for wp-block-search.
  4. Confirm there's no form inside a form.
  5. Confirm everything works as expected.
  6. Repeat in customizer.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels Aug 27, 2020
@adamziel adamziel self-assigned this Aug 27, 2020
@github-actions
Copy link

github-actions bot commented Aug 27, 2020 •
edited
Loading

Size Change: +256 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 126 kB -3 B (0%)
build/block-editor/style-rtl.css 10.8 kB +1 B
build/block-editor/style.css 10.8 kB +1 B
build/block-library/index.js 136 kB +243 B (0%)
build/blocks/index.js 47.7 kB -1 B
build/components/index.js 200 kB -1 B
build/edit-post/index.js 304 kB +1 B
build/edit-site/index.js 17 kB +1 B
build/edit-widgets/index.js 11.9 kB +17 B (0%)
build/editor/index.js 45.3 kB -3 B (0%)
build/i18n/index.js 3.57 kB +1 B
build/keyboard-shortcuts/index.js 2.52 kB -1 B
build/nux/index.js 3.4 kB +1 B
build/shortcode/index.js 1.7 kB -1 B
ℹ️ 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.44 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-library/editor-rtl.css 8.52 kB 0 B
build/block-library/editor.css 8.52 kB 0 B
build/block-library/style-rtl.css 7.45 kB 0 B
build/block-library/style.css 7.46 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 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/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 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/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 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/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 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/is-shallow-equal/index.js 710 B 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/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/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

TimothyBJacobs
TimothyBJacobs reviewed Aug 27, 2020
View reviewed changes
gutenberg.php Outdated Show resolved Hide resolved
@noisysocks noisysocks mentioned this pull request Aug 27, 2020
Closed
20 tasks
@adamziel
Copy link
Contributor Author

adamziel commented Aug 27, 2020 •
edited
Loading

Don't be discouraged by failing test - it's just a lint issue, I'll fix it tomorrow morning.

@draganescu
Copy link
Contributor

draganescu commented Aug 27, 2020 •
edited
Loading

The failing test says:

FILE: /app/gutenberg.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 209 | ERROR | Missing doc comment for function
     |       | gutenberg_override_sidebar_params_for_block_widget()
     |       | (Squiz.Commenting.FunctionComment.Missing)
----------------------------------------------------------------------

Later edit: didn't see your comment @adamziel

draganescu
draganescu approved these changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

This works as described. Why do you say "This solution is not awesome" @adamziel , the dynamic_sidebar_params hook seems the right choice.

This does not affect the front-end in any way.

@noisysocks
Copy link
Member

noisysocks commented Aug 28, 2020 •
edited
Loading

Does it make sense to output the rendered Search block at all? What does submitting the Search form do?

More generally, does it make sense to render any block on widgets.php? The semantics of this page are that there is a widget control for each widget. It's not possible to preview a widget on widgets.php. It's not WYSIWYG.

In #24503, to get around this exact problem on nav-menus.php, I ended up showing the block markup inside a <textarea readonly> instead of rendering the block. What do you think of that approach?

@adamziel
Copy link
Contributor Author

@noisysocks It could work! I'm going to merge this PR regardless to at least have a correct HTML structure and then I'm going to spin a one like you just mentioned.

@adamziel adamziel merged commit cefab03 into master Aug 28, 2020
@adamziel adamziel deleted the fix/invalid-html-on-legacy-widgets-screen branch August 28, 2020 16:08
@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 28, 2020
@adamziel adamziel mentioned this pull request Aug 28, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyBJacobs TimothyBJacobs TimothyBJacobs left review comments

@draganescu draganescu draganescu approved these changes

@noisysocks noisysocks Awaiting requested review from noisysocks

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

Assignees

@adamziel adamziel

Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
None yet
Milestone
Gutenberg 8.9
Development

Successfully merging this pull request may close these issues.
4 participants
@adamziel @draganescu @noisysocks @TimothyBJacobs