Block Editor: Fix overflowing `LinkControl`. #20154

epiqueras · 2020-02-10T20:23:42Z

Improves #20146.

aduth · 2020-02-10T20:28:52Z

This seems to have the effect of making the input much narrower than it was previously in larger viewports:

Before	After

Any way we can retain the width?

epiqueras · 2020-02-10T20:32:38Z

Yes, a media query. Updated!

aduth · 2020-02-10T20:50:03Z

I think this helps make it less likely to overflow, since the natural width of the popover will be much narrower, but it's still possible for overflow to happen, at least on some smaller devices like iPhone SE.

Conversely, on larger mobile devices, there's a lot of extra space available, and the input feels unnecessarily small.

For me, an ideal solution would be one in which the component maximizes the space available, up to a point. For example:

width: 90vw;
max-width: $modal-min-width;

The trouble is that the popover implementation is rather naive in how it tries to reposition when space is not available. Currently, it will only ever try to flip the desired position, which means for something which occupies most of the width of the viewport, it's quite likely it's still going to overflow after the flip. I think it would be nice if the popover was implemented to "push itself away" from the edge of the screen instead.

There's also another issue here: The width of the suggestions have their own separate width, so the popover can suddenly widen after entering a search term. The widened popover can stretch beyond the viewport, even in larger devices like iPhone X.

I believe the intention is that the popover shouldn't ever resize after it's already visible. At least, this is how it behaves on larger displays in master.

ZebulanStanphill · 2020-02-10T21:04:01Z

I think it currently only exists in the G2 UI branches, but perhaps the same method of keeping the block toolbar from going off-screen could be used here.

epiqueras · 2020-02-10T21:08:30Z

@aduth The latest update should handle those cases better.

aduth · 2020-02-12T17:49:26Z

For me, that regresses back to what it was in #20146.

Which, from my perspective, would make sense following the change. I could have been clearer, but in my previous comment I meant to imply that it would be ideal if we could express it this way (at least as far as expressing the desired width of the popover), but that the current implementation of popover cannot support this because its implementation of repairing overflow is currently rather naive.

epiqueras · 2020-02-12T17:59:30Z

I’m not seeing that with my latest changes. The popover stays at 90vw so it never overflows.

aduth · 2020-02-12T18:15:18Z

It would never overflow if it could be guaranteed to be centered on the screen. It likely depends where horizontally in a line of a paragraph the dialog is opened, since the behavior of that dialog is to open centered at wherever the selection is, which can be (but isn’t always) aligned to the center of the viewport.

epiqueras · 2020-02-12T18:24:53Z

Right, but this is still way better than master. We can fix the popover later.

aduth · 2020-02-12T18:44:41Z

It's slightly better, in that the natural width will be slightly narrower on smaller viewports (360px -> 337px on iPhone X). And it scales relative to the viewport size, which helps. The overflow still happens though.

I would be okay to merge it as an improvement, but I'd not mark it as fixing #20146.

epiqueras · 2020-02-12T19:23:19Z

Yeah, there are still edge cases because of the popover flip. I've updated the description.

This PR is still needed even if we fix the popover.

aduth · 2020-02-12T19:31:58Z

I might wonder if this would negatively impact the reusability of this component. Specifically thinking about #18061. LinkControl should not be assumed to be rendered in a popover (as of #19638), so constraining its width might not be how we'd want it to behave in other contexts (e.g. #18061 (comment)). I think it would be fair to apply this styling when it is the case that LinkControl is rendered within a popover, but I'm not sure where these styles should live. It's the sort of thing I was wondering in #18061 (comment) with my comment of "Could URLPopover be repurposed to render the stylized LinkControl".

epiqueras · 2020-02-12T19:40:37Z

Good points. I've scoped the styles.

packages/block-editor/src/components/link-control/style.scss

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

aduth · 2020-02-12T20:15:53Z

packages/block-editor/src/components/link-control/style.scss

 		text-overflow: ellipsis;
 		max-width: 230px;
 		overflow: hidden;
-		white-space: nowrap;


The three styles text-overflow, overflow, and white-space are pretty directly linked, if I understand it correctly. I would expect that either all should be removed, or all should remain.

What was the purpose for removing white-space? As best I can tell, in combination with the other styles, it would not been the case that the element would stretch, and instead it would be truncated.

Contrast that with how it behaves after these changes:

