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

Block Toolbar: Link Tooltip overlapping #38723

Open
sadikmultani opened this issue Feb 11, 2022 · 22 comments
Open

Block Toolbar: Link Tooltip overlapping #38723

sadikmultani opened this issue Feb 11, 2022 · 22 comments
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release

Comments

@sadikmultani
Copy link

Description

Hi

When we try to add a link on starting or ending of the line in editor from block toolbar then the link tooltip is overlapping behind the sidebars & left admin menu navigation.

Thanks

Step-by-step reproduction instructions

  1. open the editor and write some content
  2. Try to add a link in starting or ending of the line

Screenshots, screen recording, code snippet

Expected behavior: ( Working in 5.8.3 )
https://prntscr.com/26stpvk
https://prntscr.com/26stwuu

Actual behavior: ( in 5.9 )
https://prntscr.com/26stpvh
https://prntscr.com/26sty4u

Environment info

  • WordPress version 5.9
  • Twenty Twenty Two theme

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@talldan talldan added [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Type] Regression Related to a regression in the latest release labels Feb 11, 2022
@talldan
Copy link
Contributor

talldan commented Feb 11, 2022

This doesn't seem to be a z-index issue (at least I tried setting the z-index very high and it still wasn't fixed).

@sadikmultani
Copy link
Author

sadikmultani commented Feb 11, 2022

yup, I've also tried to fix it with z-index but not found the solution.

@ramonjd
Copy link
Member

ramonjd commented Feb 17, 2022

Could it be related to the Popover?

It looks like it's not calculating its left position relative to the editor wrapper:

Screen Shot 2022-02-17 at 4 08 46 pm

I see that the parent with the most relevant offsetLeft is #wpbody. If add document.querySelector( '#wpbody' ).offsetLeft to the Popover's calculated left position, it sits where it should.

Screen Shot 2022-02-17 at 4 29 36 pm

The offsetParent returned at this line is .edit-post-visual-editor, but that has an offsetLeft of 0.

@ironprogrammer
Copy link
Contributor

If there was a minimum width for the content area, then aligning the offset could work -- so long as the popover doesn't get wider than the minimum. But at this time the content area doesn't have a minimum width, and will shrink until the mobile breakpoint is hit (<782px).

Consider this example, where the content area is narrower than the popover:

image
Block editor with link popover activated; z-index fix applied for clarity of popover position.

A) The popover acts as a true modal (fixed position, z-index of 1MM), and its static width can overlap either sidebar. It cannot easily be accessed through scrolling.
B) The toolbar in the content section (absolute position, relative to the editor) affects the section's width, so is accessible with horizontal scrolling.

The problem of the popover being hidden under the sidebars appears to have been introduced when the content area's z-index was statically set via #32869 (via #32732) to resolve a different issue, #32631. The .interface-interface-skeleton__content element's z-index was set to 20.

Stacking context is at play here. So long as the content area's z-index (20) is lower than its sibling element sidebars (90), any descendant elements like the popover will appear "underneath" the sidebars.

I've been testing some adjustments, like moving the static z-index to .interface-interface-skeleton__body (the original solution for #32869), but as discussed in the PR this has a side affect to the Publish button's visibility. There are a couple more avenues I'm exploring, and will share those findings soon.

@ironprogrammer
Copy link
Contributor

I have a fix inbound; writing up the PR now.

@stokesman
Copy link
Contributor

I appears this should be closed since #38893. Do reopen if I'm mistaken.

@ironprogrammer
Copy link
Contributor

The PR to address this issue was reverted in #39620 (see PR for details).

Please re-open this issue.

@stokesman stokesman reopened this Mar 22, 2022
@willrowe
Copy link

willrowe commented Mar 29, 2022

As was mentioned earlier in this issue (and was discovered when the "fix" needed to be reverted), adjusting the z-index is not the solution.

The main issue is really the edge detection and sizing of the popovers. This is best demonstrated when turning off Fullscreen mode, since the popovers will then appear underneath the admin sidebar on the left. You can see an example of this in the comment from @ramonjd above. Because that admin sidebar can display sub-menus that should appear above the editor when hovering, you do not want the editor to ever be displayed above it, hence why adjusting the z-index is not the solution.

All popovers in the content area should determine their position relative to edges of the content area itself, not the entire document. Doing so will ensure that it is always displayed within the content area and has no chance to overlap with any block editor or admin sidebars that may or may not be displayed. Additionally, the width of popovers should probably be constrained to be no larger than the width of the content area to ensure that it does not overflow and get clipped on narrow screens.

@bfintal
Copy link
Contributor

bfintal commented May 6, 2022

I was investigating this and I don't think the issue is with z-index. When you bring up the link Popover from the block toolbar, and investigate the Popover, you'll see multiple popover-slot divs:

Screen Shot 2022-05-07 at 12 56 43 AM

I labeled them 1, 2 and 3 in the screenshot above.

The link popover goes to slot 2, and the slot is contained inside the editor content area (not sure what that's called), therefore clipping the popover because of overflow.

If you open other Popovers, for example the 3-dots / more option toolbar button, it will go into slot 3. Slot 3 is located outside the editor content area, so the Popover doesn't get clipped.

I think the Popover should go into slot 3 as well.

Another possibly important finding. In my plugin, I have a custom format type created via registerFormatType, and that renders a Popver, I'm also seeing that the Popover is also added in slot 2. However, when I use a Popover in other locations (such as in a block's inspector), the Popover gets added in slot 3.

The issue also happens in WordPress 6.0-RC1.

@ironprogrammer
Copy link
Contributor

In response to @bfintal:

I think the Popover should go into slot 3 as well.

Agreed, this was essentially the conclusion reached with the reverted z-index adjustment PR, which had quite a number of side effects given the already complicated z-axis contexts involved with various parts of the editor and tool surfaces.

