WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.45k stars 4.17k forks source link

Block Toolbar: Link Tooltip overlapping #38723

Open sadikmultani opened 2 years ago

sadikmultani commented 2 years ago

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

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 commented 2 years ago

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 commented 2 years ago

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

ramonjd commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

stokesman commented 2 years ago

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

ironprogrammer commented 2 years ago

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

Please re-open this issue.

willrowe commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

ironprogrammer commented 2 years ago

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 commented 2 years ago

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.

talldan commented 1 year ago

Did some investigation, I think it may have changed after https://github.com/WordPress/gutenberg/pull/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: https://github.com/WordPress/gutenberg/blob/95c65750ac12440bb53446d0557d7c4e91968372/packages/components/src/popover/index.tsx#L339

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.

willrowe commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.