Improve gutenberg compatibility for tooltips#748
Improve gutenberg compatibility for tooltips#748stokesman wants to merge 15 commits intosybrew:masterfrom
Conversation
stokesman
left a comment
stokesman
left a comment
There was a problem hiding this comment.
Some hopefully helpful review notes.
| 'id' => 'tsf-gbc', | ||
| 'type' => 'js', | ||
| 'deps' => [ 'jquery', 'tsf', 'tsf-utils', 'wp-editor', 'wp-data', 'react' ], | ||
| 'deps' => [ 'jquery', 'tsf', 'tsf-utils', 'wp-editor', 'wp-data', 'react', 'wp-element', 'wp-components' ], |
There was a problem hiding this comment.
Tangential note: I don’t think 'react' was needed in the deps. It’s certainly not now that 'wp-element' is in there.
There was a problem hiding this comment.
I've used nesting in the CSS, which I didn’t see elsewhere. Here, I mostly like it for visually conveying hierarchy and it’s by no means necessary. It’s supported in all evergreen browsers since 2023.
There was a problem hiding this comment.
Let's defer nesting until a major release. I'm in favor, but mixed coding style changes can be confusing to maintain.
Also, so much extra CSS to accommodate a specific page type for the tooltip, which otherwise works universally, seems like a workaround for a bug—are you certain this isn't an upstream issue?
There was a problem hiding this comment.
The tooltip colors are currently handled via The_SEO_Framework\Admin\Script\Registry::convert_color_css_declaration(), where we also calculate relative font colors. Working with var(--wp...) indeed is a solid workaround as per the issue you mentioned here, but what you're ultimately resolving is an upstream bug, where #999 should be #888.
I only apply patches when there's literally no feasible fix/workaround upstream.
There was a problem hiding this comment.
Oh, I hadn’t used var(--wp…) to patch the contrast issue, but to avoid having to add to the inline CSS for the GB tooltip arrow—although that would be a small addition. It also seemed like it might be okay to have it match the used accent color in GB which is different from classic (and seemingly with intention).
Now, as a way to keep colors as before without having to add to the inline CSS, I've pushed c8cacba, that proposes to use CSS variables. No sweat if that’s off the table for any reason—it seemed worth a try.
I also put in a brief effort to understand why the GB admin accent color isn’t the same as what TSF pulls from the global. I didn’t find out but I did find that contrast issue is seemingly not considered one upstream:
“Historically, only the default color scheme is meant to be accessible” WordPress/gutenberg#6438 (review)
There was a problem hiding this comment.
Also, so much extra CSS to accommodate a specific page type for the tooltip, which otherwise works universally, seems like a workaround for a bug—are you certain this isn't an upstream issue?
While it might be considered an upstream issue I doubt it would be considered a bug. If their Popover component’s API had a few more features this could likely be reduced. There’s another approach that could be tried but I can’t say if it’d reduce this by much. It would also require :has() which I still hesitate to use.
| box-sizing: border-box; | ||
| display: flex; | ||
| flex: 1 1 auto; | ||
| /* flex: 1 1 auto;*/ /* Is this ever in a flex-container? */ |
There was a problem hiding this comment.
I kept noticing the dev tools telling me this has no effect because the parent element is not a flex container. Perhaps it’s there for a known case but if not I just hoped to remove it and tidy up.
There was a problem hiding this comment.
It was probably accommodating an old version of the Extension Manager or the SEO Bar pre-TSF v5.0. I'll investigate. Or, most likely, a bad habit of mine where I applied flex normalization everywhere to work around old Safari and Firefox bugs.
| imageObject.style = "max-width:100%;max-height:100%;object-fit:contain;aspect-ratio:1.91;height:auto;border-radius:3px;display:block;"; | ||
| // Add dimensions to prevent jumping. | ||
| imageObject.width = imageObject.naturalWidth; | ||
| imageObject.height = imageObject.naturalHeight; |
There was a problem hiding this comment.
Adding the width and height attributes seems to be what fixes the tooltip placement even outside of gutenberg. I’d be glad to extract those to a separate PR if you’d like (or in case this PR isn’t favored). The style attribute changes are for a few reasons:
max-width/max-heightneed not be specific pixel values- It seems nice to use the recommended aspect ratio and
object-fitto prevent distorting the images as can happen currently.
Looking at this again maybe the min-width/min-height would still be good to keep. I just thought if they were primarily to "prevent jumping" then the dimension attributes made them extraneous.
There was a problem hiding this comment.
To consider if min-width/min-height is still necessary, we need to test changing images while keeping the tooltip rendered.
i.e., Hover the cursor over the image tooltip, then type/paste a new Social Image URL (the example is incorrect since we get an error, but hopefully illustrates the point).
...why isn't the GIF looping?
There was a problem hiding this comment.
It’s looking stable in my testing:
Gutenberg:
social-image-jump-test.mp4
Category edit:
social-image-category-jump-test.mp4
| // Yes, I'm too lazy to copy/paste whatever's above again to prepend a dot, so I spent half an hour figuring this. | ||
| const ttSelectors = Object.fromEntries( Object.entries( ttNames ).map( ( [ i, v ] ) => [ i, `.${v}` ] ) ); | ||
|
|
||
| const ttMap = new Map(); |
There was a problem hiding this comment.
This maps tooltip items to tooltips. It’s added to make getTooltip ignorant of where to find tooltips because querying within the item element won’t work for the body-level rendered tooltips.
| get: ( element ) => { | ||
| if ( ! _activeTooltipElements.tooltip ) { | ||
| const tooltip = getTooltip( element ); | ||
| if ( ! tooltip ) return {}; | ||
|
|
||
| Object.assign( _activeTooltipElements, { | ||
| tooltip, | ||
| arrow: tooltip.querySelector( ttSelectors.arrow ), | ||
| wrap: element.closest( ttSelectors.wrap ) || element.parentNode, | ||
| } ); | ||
| } | ||
| return _activeTooltipElements; |
There was a problem hiding this comment.
This was first added because at first I thought finding the arrow elements in Gutenberg would need to be overridden. I found how to avoid that but I kept this around because I liked the accompanying changes within animate for being a little more concise. The only thing here that’s critical is not querying within element to find tooltip. Looking at this again, I suppose that can be optimized by using ttMap.get directly instead of getTooltip.
There was a problem hiding this comment.
If we go the Map route (which is super performant), then _activeTooltipElements and _pointer should be replaced with Maps entirely. Since Maps are pointers, we should reset them, just in case, inside the init callback of tsf-tooltip-reset. This is needed when we spawn new objects that can initiate tooltips, such as a new Term's SEO Bar, or Post/Term quick-edit fields.
In any case, this "get" function's Object assignment seems to patch an underlying issue. Should it not just default to "no tooltip" if the registry (Map) is empty, without trying to find and register one anyway?
There was a problem hiding this comment.
Should it not just default to "no tooltip" if the registry (Map) is empty, without trying to find and register one anyway?
In 9a3d561, I think I did what you suggest (just the quoted bit).
I'm not sure I get the idea with maps for _activeTooltipElements and _pointer. I actually don’t see the need for a map for the tooltips and have tested doing without but the map makes sense with being more congruent with what’s being done already.
| if ( boundaryRight + adjust > _activeTooltipElements.wrap.offsetWidth ) { | ||
| if ( boundaryRight + adjust > wrapRect.right ) { |
There was a problem hiding this comment.
This fixed an issue in Gutenberg tooltips that were near the right edge of the window. The arrow was not being allowed to go far enough to the right. The wrap rect was already being used for mousex so this actually saves a DOM read.
| * @return {Promise<Boolean>} True on success, false otherwise. | ||
| */ | ||
| function doTooltip( event, element, desc ) { | ||
| async function doTooltip( event, element, desc ) { |
There was a problem hiding this comment.
Just noting this was already being await’d from _activeTooltipElements.pointerEnter. Now it actually is async for Gutenberg (the _renderTooltip override is).
| * @param {!jQuery|Element|string} element | ||
| * @param {!jQuery|Element} element |
There was a problem hiding this comment.
I could see no handling of strings for element so I figured this is out of date.
There was a problem hiding this comment.
That's actually a bug inside the function from when we converted from jQuery to native JS. But, since this has been such a long-standing bug, let's turn it into a feature. So, yes, we should remove the string type mention.
| document.dispatchEvent( | ||
| new CustomEvent( 'tsf-gutenberg-tt', { detail: overrideTooltip } ) | ||
| ); |
There was a problem hiding this comment.
Using an event for this just seemed nice to not add publicly accessible members though of course it’s still public 🤷.
| > .tsf-tooltip-wrap { | ||
| /* In gutenberg, this ensures the wrap has the size of its contents, not sure why it’s | ||
| not required in the classic editor. */ | ||
| display: inline-block; |
There was a problem hiding this comment.
I cannot reproduce this error on Chromium. What browser are you using?
There was a problem hiding this comment.
Firefox, Safari and Chrome all show me the same thing. Probably, it’s just not clear what to look for. Here’s a recording that should clarify:
social-image-tt-wrap-size.mp4
Apparently the popover component in Gutenberg reads the wrap’s rect differently. So, of course, this isn’t required in regular admin screens - the comment could have been better. I suppose this style should move to gbc.css and maybe applied generically to all wraps if it doesn’t mess up any other wraps.
| ); | ||
| } | ||
|
|
||
| function GBCTooltip( { element, desc, setTooltip, tt } ) { |
There was a problem hiding this comment.
Same as https://github.com/sybrew/the-seo-framework/pull/748/files#r2549062861: Is this workaround necessary? Is this not a bug upstream? The tooltips work perfectly everywhere else. I'm also not in favor of creating new HTML wrappers, since tt.js is supposed to handle all wrapping autonomously.
There was a problem hiding this comment.
I think it might not be clear how much responsibility this wrapper has in GB. The tooltips work everywhere else because they can position themselves to their relative positioned wrap. Once rendered apart from their wraps the positioning task is trickier and this wrapper (react component) handles that. This includes keeping the position updated through scrolling or sizing changes.
It’s probably true that ideally the wp-components package could expose a (react) hook, that enables applying the popover positioning system (which is floating-ui) without the Popover component. Speaking of that, before I tried this PR, I tried a branch using floating-ui instead. It’s great in some ways but I doubted it would be favorable because it adds ~20kb.
Since the last time I worked on this I happened to remember that the web standard Popover related APIs are coming along nicely. That seems like the ideal thing to base tt.js on in the near future. I mention that in case it helps to think of this a temporary measure which can be ripped out later. That said, I’d understand if you want to look at doing a even smaller temporary measure.
| } | ||
|
|
||
| return true; | ||
| return tooltip; |
There was a problem hiding this comment.
We shouldn't need to manipulate the tooltip outside this function.
| ttMap.set( element, | ||
| // It’s async when Gutenberging. | ||
| await _renderTooltip( event, element, desc ) | ||
| ); |
There was a problem hiding this comment.
I think ttMap.set() should be moved inside _renderTooltip, so we can keep this function synchronous. This, however, ignores your comment above regarding Gutenberg's patch.
There was a problem hiding this comment.
I’m confused on this. The jsdoc says it’s asynchronous (since 4.2.0) and returns a promise. I guess that’s not accurate but the only place it’s invoked within the plugin it’s awaited as if already async:
the-seo-framework/lib/js/tt.js
Line 71 in ff95bd8
Anyway, I did give it shot and it can work synchronously. Apart from whether sync or async, I only dislike how it requires reproducing one more bit of logic (ttMap.set()) in the gbc _renderTooltip override. There’s already a little duplication there that might ideally be obviated but I’d not stressed it since this is speculative. I didn’t push the changes but the patch follows.
Patch to move ttMap.set() inside _renderTooltip and make Gutenberg _renderTooltip override work synchronously. Also removes seemingly unneeded await and async to beg the question.
diff --git a/lib/js/gbc.js b/lib/js/gbc.js
index 2f2c06ac..2e2ab3ef 100644
--- a/lib/js/gbc.js
+++ b/lib/js/gbc.js
@@ -346,21 +346,21 @@ window.tsfGBC = function( $ ) {
);
base.ttSelectors.arrow = '.components-popover__arrow';
return {
- _renderTooltip: async ( _event, element, desc ) => {
+ _renderTooltip: ( _event, element, desc ) => {
element.dataset.hasTooltip = 1;
- const tooltipPromise = new Promise( ( resolve ) => {
- const setTooltip = ( tooltip ) => resolve( tooltip );
+ const setTooltip = tooltip => base.ttMap.set( element, tooltip );
+ wp.element.flushSync( () =>
root.render(
wp.element.createElement(
GBCTooltip,
{ element, desc, setTooltip, tt: base }
)
- );
- } );
- return await tooltipPromise;
+ )
+ );
+ return true;
},
removeTooltip: ( element ) => {
- root.render( null );
+ root.render( null ); // Should this also be wrapped in flushSync? — it tests okay as is…
base.removeTooltip( element );
}
};
diff --git a/lib/js/tt.js b/lib/js/tt.js
index 36460ace..6a9b302b 100644
--- a/lib/js/tt.js
+++ b/lib/js/tt.js
@@ -62,7 +62,7 @@ window.tsfTT = function () {
event.target.dispatchEvent( new Event( 'mousemove' ) ); // event time: <.3ms.
}
},
- pointerEnter: async event => {
+ pointerEnter: event => {
let desc = event.target.dataset.desc || event.target.title || '';
// Don't create tooltip if bubbled.
@@ -72,7 +72,7 @@ window.tsfTT = function () {
// Clear title to prevent default browser tooltip.
event.target.removeAttribute( 'title' );
- return await doTooltip( event, event.target, desc );
+ return doTooltip( event, event.target, desc );
}
return false;
@@ -339,7 +339,7 @@ window.tsfTT = function () {
* @function
* @param {Event} event
*/
- const loadToolTip = async event => {
+ const loadToolTip = event => {
if ( event.target.dataset.hasTooltip ) return;
@@ -365,7 +365,7 @@ window.tsfTT = function () {
_cancelArrowAnimation();
- if ( ! ( await _activeToolTipHandles.pointerEnter( event ) ) ) return;
+ if ( ! ( _activeToolTipHandles.pointerEnter( event ) ) ) return;
// Initiate arrow placement directly.
_activeToolTipHandles.pointerMove( event );
@@ -471,6 +471,7 @@ window.tsfTT = function () {
);
element.prepend( tooltip );
+ ttMap.set( element, tooltip );
const boundary =
element.closest( ttSelectors.boundary )
@@ -631,7 +632,7 @@ window.tsfTT = function () {
tooltip.style.bottom = `${tooltipHeight - offsetTop}px`;
}
- return tooltip;
+ return true;
}
/**
@@ -652,7 +653,7 @@ window.tsfTT = function () {
* @param {string} desc The tooltip, may contain renderable HTML.
* @return {Promise<Boolean>} True on success, false otherwise.
*/
- async function doTooltip( event, element, desc ) {
+ function doTooltip( event, element, desc ) {
// Backward compatibility for jQuery vs ES.
if ( element?.[0] )
@@ -666,10 +667,7 @@ window.tsfTT = function () {
if ( ! desc.length ) return false;
- ttMap.set( element,
- // It’s async when Gutenberging.
- await _renderTooltip( event, element, desc )
- );
+ _renderTooltip( event, element, desc );
return true;
}
@@ -792,7 +790,7 @@ window.tsfTT = function () {
document.addEventListener( 'tsf-gutenberg-tt', ( event ) => {
( { _renderTooltip, removeTooltip } =
- event.detail( { removeTooltip, ttNames, ttSelectors } ) );
+ event.detail( { removeTooltip, ttMap, ttNames, ttSelectors } ) );
} );
},
}, {
There was a problem hiding this comment.
Pull Request Overview
This PR adds Gutenberg (block editor) compatibility for tooltips by rendering them into the document body using WordPress's Popover component, preventing clipping issues caused by overflow and positioning constraints in the block editor. The implementation overrides tooltip rendering logic specifically for Gutenberg while keeping the classic editor behavior unchanged.
- Introduces a tooltip map (
ttMap) to track tooltip elements and enable external override of rendering logic - Implements
GBCTooltipReact component that wraps tooltips in WP'sPopoverfor proper positioning - Updates image preview tooltip styling to use flexible dimensions with aspect ratio constraints
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/js/tt.js | Adds tooltip map for external overrides, refactors element retrieval into getter method, makes doTooltip async, removes Gutenberg-specific boundary selector |
| lib/js/gbc.js | Implements React-based tooltip override using WP Popover component, dispatches custom event to inject override into tt.js |
| lib/js/media.js | Updates image preview tooltip styling to use percentage-based sizing with aspect ratio and natural dimensions |
| lib/css/tt.css | Comments out flex property with question about its necessity |
| lib/css/post.css | Removes Gutenberg-specific overflow fixes that are no longer needed |
| lib/css/media.css | Adds inline-block display for tooltip wrap in Gutenberg context |
| lib/css/gbc.css | New stylesheet with Popover-specific tooltip styling for Gutenberg |
| inc/classes/admin/script/loader.class.php | Adds wp-element and wp-components dependencies, registers new gbc.css stylesheet |
|
|
||
| // Use textWidth for right boundary if adjustment exceeds. | ||
| if ( boundaryRight + adjust > _activeTooltipElements.wrap.offsetWidth ) { | ||
| if ( boundaryRight + adjust > wrapRect.right ) { |
There was a problem hiding this comment.
The comparison on this line is incorrect. boundaryRight + adjust is a width value in pixels, while wrapRect.right is an absolute viewport coordinate. This should likely compare against wrapRect.width or similar dimension property instead.
| if ( boundaryRight + adjust > wrapRect.right ) { | |
| if ( boundaryRight + adjust > wrapRect.width ) { |
lib/js/gbc.js
Outdated
|
|
||
| if ( ! desc ) return; | ||
|
|
||
| const __html = `<span class=${ttNames.textWrap}><span class=${ttNames.text}>${desc}</span></span><div class=${ttNames.arrow} style=will-change:left></div>` |
There was a problem hiding this comment.
Missing semicolon at the end of this line. According to project standards, statements should end with semicolons.
sybrew
changed the title
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
stokesman
left a comment
stokesman
left a comment
There was a problem hiding this comment.
I addressed what feedback seemed directly actionable.
There was a problem hiding this comment.
Oh, I hadn’t used var(--wp…) to patch the contrast issue, but to avoid having to add to the inline CSS for the GB tooltip arrow—although that would be a small addition. It also seemed like it might be okay to have it match the used accent color in GB which is different from classic (and seemingly with intention).
Now, as a way to keep colors as before without having to add to the inline CSS, I've pushed c8cacba, that proposes to use CSS variables. No sweat if that’s off the table for any reason—it seemed worth a try.
I also put in a brief effort to understand why the GB admin accent color isn’t the same as what TSF pulls from the global. I didn’t find out but I did find that contrast issue is seemingly not considered one upstream:
“Historically, only the default color scheme is meant to be accessible” WordPress/gutenberg#6438 (review)
| ttMap.set( element, | ||
| // It’s async when Gutenberging. | ||
| await _renderTooltip( event, element, desc ) | ||
| ); |
There was a problem hiding this comment.
I’m confused on this. The jsdoc says it’s asynchronous (since 4.2.0) and returns a promise. I guess that’s not accurate but the only place it’s invoked within the plugin it’s awaited as if already async:
the-seo-framework/lib/js/tt.js
Line 71 in ff95bd8
Anyway, I did give it shot and it can work synchronously. Apart from whether sync or async, I only dislike how it requires reproducing one more bit of logic (ttMap.set()) in the gbc _renderTooltip override. There’s already a little duplication there that might ideally be obviated but I’d not stressed it since this is speculative. I didn’t push the changes but the patch follows.
Patch to move ttMap.set() inside _renderTooltip and make Gutenberg _renderTooltip override work synchronously. Also removes seemingly unneeded await and async to beg the question.
diff --git a/lib/js/gbc.js b/lib/js/gbc.js
index 2f2c06ac..2e2ab3ef 100644
--- a/lib/js/gbc.js
+++ b/lib/js/gbc.js
@@ -346,21 +346,21 @@ window.tsfGBC = function( $ ) {
);
base.ttSelectors.arrow = '.components-popover__arrow';
return {
- _renderTooltip: async ( _event, element, desc ) => {
+ _renderTooltip: ( _event, element, desc ) => {
element.dataset.hasTooltip = 1;
- const tooltipPromise = new Promise( ( resolve ) => {
- const setTooltip = ( tooltip ) => resolve( tooltip );
+ const setTooltip = tooltip => base.ttMap.set( element, tooltip );
+ wp.element.flushSync( () =>
root.render(
wp.element.createElement(
GBCTooltip,
{ element, desc, setTooltip, tt: base }
)
- );
- } );
- return await tooltipPromise;
+ )
+ );
+ return true;
},
removeTooltip: ( element ) => {
- root.render( null );
+ root.render( null ); // Should this also be wrapped in flushSync? — it tests okay as is…
base.removeTooltip( element );
}
};
diff --git a/lib/js/tt.js b/lib/js/tt.js
index 36460ace..6a9b302b 100644
--- a/lib/js/tt.js
+++ b/lib/js/tt.js
@@ -62,7 +62,7 @@ window.tsfTT = function () {
event.target.dispatchEvent( new Event( 'mousemove' ) ); // event time: <.3ms.
}
},
- pointerEnter: async event => {
+ pointerEnter: event => {
let desc = event.target.dataset.desc || event.target.title || '';
// Don't create tooltip if bubbled.
@@ -72,7 +72,7 @@ window.tsfTT = function () {
// Clear title to prevent default browser tooltip.
event.target.removeAttribute( 'title' );
- return await doTooltip( event, event.target, desc );
+ return doTooltip( event, event.target, desc );
}
return false;
@@ -339,7 +339,7 @@ window.tsfTT = function () {
* @function
* @param {Event} event
*/
- const loadToolTip = async event => {
+ const loadToolTip = event => {
if ( event.target.dataset.hasTooltip ) return;
@@ -365,7 +365,7 @@ window.tsfTT = function () {
_cancelArrowAnimation();
- if ( ! ( await _activeToolTipHandles.pointerEnter( event ) ) ) return;
+ if ( ! ( _activeToolTipHandles.pointerEnter( event ) ) ) return;
// Initiate arrow placement directly.
_activeToolTipHandles.pointerMove( event );
@@ -471,6 +471,7 @@ window.tsfTT = function () {
);
element.prepend( tooltip );
+ ttMap.set( element, tooltip );
const boundary =
element.closest( ttSelectors.boundary )
@@ -631,7 +632,7 @@ window.tsfTT = function () {
tooltip.style.bottom = `${tooltipHeight - offsetTop}px`;
}
- return tooltip;
+ return true;
}
/**
@@ -652,7 +653,7 @@ window.tsfTT = function () {
* @param {string} desc The tooltip, may contain renderable HTML.
* @return {Promise<Boolean>} True on success, false otherwise.
*/
- async function doTooltip( event, element, desc ) {
+ function doTooltip( event, element, desc ) {
// Backward compatibility for jQuery vs ES.
if ( element?.[0] )
@@ -666,10 +667,7 @@ window.tsfTT = function () {
if ( ! desc.length ) return false;
- ttMap.set( element,
- // It’s async when Gutenberging.
- await _renderTooltip( event, element, desc )
- );
+ _renderTooltip( event, element, desc );
return true;
}
@@ -792,7 +790,7 @@ window.tsfTT = function () {
document.addEventListener( 'tsf-gutenberg-tt', ( event ) => {
( { _renderTooltip, removeTooltip } =
- event.detail( { removeTooltip, ttNames, ttSelectors } ) );
+ event.detail( { removeTooltip, ttMap, ttNames, ttSelectors } ) );
} );
},
}, {
| > .tsf-tooltip-wrap { | ||
| /* In gutenberg, this ensures the wrap has the size of its contents, not sure why it’s | ||
| not required in the classic editor. */ | ||
| display: inline-block; |
There was a problem hiding this comment.
Firefox, Safari and Chrome all show me the same thing. Probably, it’s just not clear what to look for. Here’s a recording that should clarify:
social-image-tt-wrap-size.mp4
Apparently the popover component in Gutenberg reads the wrap’s rect differently. So, of course, this isn’t required in regular admin screens - the comment could have been better. I suppose this style should move to gbc.css and maybe applied generically to all wraps if it doesn’t mess up any other wraps.
| imageObject.style = "max-width:100%;max-height:100%;object-fit:contain;aspect-ratio:1.91;height:auto;border-radius:3px;display:block;"; | ||
| // Add dimensions to prevent jumping. | ||
| imageObject.width = imageObject.naturalWidth; | ||
| imageObject.height = imageObject.naturalHeight; |
There was a problem hiding this comment.
It’s looking stable in my testing:
Gutenberg:
social-image-jump-test.mp4
Category edit:
social-image-category-jump-test.mp4
There was a problem hiding this comment.
Also, so much extra CSS to accommodate a specific page type for the tooltip, which otherwise works universally, seems like a workaround for a bug—are you certain this isn't an upstream issue?
While it might be considered an upstream issue I doubt it would be considered a bug. If their Popover component’s API had a few more features this could likely be reduced. There’s another approach that could be tried but I can’t say if it’d reduce this by much. It would also require :has() which I still hesitate to use.
| ); | ||
| } | ||
|
|
||
| function GBCTooltip( { element, desc, setTooltip, tt } ) { |
There was a problem hiding this comment.
I think it might not be clear how much responsibility this wrapper has in GB. The tooltips work everywhere else because they can position themselves to their relative positioned wrap. Once rendered apart from their wraps the positioning task is trickier and this wrapper (react component) handles that. This includes keeping the position updated through scrolling or sizing changes.
It’s probably true that ideally the wp-components package could expose a (react) hook, that enables applying the popover positioning system (which is floating-ui) without the Popover component. Speaking of that, before I tried this PR, I tried a branch using floating-ui instead. It’s great in some ways but I doubted it would be favorable because it adds ~20kb.
Since the last time I worked on this I happened to remember that the web standard Popover related APIs are coming along nicely. That seems like the ideal thing to base tt.js on in the near future. I mention that in case it helps to think of this a temporary measure which can be ripped out later. That said, I’d understand if you want to look at doing a even smaller temporary measure.
| get: ( element ) => { | ||
| if ( ! _activeTooltipElements.tooltip ) { | ||
| const tooltip = getTooltip( element ); | ||
| if ( ! tooltip ) return {}; | ||
|
|
||
| Object.assign( _activeTooltipElements, { | ||
| tooltip, | ||
| arrow: tooltip.querySelector( ttSelectors.arrow ), | ||
| wrap: element.closest( ttSelectors.wrap ) || element.parentNode, | ||
| } ); | ||
| } | ||
| return _activeTooltipElements; |
There was a problem hiding this comment.
Should it not just default to "no tooltip" if the registry (Map) is empty, without trying to find and register one anyway?
In 9a3d561, I think I did what you suggest (just the quoted bit).
I'm not sure I get the idea with maps for _activeTooltipElements and _pointer. I actually don’t see the need for a map for the tooltips and have tested doing without but the map makes sense with being more congruent with what’s being done already.
| return element?.classList.contains( ttNames.base ) | ||
| ? element | ||
| : element?.querySelector( ttSelectors.base ); | ||
| return ttMap.get( element ) || ttMap.get( | ||
| element?.querySelector( ttSelectors.base ) | ||
| ); |
There was a problem hiding this comment.
If I'm not mistaken I just realized this change isn’t BC. The existing logic returns element if it is a tooltip. The changed logic tries to lookup a tooltip by the element, so if element is a tooltip itself, then it’d return undefined.
| 'border-bottom-color:{{$bg_accent}}', | ||
| ':root' => [ | ||
| '--tsf-user-colors-bg-accent:{{$bg_accent}}', | ||
| '--tsf-user-colors-fg-accent:{{$rel_bg_accent}}', |
Added support for the new Gutenberg sidebar container ID in WordPress 6.9 to prevent tooltip content from overflowing or being clipped. Updated tooltip boundary detection logic and documented the fix in the changelog.
Refactored admin pool to expose seobar and scripts subpools, and made Loader and Builder classes extensible. Improved tooltip accessibility and usability, including better focus handling and max-width enforcement. Updated CSS for SEO bar and tooltips, and clarified description generation messaging.
Meant to robustly fix #741. This adds some overriding of
tsfTTand applies it viatsfGBC. It makes tooltips be rendered into a child of thebodyso they’ll have no clipping ancestors (i.e. no boundaries other than the window). It utilizes WP’sPopovercomponent as that handles the placement of the tooltip.Why do it this way?
tt.js– e.g. boundaries.tt.jsa bit by removing its positioning logic. In WP 6.9 thewp-componentsis going to be loaded on every admin screen because they're adding the “Command palette” everywhere.Show & tell
added-gbc-tooltip.mp4
While testing this out I found that it fixes a related issue with the social image popover that occurs for me in both the classic and the block editor. See below: