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: Directive processor failing on updated HTML API #55404

Merged
merged 1 commit into from Oct 17, 2023

Conversation

dmsnell
Copy link
Contributor

@dmsnell dmsnell commented Oct 17, 2023

Discovered after backporting the WordPress 6.4 release into Gutenberg.
https://wordpress.slack.com/archives/C02QB2JS7/p1697471144510409

Identified broken with the merge of WordPress/wordpress-develop#5475, which hadn't yet been backported into Gutenberg, and which wouldn't have been noticed because of the Directive Processor using WP_HTML_Tag_Processor() directly.

@github-actions
Copy link

github-actions bot commented Oct 17, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/interactivity-api/class-wp-directive-processor.php
❔ lib/load.php

@dmsnell dmsnell force-pushed the fix/directive-processor-html-api branch from 55c9c6a to 0fb331a Compare October 17, 2023 05:43
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Thanks for helping to track this down!

I've tested against the latest WordPress version (WordPress 6.4-beta4-20231017.160904 ) and, with this PR's changes the WP_Directive_Processor unit tests pass again.

@ramonjd ramonjd marked this pull request as ready for review October 17, 2023 05:48
@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Feature] HTML API An API for updating HTML attributes in markup labels Oct 17, 2023
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing nicely for me, too! Thanks again for the quick fix @dmsnell

@ntsekouras
Copy link
Contributor

Thank you for handling this!

@github-actions
Copy link

github-actions bot commented Oct 17, 2023

Flaky tests detected in e562e192171a159fbc299a23d219fa87247ef521.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6543288911
📝 Reported issues:

@dmsnell dmsnell force-pushed the fix/directive-processor-html-api branch from 8202011 to e562e19 Compare October 17, 2023 06:17
Temporary fix to resolve breaking tests in `trunk` after the WordPress 6.4 backport.
@dmsnell dmsnell force-pushed the fix/directive-processor-html-api branch from e562e19 to 7661b21 Compare October 17, 2023 07:06
@ramonjd ramonjd merged commit 7cbf38f into trunk Oct 17, 2023
49 checks passed
@ramonjd ramonjd deleted the fix/directive-processor-html-api branch October 17, 2023 08:22
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 17, 2023
@SiobhyB SiobhyB added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 17, 2023
SiobhyB pushed a commit that referenced this pull request Oct 17, 2023
…5404)

Temporary fix to resolve breaking tests in `trunk` after the WordPress 6.4 backport.
@SiobhyB SiobhyB removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 17, 2023
SiobhyB added a commit that referenced this pull request Oct 17, 2023
SiobhyB added a commit that referenced this pull request Oct 17, 2023
* Unconditionally load the Gutenberg copy of the HTML Tag Processor (#55404)

Temporary fix to resolve breaking tests in `trunk` after the WordPress 6.4 backport.

* Revert "Unconditionally load the Gutenberg copy of the HTML Tag Processor (#55404)"

This reverts commit 271587a.

* Image: Reimplement lightbox trigger as a minimal button in corner of image (#55212)

* Reimplement lightbox trigger as a minimal button in corner of image

* Remove obsolete directives

* Update directives to fire logic properly via image or button click

* Ensure lightbox button only appears when hovering over image, not whole figure

* Ensure close button does not receive focus when opening lightbox via mouse

* Ensure button only receives focus when lightbox is closed via keyboard

* Add comments

* Prevent unnecessary focus being shown on mobile

* Add dynamic positioning for button when image uses 'contain' setting

* WORK IN PROGRESS - Begin accounting for various edge cases

We need to account for the fact that an image can have
custom dimensions, aspect ratio, cover or contain, captions,
thumbnail dimensions, and potentially other scenarios.

This commit begins to address those issues but notably fails
in cases where one uses a horizontal image, at full scale,
with custom aspect ratio, using 'contain'.

It seems to work in all other cases that I've checked but
needs more thorough testing and the code can probably be cleaner,
and may contain some unnecessary items.

* Simplify and improve button placement logic

* Simplify logic to show button on hover

* Fix styles

* Simplify calls to showLightbox

* Fix style inconsistency between browsers

* Change button position slightly

* Reduce button offset

* Add style override for better consistency across themes

* Fix logic so lightbox animates as intended; remove extraneous code

* Update comment

* [Edit Widgets] Only suppress admin notices when JS enabled. (#55403)

Only suppresses the display of notices on the widget block editor screen if JavaScript is available.

---------

Co-authored-by: Dennis Snell <dennis.snell@automattic.com>
Co-authored-by: Artemio Morales <artemio.morales@a8c.com>
Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
@dmsnell
Copy link
Contributor Author

dmsnell commented Oct 31, 2023

To follow up, I think this "workaround" is the more permanent fix on the Gutenberg side. Updates and other backports in #55703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] HTML API An API for updating HTML attributes in markup [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants