Added a fix for the lightbox#762
Conversation
|
@CrochetFeve0251 Thank you for the PR. The issue has been fixed but there is a console error that's not there with Uncaught TypeError: Cannot read properties of null (reading 'focus')
This can be seen here:
Can you please check this? 🙏 |
|
@remyperona Thanks for the update. |
|
It's not expected, but this WP feature is also not planned to work with picture element I think. I would say it's an acceptable side effect compared to the current issue. |
|
Items for exploratory
|
nicomollet
left a comment
nicomollet
left a comment
There was a problem hiding this comment.
It looks good but needs proper QA
There was a problem hiding this comment.
Pull request overview
This pull request fixes a lightbox issue by preventing WordPress-specific data-wp attributes from being passed to the <picture> tag element. The fix filters out attributes with keys containing 'data-wp' before building the picture tag HTML, while still preserving these attributes on the inner <img> tag where they may be needed.
Key changes:
- Added filtering logic using
array_filterto removedata-wp*attributes from picture tag - Maintains backward compatibility by only affecting the picture tag, not the img tag
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $picture_attributes = array_filter( | ||
| $attributes, | ||
| function ( $attribute ) { | ||
| return strpos( $attribute, 'data-wp' ) === false; |
There was a problem hiding this comment.
The current check using strpos($attribute, 'data-wp') === false will filter out any attribute containing 'data-wp' anywhere in the key name, not just those starting with 'data-wp'. For more precise filtering of WordPress-specific attributes, consider using strpos($attribute, 'data-wp-') !== 0 or strncmp($attribute, 'data-wp-', 8) !== 0 to ensure only attributes that start with 'data-wp-' are filtered out. This prevents accidentally filtering attributes that may contain 'data-wp' in other positions.
| return strpos( $attribute, 'data-wp' ) === false; | |
| return strncmp( $attribute, 'data-wp-', 8 ) !== 0; |
| $picture_attributes = array_filter( | ||
| $attributes, | ||
| function ( $attribute ) { | ||
| return strpos( $attribute, 'data-wp' ) === false; | ||
| }, | ||
| ARRAY_FILTER_USE_KEY | ||
| ); |
There was a problem hiding this comment.
Consider adding an inline comment above the array_filter call to explain why data-wp attributes are being filtered out from the picture tag. This would help future maintainers understand the purpose of this filtering, especially in relation to the lightbox fix mentioned in issue #758.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
🟢 Coverage 100.00% diff coverage
Metric Results Coverage variation Report missing for 3fb41021 Diff coverage ✅ 100.00% diff coverage (50.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (3fb4102) Report Missing Report Missing Report Missing Head commit (4ea5bf0) 19540 663 3.39% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#762) 8 8 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Covers the lightbox fix (#758): data-wp-* directive attributes must be excluded from the <picture> tag while remaining on the inner <img> tag, without breaking the existing Gutenberg cover-background exclusion logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Description
Added a fix for the lightbox by not passing the wp attributes to the picture tag.
Fixes #758
Type of change
Please delete options that are not relevant.
Is the solution different from the one proposed during the grooming?
No.
Checklists
Generic development checklist
Test summary
If not, detail what you could not test.
Please describe any additional tests you performed.