Fix tab/focus order issue in Media & text block #40806
Conversation
|
Size Change: +10.1 kB (+1%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
Help wanted:
Still in progress: |
carolinan
added
[Type] Bug
aristath
force-pushed
the
update/media-text-markup
branch
2 times, most recently
from
34ae243 to
9da71ee
Compare
There was a problem hiding this comment.
This is a great starting point!
Some things that need fixing:
- Since the markup changes, we will need to tweak the CSS too:
- remove
or make the value
1fr 1fr - remove
gutenberg/packages/block-library/src/media-text/style.scss
Lines 10 to 12 in f2f7ea0
- remove
gutenberg/packages/block-library/src/media-text/style.scss
Lines 54 to 66 in f2f7ea0
- remove
gutenberg/packages/block-library/src/media-text/style.native.scss
Lines 9 to 11 in f2f7ea0
- remove
gutenberg/packages/block-library/src/media-text/style.native.scss
Lines 15 to 18 in f2f7ea0
- remove
or make the value
- In
edit.native.jswe should also probably remove the extra styles formediaPosition - We should look at all the native files and make sure we make changes where needed there as well
- After making the above changes, is the
has-media-on-the-rightclass even needed? Should we remove it? 🤔 - I'm getting deprecation errors in the console, but I was unable to figure them out after spending a few hours here. I think we'll need some assistance on this one... cc @jorgefilipecosta, I see you've done some work on the deprecation calls in some blocks - including this one. Do you have any insights for this one? 🙏
|
137 themes https://wpdirectory.net/search/01G2VJ8W8V3ETYE34T8Q9B51DJ and 25 plugins https://wpdirectory.net/search/01G2VJY1GKY7K1H9S3H9DCHT1W uses has-media-on-the-right but these results are not limited to css. |
|
If this can't be solved with ease, I think a better alternative is to deprecate the block and replace it with block patterns The current block is more limited than using separate blocks to create the same design. |
There was a problem hiding this comment.
Thank you @carolinan, @aristath for your work on this.
After making the above changes, is the has-media-on-the-right class even needed? Should we remove it? 🤔
We need to keep the class, when a website updates the old blocks will still output the class on the frontend they will only be migrated if opened again on the editor.
This seems like a case where a migration should be possible and we don't need to deprecate, I will look into the issues we are having with the migration and try to fix them.
jorgefilipecosta
force-pushed
the
update/media-text-markup
branch
from
9da71ee to
96b8804
Compare
|
I applied a fix to the deprecations it seems to work well, I tested it with several different blocks created on the trunk (with the old markup) and everything still looks as expected on the frontend on this branch and also load well on the editor without any issue. Let me know if there is any issue pending. |
|
Thank you @jorgefilipecosta. Besides the native changes and the tests, there is the remaining issue of the "stack on mobile" setting. This feature still uses the grid columns, and the focus order is incorrect. What options do we have? I feel that accessibility should always be the strongest case.
I'm not mentioning JS solutions because I don't think we should add another script to the front. @bgardner can you think of anything more? A question for @ryelle: |
carolinan
changed the title
|
@carolinan, I'm seeing the following error when using the 2020 theme.
|
This seems to introduced a deprecation issue, yes. I saw it now with 2022 as well. |
|
Those errors are from patterns in the pattern directory. I tried asking @ryelle about them earlier. |
|
@carolinan, you're right; the errors are coming from the patterns. It also might mean that deprecation introduced in this PR is failing, so we need to double-check that. |
|
Still unable to reproduce this with local media & text blocks. |
|
I just tested with Frost, where we use Media & Text blocks in patterns. I am getting the same errors that @Mamaduka got, but they are related to patterns from the directory. The patterns in Frost are all working correctly without errors. This leads me to believe that the patterns in the directory just need updating.
The two culprit patterns are the following:
|
|
If a pattern had some markup that worked before and now it doesn't, it means we missed some deprecation logic. It doesn't matter where this comes from and it seems we were lucky to find out through the patterns directory patterns 😄 . What we need to do is get the failing markup from the patterns and fix the deprecation logic. We should probably test first if it worked before this PR though. |
|
I just tested in 6.0.1 and Gutenberg 13.7.2 with Twenty Twenty-Two, and I am not seeing the errors. |
|
With Gutenberg trunk active, I copy pasted this pattern into the block editor https://wordpress.org/patterns/pattern/event/. The order of the colors in the style tag is different in the body and save function. Body: The content div is empty: Save: The content div has spacing: |
|
When the color is removed from the markup there is no block validation error. |
| </div> | ||
| <!-- /wp:media-text --> | ||
| <figure class="wp-block-media-text__media"> |
There was a problem hiding this comment.
This is the issue with deprecations. Did you manually update this file? Because normally we keep the deprecated .html file intact and the one that will be updated to the new markup is the generated serialized.html..
There was a problem hiding this comment.
I don't recall anymore. I think I just ran the fixture script.
There was a problem hiding this comment.
That's fine, but everything is produced from the html files. I'll open a PR for the fix.
What?
Changes the HTML markup of the block depending on the position of the media.
Closes #38537
Why?
When elements are repositioned using only CSS, the visual order does not always match the source order, and that is the case when we move the media in the media & text block from the left to right position. When elements are linked, and you focus on the links, the focus moves unexpectedly.
How?
The
figureelement is placed before or after the content, depending on themediaPositionblock attribute.Testing Instructions
Screenshots or screencast