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: Reimplement lightbox trigger as a minimal button in corner of image #55212

Merged
merged 20 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2956be6
Reimplement lightbox trigger as a minimal button in corner of image
artemiomorales Oct 10, 2023
5a084d4
Remove obsolete directives
artemiomorales Oct 10, 2023
c264096
Update directives to fire logic properly via image or button click
artemiomorales Oct 10, 2023
6302aea
Ensure lightbox button only appears when hovering over image, not who…
artemiomorales Oct 10, 2023
de70bcb
Ensure close button does not receive focus when opening lightbox via …
artemiomorales Oct 10, 2023
25a91bc
Ensure button only receives focus when lightbox is closed via keyboard
artemiomorales Oct 10, 2023
994e04c
Add comments
artemiomorales Oct 10, 2023
0e86105
Prevent unnecessary focus being shown on mobile
artemiomorales Oct 11, 2023
d9357e5
Add dynamic positioning for button when image uses 'contain' setting
artemiomorales Oct 11, 2023
cea71f1
WORK IN PROGRESS - Begin accounting for various edge cases
artemiomorales Oct 11, 2023
e3c0d00
Simplify and improve button placement logic
artemiomorales Oct 16, 2023
6bae728
Simplify logic to show button on hover
artemiomorales Oct 16, 2023
c7bfa37
Fix styles
artemiomorales Oct 16, 2023
bd9d8b8
Simplify calls to showLightbox
artemiomorales Oct 16, 2023
cd54522
Fix style inconsistency between browsers
artemiomorales Oct 16, 2023
09b2f7b
Change button position slightly
artemiomorales Oct 16, 2023
94d8c3a
Reduce button offset
artemiomorales Oct 16, 2023
dce84bc
Add style override for better consistency across themes
artemiomorales Oct 16, 2023
c1e79f9
Fix logic so lightbox animates as intended; remove extraneous code
artemiomorales Oct 17, 2023
ef8f9e2
Update comment
artemiomorales Oct 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions packages/block-library/src/image/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ function block_core_image_render_lightbox( $block_content, $block ) {
$w->set_attribute( 'data-wp-init', 'effects.core.image.setCurrentSrc' );
$w->set_attribute( 'data-wp-on--load', 'actions.core.image.handleLoad' );
$w->set_attribute( 'data-wp-effect', 'effects.core.image.setButtonStyles' );
// We need to set an event callback on the `img` specifically
// because the `figure` element can also contain a caption, and
// we don't want to trigger the lightbox when the caption is clicked.
$w->set_attribute( 'data-wp-on--click', 'actions.core.image.callShowLightboxFromImage' );
$w->set_attribute( 'data-wp-on--mouseover', 'actions.core.image.handleMouseOver' );
$w->set_attribute( 'data-wp-on--mouseout', 'actions.core.image.handleMouseOut' );
$w->set_attribute( 'data-wp-effect--setStylesOnResize', 'effects.core.image.setStylesOnResize' );
$body_content = $w->get_updated_html();

Expand All @@ -213,12 +219,18 @@ function block_core_image_render_lightbox( $block_content, $block ) {
type="button"
aria-haspopup="dialog"
aria-label="' . esc_attr( $aria_label ) . '"
data-wp-on--click="actions.core.image.showLightbox"
data-wp-style--width="context.core.image.imageButtonWidth"
data-wp-style--height="context.core.image.imageButtonHeight"
data-wp-style--left="context.core.image.imageButtonLeft"
data-wp-on--click="actions.core.image.callShowLightboxFromButton"
data-wp-class--show="context.core.image.isHovering"
data-wp-style--right="context.core.image.imageButtonRight"
data-wp-style--top="context.core.image.imageButtonTop"
></button>';
>
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none">
<path d="M9 5H5V9" stroke="#FFFFFF" stroke-width="1.5"/>
<path d="M15 19L19 19L19 15" stroke="#FFFFFF" stroke-width="1.5"/>
<path d="M15 5H19V9" stroke="#FFFFFF" stroke-width="1.5"/>
<path d="M9 19L5 19L5 15" stroke="#FFFFFF" stroke-width="1.5"/>
</svg>
</button>';

$body_content = preg_replace( '/<img[^>]+>/', $button, $body_content );

Expand Down
31 changes: 27 additions & 4 deletions packages/block-library/src/image/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -157,25 +157,48 @@
display: flex;
flex-direction: column;

img {
cursor: zoom-in;
}

button {
opacity: 0;
border: none;
background: none;
background: #1e1e1e;
artemiomorales marked this conversation as resolved.
Show resolved Hide resolved
cursor: zoom-in;
width: 100%;
height: 100%;
width: 25px;
artemiomorales marked this conversation as resolved.
Show resolved Hide resolved
height: 25px;
position: absolute;
z-index: 100;
top: 5px;
right: 5px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure about conventions, but should this be left for rtl languages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe! That sounds like a good candidate for a follow up PR. I've added this point to the next version in the lightbox tracking issue.

text-align: center;
padding: 0;
border-radius: 10%;

&.show {
opacity: 1;
}

&:focus-visible {
outline: 5px auto #212121;
outline: 5px auto -webkit-focus-ring-color;
outline-offset: 5px;
}

&:hover {
cursor: pointer;
opacity: 1;
}

&:focus {
opacity: 1;
}

&:hover,
&:focus,
&:not(:hover):not(:active):not(.has-background) {
background: none;
background: #1e1e1e;
border: none;
}
}
Expand Down
176 changes: 125 additions & 51 deletions packages/block-library/src/image/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ store(
actions: {
core: {
image: {
showLightbox: ( { context, event } ) => {
showLightbox: ( { context, event }, image ) => {
// We can't initialize the lightbox until the reference
// image is loaded, otherwise the UX is broken.
if ( ! context.core.image.imageLoaded ) {
Expand All @@ -103,12 +103,10 @@ store(
context.core.image.lastFocusedElement =
window.document.activeElement;
context.core.image.scrollDelta = 0;
context.core.image.pointerType = event.pointerType;

context.core.image.lightboxEnabled = true;
setStyles(
context,
event.target.previousElementSibling
);
setStyles( context, image );

context.core.image.scrollTopReset =
window.pageYOffset ||
Expand Down Expand Up @@ -137,7 +135,42 @@ store(
false
);
},
hideLightbox: async ( { context } ) => {
// When opening the lightbox via clicking an image,
// we can use the event target directly and pass it to the
// showLightbox action, which uses it to create the styles.
callShowLightboxFromImage: ( {
context,
event,
actions,
} ) => {
actions.core.image.showLightbox(
{
context,
event,
},
event.target
);
},
// When opening the lightbox via clicking the button,
// we need to reach into event target's parent element to
// get the image element needed to create the styles.
callShowLightboxFromButton: ( {
context,
event,
actions,
} ) => {
actions.core.image.showLightbox(
{
context,
event,
},
// The event target we receive when clicking the button
// is the SVG element inside of it, so we need to go to
// the parent element's sibling to get the image.
event.target.parentElement.previousElementSibling
);
},
artemiomorales marked this conversation as resolved.
Show resolved Hide resolved
hideLightbox: async ( { context, event } ) => {
context.core.image.hideAnimationEnabled = true;
if ( context.core.image.lightboxEnabled ) {
// We want to wait until the close animation is completed
Expand All @@ -154,11 +187,27 @@ store(
}, 450 );

context.core.image.lightboxEnabled = false;
context.core.image.lastFocusedElement.focus( {
preventScroll: true,
} );

// We want to avoid drawing attention to the button
// after the lightbox closes for mouse and touch users.
// Note that the `event.pointerType` property returns
// as an empty string if a keyboard fired the event.
if ( event.pointerType === '' ) {
context.core.image.lastFocusedElement.focus( {
preventScroll: true,
} );
}
Comment on lines +156 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with the close button: if we navigate to a lightbox with the keyboard, but I close it using a click, I lose the focus, which I assume is not correct, right?

}
},
// We need to use a handler to know whether the mouse is hovering
// so we know when to show the lightbox trigger button. We are unable
// to use just CSS for this because the button is not a child of the image.
artemiomorales marked this conversation as resolved.
Show resolved Hide resolved
handleMouseOver( { context } ) {
context.core.image.isHovering = true;
},
handleMouseOut( { context } ) {
context.core.image.isHovering = false;
},
handleKeydown: ( { context, actions, event } ) => {
if ( context.core.image.lightboxEnabled ) {
if ( event.key === 'Tab' || event.keyCode === 9 ) {
Expand Down Expand Up @@ -191,14 +240,9 @@ store(
}
}
},
handleLoad: ( { state, context, effects, ref } ) => {
handleLoad: ( { context, ref } ) => {
context.core.image.imageLoaded = true;
context.core.image.imageCurrentSrc = ref.currentSrc;
effects.core.image.setButtonStyles( {
state,
context,
ref,
} );
},
handleTouchStart: () => {
isTouching = true;
Expand Down Expand Up @@ -279,10 +323,17 @@ store(
focusableElements.length - 1
];

ref.querySelector( '.close-button' ).focus();
// We want to avoid drawing unnecessary attention to the close
// button for mouse and touch users. Note that even if opening
// the lightbox via keyboard, the event fired is of type
// `pointerEvent`, so we need to rely on the `event.pointerType`
// property, which returns an empty string for keyboard events.
if ( context.core.image.pointerType === '' ) {
ref.querySelector( '.close-button' ).focus();
}
Comment on lines +285 to +292
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we want to focus the close button when opening the button by clicking the image? Right now, it keeps the active element outside of the modal, which I believe is not correct, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a point brought up some folks from design a while back — they did not want a close button at all because they felt it would distract and be unnecessary.

I figured adding this so the focus ring doesn't become visible and draw attention to the close button would be a good move. Personally I would prefer to not have my attention immediately drawn to a close button when trying to get a better view of an image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are potential accessibility issues here, though. The context would indeed be incorrect for folks who activate the lightbox using a mouse while also using a screen reader.

Is this a use case we should handle? I'm currently searching Google for guidance around this, and will also ping @afercia @joedolson @alexstine for feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that with that approach we are losing the focus and it is causing some issues. When opening an image by clicking on it:

  1. I can't close it with the ESC key.
  2. I can't close it navigating with the keyboard because I can't focus on the close button.

}
},
setButtonStyles: ( { state, context, ref } ) => {
setButtonStyles: ( { context, ref } ) => {
const {
naturalWidth,
naturalHeight,
Expand All @@ -291,54 +342,77 @@ store(
} = ref;

// If the image isn't loaded yet, we can't
// calculate how big the button should be.
// calculate where the button should be.
if ( naturalWidth === 0 || naturalHeight === 0 ) {
return;
}

// Subscribe to the window dimensions so we can
// recalculate the styles if the window is resized.
if (
( state.core.image.windowWidth ||
state.core.image.windowHeight ) &&
context.core.image.scaleAttr === 'contain'
) {
// In the case of an image with object-fit: contain, the
// size of the img element can be larger than the image itself,
// so we need to calculate the size of the button to match.
const figure = ref.parentElement;
const figureComputedStyle =
window.getComputedStyle( figure );

const figureWidth =
ref.parentElement.offsetWidth -
parseFloat( figureComputedStyle.marginLeft ) -
parseFloat( figureComputedStyle.marginRight );

// We need special handling for the height because
// a caption will cause the figure to be taller than
// the image, which means we need to account for that
// when calculating the placement of the button in the
// top right corner of the image.
let figureHeight = ref.parentElement.offsetHeight;
const caption = figure.querySelector( 'figcaption' );
if ( caption ) {
figureHeight =
figureHeight -
parseFloat( figureComputedStyle.marginTop ) -
parseFloat( figureComputedStyle.marginBottom ) -
caption.offsetHeight;
}

const buttonOffsetTop = figureHeight - offsetHeight;
const buttonOffsetRight = figureWidth - offsetWidth;

// In the case of an image with object-fit: contain, the
// size of the <img> element can be larger than the image itself,
// so we need to calculate where to place the button.
if ( context.core.image.scaleAttr === 'contain' ) {
// Natural ratio of the image.
const naturalRatio = naturalWidth / naturalHeight;
// Offset ratio of the image.
const offsetRatio = offsetWidth / offsetHeight;

if ( naturalRatio > offsetRatio ) {
// If it reaches the width first, keep
// the width and recalculate the height.
context.core.image.imageButtonWidth =
offsetWidth;
const buttonHeight = offsetWidth / naturalRatio;
context.core.image.imageButtonHeight =
buttonHeight;
if ( naturalRatio >= offsetRatio ) {
// If it reaches the width first, use a fixed
// position for the X axis and calculate Y position.
context.core.image.imageButtonRight =
buttonOffsetRight + 25;
const imageHeight = offsetWidth / naturalRatio;
context.core.image.imageButtonTop =
( offsetHeight - buttonHeight ) / 2;
( offsetHeight - imageHeight ) / 2 +
buttonOffsetTop +
25;
} else {
// If it reaches the height first, keep
// the height and recalculate the width.
context.core.image.imageButtonHeight =
offsetHeight;
const buttonWidth = offsetHeight * naturalRatio;
context.core.image.imageButtonWidth =
buttonWidth;
context.core.image.imageButtonLeft =
( offsetWidth - buttonWidth ) / 2;
// If it reaches the height first, use a fixed
// position for the Y axis and calculate X position.
context.core.image.imageButtonTop =
buttonOffsetTop + 25;
const imageWidth = offsetHeight * naturalRatio;
context.core.image.imageButtonRight =
( offsetWidth - imageWidth ) / 2 +
buttonOffsetRight +
25;
}
} else {
// In all other cases, we can trust that the size of
// the image is the right size for the button as well.

context.core.image.imageButtonWidth = offsetWidth;
context.core.image.imageButtonHeight = offsetHeight;
// In other cases, we can just put the button in
// the top right corner of the containing element,
// accounting for the fact that users may have resized
// the image, which will require us to reposition
// the button on the X axis but not the Y axis.
context.core.image.imageButtonTop = 25;
context.core.image.imageButtonRight =
buttonOffsetRight + 25;
}
},
setStylesOnResize: ( { state, context, ref } ) => {
Expand Down
Loading