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

Image: Update default fullscreen icon for lightbox trigger #55463

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

artemiomorales
Copy link
Contributor

@artemiomorales artemiomorales commented Oct 18, 2023

What?

This PR updates the lightbox trigger button to use the latest version of the full screen icon.

Why?

Addresses #55462
We want to have consistent UI throughout Gutenberg.

How?

Testing Instructions

  1. Insert an image into a post
  2. Make sure 'Expand on click' is enabled in the image block settings
  3. Publish and view the post
  4. Hover over the image — see that the icon resembles the one used in Image: Update lightbox full screen icon #55462

Testing Instructions for Keyboard

When viewing the post, make sure tab works and that the full screen button appears when it receives focus.

@artemiomorales artemiomorales added [Type] Bug An existing feature does not function as intended [Block] Image Affects the Image Block Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 18, 2023
@artemiomorales artemiomorales linked an issue Oct 18, 2023 that may be closed by this pull request
@github-actions
Copy link

Flaky tests detected in 174bfe9.
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/6565670505
📝 Reported issues:

Copy link
Contributor

@SantosGuillamot SantosGuillamot 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 tested it and it seems to be working fine 👍

Copy link
Contributor

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@artemiomorales artemiomorales merged commit 6b498f2 into trunk Oct 19, 2023
56 checks passed
@artemiomorales artemiomorales deleted the fix/lightbox-fullscreen-button branch October 19, 2023 17:51
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 19, 2023
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 19, 2023

Cherry-picked for inclusion in RC2 here: f469741

@SiobhyB SiobhyB removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 19, 2023
SiobhyB pushed a commit that referenced this pull request Oct 23, 2023
* Focus submenu button when clicked (#55198)

* Focus element manually when open submenu on click

* Try using `tabindex="-1"`

* Use `tabindex="-1"` also in body when a submenu is opened

* Replace tabindex with event listener

* Explain the tabindex on <li>

* Don't store the element on hover to restore the focus later

* Improve explanations

* Add tests to cover webkit frontend menu interactions

Safari doesn't place focus on a clicked button as expected. These tests verify that when a submenu chevron button is clicked, focus is correctly placed on that button. It also verifies that the click on the body correctly closes any open submenus, which was failing in Safari.

* Focus clicked button on Safari

Combining the tabindex -1 on the parent li and focusing the button on Safari, and also checking that the relatedTarget is null inside the handleMenuFocusout seems to contain the focus within the menu to not fire the handleMenuFocusout as often, and still works to click on the body to close the menu.

* Added the document.addEventListener body click back in

Authored by Luis Herranz <luisherranz@gmail.com>. I'm just re-applying the change.

* Remove tab keypresses from webkit menu interaction tests

Tab keypreses on webkit playwright are really flakey (or it's something in our code that we haven't isolated) so I've split out the webkit tests to test everything I can without using a tab keypress.

* Use body click instead for consistency across environments

---------

Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

* Make layout support compatible with enhanced pagination (#55416)

* Make layout supports compatible with enhanced pagination

* Use sanitize_title and add `layout` to the class name

* Update default fullscreen icon for lightbox trigger (#55463)

* Make duotone compatible with enhanced pagination (#55415)

* Patterns: fix capabilities settings for pattern categories (#55379)

Co-authored-by: Daniel Richards <daniel.richards@automattic.com>

* Revert "Patterns: fix capabilities settings for pattern categories (#55379)"

This reverts commit 6f83c92.

* Image block: wrap images with hrefs in an A tag (#55470)

* This commit sees what happens when we wrap the image element in an A tag in the editor.
This is to ensure any inherited styles from the link element, such as border color, apply to the image.

* Removing duplicate <a href /> wrapper
Adding disabled onClick and aria attribute

* Image: Improve focus management in lightbox (#55428)

* Improve focus management

This commit removes the logic to set focus differently
based on event.pointerType and instead sets focus on the
dialog itself when the lightbox opens, and on the lightbox
trigger when the lightbox closes.

* Add delay before focusing when closing lightbox

* Put focus back on close button when opening lightbox

It turns out that placing focus on the modal was causing
inconsistent behavior in Safari, so I've put the focus back
on the close button when the lightbox opens, which
performs predictably.

I've also added a tabindex to the image, which prevents
the focus ring from erroneously showing when opening the lightbox
with a mouse in Chrome and Firefox.

* Move focus to the dialog when opening the lightbox.

* Fix SVG markup.

* Consistent indentation with spaces.

* Remove unnecessary tabindex

---------

Co-authored-by: Andrea Fercia <a.fercia@gmail.com>

* Fix: - Update the title when using enhanced pagination

---------

Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com>
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Artemio Morales <artemio.morales@a8c.com>
Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image: Update lightbox full screen icon
4 participants