I think either could work, but it seems like the behavior of the former was likely a conscious design decision to keep the heights of these suggestions consistent (cc @getdave).

I'd guess this might have been related to my comment at #20154 (comment) ? It feels to me we shouldn't want there to be an explicitly assigned max-width here, at least not one with a fixed pixel value. I'd guess this also hurts reusability of the component, since it seems pretty focused on it being rendered in the popover.

I removed it because the header still stretches up to 230px with it.

Yes it was deliberate to ensure consistent height

Sorry i dont get these pings. I need better GH skills!

It's not working for me in Chrome. The content is cut off without an ellipsis.

youknowriad · 2020-02-13T08:18:52Z

packages/block-editor/src/components/link-control/style.scss

@@ -3,6 +3,12 @@ $block-editor-link-control-number-of-actions: 1;
 .block-editor-link-control {
 	position: relative;
 	min-width: $modal-min-width;
+
+	.components-popover__content & {


Why the LinkControl should be aware that. it can be used in a Popover?

Are you saying the parent component should apply a custom class name?

I don't know but it feels like:

The LinkControl shouldn't be aware of a Popover and should behave properly no matter where it's rendered (fluid maybe)

The Popover shouldn't be aware of its children of course but has smart logic to adapt to the viewport.

Now, I don't know the details of this issue to argue exactly what the fix should be.

The LinkControl shouldn't be aware of a Popover and should behave properly no matter where it's rendered (fluid maybe)

Agreed, this PR furthers that.

The Popover shouldn't be aware of its children of course but has smart logic to adapt to the viewport.

This requires a bigger refactor of popovers.

Agreed, this PR furthers that.

It doesn't seem like it since there's an explicit mention of popover in the link control style.

This requires a bigger refactor of popovers.

The popover should already have the logic to adapt the width to the viewport built-in. Maybe there's a bug there.

It doesn't seem like it since there's an explicit mention of popover in the link control style.

Which is used to remove pixel-based dimension constraints.

The popover should already have the logic to adapt the width to the viewport built-in. Maybe there's a bug there.

Yeah, that logic is not very robust and it overflows the screen in most cases.

Which is used to remove pixel-based dimension constraints.

Yes, this is cool but can't we do it consistently without any mention of the popover class. I'm thinking the LinkControl can also be shown in narrow contexts independent of the Popover being present or not.

No, because the way we can dampen the effect of the popover bug here does not necessarily work in other narrow contexts. We can only assume a 90vw is OK because the popover renders on top of everything.

Do you think a generic solution based on "react-resize-aware" would work here like the Placeholder component?

Yes, or react-spring/react-use-measure.

epiqueras · 2020-02-17T18:51:57Z

@aduth This is still an improvement over master, right? Can we merge this and fix the Popover issue separately?

youknowriad · 2020-02-18T08:32:15Z

Why don't we try to fix the issue once and for all by making the component responsive?

epiqueras · 2020-02-18T13:35:20Z

See #20154 (comment).

jorgefilipecosta · 2020-02-24T20:25:38Z

I'm merging this PR as it seems a simple enhancement that can be included in WordPress 5.4. As a follow up I guess we can use a "react-resize-aware" or react-use-measure based approach.

aduth · 2020-02-25T19:24:45Z

With regards to the discussion here about the tooltip's overflow mechanics, I think it might be worth considering to pull in Popper to replace our implementation. I've been a bit resistant to it when brought up previously, in part because our custom implementation has been mostly sufficient for our purposes, and we don't need all of what the library brings with it (i.e. avoid inflating bundle size).

Notably, it does quite well in this regards of "pushing away" from the bounds of a container:

https://popper.js.org/docs/v2/modifiers/arrow/

There are two other things to follow-up on here:

My comment at Block Editor: Fix overflowing LinkControl. #20154 (comment) alluded that if white-space is removed, then we probably should also remove the related text-overflow and overflow styles.
Per @getdave's comment at Block Editor: Fix overflowing LinkControl. #20154 (comment), we now no longer have the consistent heights intended with the link suggestions.

Issue: #20446

epiqueras · 2020-02-25T19:51:36Z

Yes, I also think this is a place for a third party library.

youknowriad · 2020-02-26T08:32:34Z

I've used Popper in a personal project. I think it's great but it won't replace our Popover entirely, it's something we can use to remove some custom logic from Popover and use it as a low-level abstraction.

We also have very custom behaviors (toolbar moving when it hits the edge of the editor canvas) that will most likely need to be implemented as "custom modifiers" for Popper.

epiqueras added [Type] Bug An existing feature does not function as intended [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Feb 10, 2020

epiqueras added this to the Gutenberg 7.5 milestone Feb 10, 2020

epiqueras requested a review from aduth February 10, 2020 20:23

epiqueras requested review from ellatrix and youknowriad as code owners February 10, 2020 20:23

epiqueras self-assigned this Feb 10, 2020

epiqueras force-pushed the fix/overflowing-link-control branch from 451cb8e to e4a911a Compare February 10, 2020 20:32

Block Editor: Fix overflowing LinkControl.

bd256c9

epiqueras force-pushed the fix/overflowing-link-control branch from e4a911a to bd256c9 Compare February 10, 2020 21:07

aduth modified the milestones: Gutenberg 7.5, Gutenberg 7.6 Feb 12, 2020

Block Editor: Scope changes to popover content.

f25c945

aduth requested changes Feb 12, 2020

View reviewed changes

packages/block-editor/src/components/link-control/style.scss Outdated Show resolved Hide resolved

Update packages/block-editor/src/components/link-control/style.scss

5f7c53e

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

aduth reviewed Feb 12, 2020

View reviewed changes

youknowriad reviewed Feb 13, 2020

View reviewed changes

jorgefilipecosta merged commit 598b24e into master Feb 24, 2020

jorgefilipecosta deleted the fix/overflowing-link-control branch February 24, 2020 20:25

This was referenced Feb 25, 2020

Format Library: Inline link formatting popover can stretch beyond viewport #20146

Closed

Block Editor: LinkControl suggestions height consistency, overflow #20446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Editor: Fix overflowing `LinkControl`. #20154

Block Editor: Fix overflowing `LinkControl`. #20154

epiqueras commented Feb 10, 2020 •

edited

Loading

aduth commented Feb 10, 2020

epiqueras commented Feb 10, 2020

aduth commented Feb 10, 2020

ZebulanStanphill commented Feb 10, 2020

epiqueras commented Feb 10, 2020

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020 via email

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020 via email

aduth commented Feb 12, 2020 •

edited

Loading

epiqueras commented Feb 12, 2020

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020

aduth Feb 12, 2020

epiqueras Feb 12, 2020

getdave Feb 17, 2020

epiqueras Feb 17, 2020

youknowriad Feb 13, 2020

epiqueras Feb 13, 2020

youknowriad Feb 13, 2020

epiqueras Feb 13, 2020

youknowriad Feb 13, 2020

epiqueras Feb 13, 2020

youknowriad Feb 13, 2020

epiqueras Feb 13, 2020

youknowriad Feb 14, 2020

epiqueras Feb 14, 2020

epiqueras commented Feb 17, 2020

youknowriad commented Feb 18, 2020

epiqueras commented Feb 18, 2020

jorgefilipecosta commented Feb 24, 2020

aduth commented Feb 25, 2020 •

edited

Loading

epiqueras commented Feb 25, 2020

youknowriad commented Feb 26, 2020

Block Editor: Fix overflowing LinkControl. #20154

Block Editor: Fix overflowing LinkControl. #20154

Conversation

epiqueras commented Feb 10, 2020 • edited Loading

aduth commented Feb 10, 2020

epiqueras commented Feb 10, 2020

aduth commented Feb 10, 2020

ZebulanStanphill commented Feb 10, 2020

epiqueras commented Feb 10, 2020

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020 via email

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020 via email

aduth commented Feb 12, 2020 • edited Loading

epiqueras commented Feb 12, 2020

aduth commented Feb 12, 2020

epiqueras commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented Feb 17, 2020

youknowriad commented Feb 18, 2020

epiqueras commented Feb 18, 2020

jorgefilipecosta commented Feb 24, 2020

aduth commented Feb 25, 2020 • edited Loading

epiqueras commented Feb 25, 2020

youknowriad commented Feb 26, 2020

Block Editor: Fix overflowing `LinkControl`. #20154

Block Editor: Fix overflowing `LinkControl`. #20154

epiqueras commented Feb 10, 2020 •

edited

Loading

aduth commented Feb 12, 2020 •

edited

Loading

aduth commented Feb 25, 2020 •

edited

Loading