I believe that the link popover is the only built-in editor control using that slot (# 2 in your example), and all other popovers are bound to the # 3 slot. As a modal-derived control, popovers should appear "above" all other elements so that the user can focus on completing the task of configuring the link.

If anyone watching this 👀 has experience with slot/fill and could tie the popover to that "outside" slot, then I'd be happy to test and see if we can move toward getting this resolved 🙌🏻

@bfintal
Copy link
Contributor

bfintal commented May 7, 2022

Thanks for pointing that out @ironprogrammer

Aside from the link popover, custom format types that use popovers are also get affected by this.. at least in my custom format type it's happening.

@willrowe
Copy link

willrowe commented May 9, 2022

@ironprogrammer I must reiterate that it does not seem that the solution is to display them in a slot with a higher z-index. There are several contexts in which these popovers can be displayed as part of the editor, so trying to push the z-index higher and higher can conflict with other UI elements. I think a good first step would be to improve the placement of the popovers to not extend outside of the editor bounds, that way it only needs to take the z-index of elements within the editor into account, not other elements outside of it, which may change depending on the context.

@ironprogrammer
Copy link
Contributor

Hi, @willrowe -- The "# 3" slot referenced in @bfintal's screenshot is where the other editor controls surface their respective popovers, so this could also be considered a matter of consistency between editor-triggered popovers. That location is already being used to ensure the popovers are "on top" of the editor itself. This was my line of reasoning.

Here is a screenshot showing one example with a narrow editor window, and how the the link popover is affected by the editor sidebar panels:

The other popovers triggered by the block's menu have this "solved" by not filling within the editor itself, but through that "external" slot.

I agree that piling on additional z-index contexts is not the right approach. By using the other existing slot, the link popover would only be using an already established location for its fill.

@willrowe
Copy link

@ironprogrammer would using this slot work with other UI elements like the dashboard sidebar when not in fullscreen?

@ironprogrammer
Copy link
Contributor

Yes, it should, inasmuch as it would be consistent with the other popovers that trigger in the editor.

The sidebar (#adminmenuwrap) is pinned to z-index: 9990. However, the external slot (#editor > div > .popover-slot > .components-popover) is set to z-index: 1000000. So existing popovers appear "on top of" the admin sidebar when invoked.

The anchor popover appears to be the odd-one-out here.

@Ryokuhi
Copy link

Ryokuhi commented Oct 14, 2022

This issue was reported again in Trac ticket #56553, and the test report in the ticket shows that this is still an issue as of WordPress 6.1 Beta 1.
As described above, there is a focused element that is only partially visible, so this is indeed an accessibility issue (more information can be found in WCAG 2.1, Success Criterion 1.4.13), and should be marked like that.

@Ryokuhi Ryokuhi added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 14, 2022
@talldan
Copy link
Contributor

talldan commented Oct 17, 2022

Did some investigation, I think it may have changed after #34956. Since that PR some popovers are using a different slot than they previously did. Possibly that PR affected more popovers than it needed to.

Reverting some of the code does make the popover appear differently:
Screen Shot 2022-10-17 at 11 03 46 am

In particular, changing this line:

const slotName = useContext( slotNameContext ) || __unstableSlotName;

to this:

const slotName = __unstableSlotName; 

Will brute force the LinkControl so that it appears the way it was before (but will cause the fix in that PR to no longer work). It still doesn't looks great though, and changing this will cause it to overlap the sidebars more because the popover tries to stay centered on whatever element it references.

Some other options to remediate this might be worth looking at.

  • Make LinkControl more responsive
  • Avoid showing both left and right sidebars at the same time on smaller screens? (this already seems to be the case for the block library)

@willrowe
Copy link

It still doesn't looks great though, and changing this will cause it to overlap the sidebars more because the popover tries to stay centered on whatever element it references.

Isn't that the point of a popover though? It's supposed to appear over all other content while in it is currently the active context. If there's enough room on the screen as a whole, I see no need to try to make it responsive, since that would just make it less accessible. The main issue is that it is currently appearing under the sidebars, which makes it unusable in some cases. I don't see having it appear above the sidebars as an issue necessarily.

@talldan
Copy link
Contributor

talldan commented Oct 18, 2022

Isn't that the point of a popover though? It's supposed to appear over all other content while in it is currently the active context.

That's not quite the point I'm making. I don't think it's great for the popover to overlap the sidebars when it doesn't need to (i.e. when there's enough space in the editor content area for the link popover). Typically most themes have a lot of margin, so it usually isn't much of an issue in the post editor. That isn't the case in the site editor where content can be very close to the edge. If there's a way to keep the popover within the confines of the editor content until it really needs to overlap a sidebar then that would be preferable, but I know the default is that it will overlap. An external library is used now for popovers (floating-ui), so I think we need to see what's possible.

There's another issue here that made me suggest only showing one sidebar at a time. The editor content is not entirely legible or usable when both sidebars are open and the content area is reduced to such a small size, so I think that could be avoided and that would help with this problem.

@willrowe
Copy link

If there's a way to keep the popover within the confines of the editor content until it really needs to overlap a sidebar then that would be preferable, but I know the default is that it will overlap.

I think it's fine to adjust it when possible to not overlap, but my point is that having it not overlap as a rule, at the expense of accessibility, doesn't make sense. The top priority should be accessibility, then try to prevent overlaps where reasonable.

The editor content is not entirely legible or usable when both sidebars are open and the content area is reduced to such a small size, so I think that could be avoided and that would help with this problem.

While it is true that it doesn't leave much usable space, I would leave it up to the user whether they want to hide or show sidebars. There is nothing more frustrating than opening something and then something else closes automatically, and then you're fighting against the interface when you may have a valid reason to need both sidebars open to be able to reference things.

@Sogeman
Copy link

Sogeman commented Apr 24, 2023

So anyway to fix this? It's impossible to add links to text if the text is close to the edge of the screen.

@benlk
Copy link
Contributor

benlk commented Sep 6, 2023

Since it hasn't been mentioned yet in this ticket: the positioning issue also applies when the Fullscreen Editor is enabled, in WordPress 6.3.1 at least:

Screenshot 2023-09-06 at 11 44 16

So a z-index fix won't solve the issue.

A better fix would be to detect the edges of the Editor on screen, then make sure the popover is positioned fully within those boundaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

No branches or pull requests

10 participants