Web-stories-wp: Regression: line-height/text element in preview has whitespace in the edges

[miina]

Bug Description

In https://github.com/google/web-stories-wp/issues/3530 we removed the whitespace of the text element so there would be no whitespace no matter the line-height.

There seems to be a regression in the front-end.

Expected Behaviour

1. Front-end and the editor view should match
2. There should be no whitespace between the text and the edges unless there's padding assigned.
   

Steps to Reproduce

1. Add a text element, set the background to Fill
2. Add some line-height for making the issue more visible
3. Compare the editor and the front-end views.
   

Screenshots

Editor:

The Front-end:

This causes additional oddities when assigning inner/center border since the border is added based on the expected text element size, not how it currently is:
Editor with inner border:

FE with inner border:

Additional Context

• Plugin Version:
• Operating System:
• Browser:

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance Criteria

Implementation Brief

[samitron7]

Please also ensure that we add any additional padding to the text box to make applying border beautiful. The same rule that applies for text box fill should apply here.

[choumx]

Can we change the editor sizing of text boxes rather than the rendered output (i.e. fix calculation of text bounding boxes with line height in editor)?

I think it actually looks better to respect line height at the top/bottom of the text box. I.e. the "front-end" screenshot looks better than the "editor" screenshot.

This will cause visual regressions in users' stories that rely on the current text box sizing behavior. But it's already affecting the output of their stories, so arguably it's better to be consistent.

[miina]

I wonder if we can change the editor sizing of text boxes rather than the rendered output (i.e. fix calculation of text bounding boxes with line height in editor). I think it actually looks better to respect line height at the top/bottom of the text box.

In terms of the looks -- if I understood @samitron7 correctly then there's a high priority issue coming for adding padding to the text to each side when the text element has background, this would fix the looks issue.

Can we change the editor sizing of text boxes rather than the rendered output (i.e. fix calculation of text bounding boxes with line height in editor)?

Essentially this would mean adding space between the edges and the content. What about the case when there is no background set and it's just text -- should there still be space between the text and the selection box in the editor? Thoughts?

[samitron7]

@miina 4980 is in review and should address the default padding for hi-hi-lite fill.

Would it be possible to see screenshot of what the text box (with no bg fill) looks like with the fix calculation?

[miina]

@samitron7 If we'd follow Will's suggestion and change the editor boxing instead of restoring the output how it was before, then this would mean that in the editor there would be space between the selection line even when there is no background assigned (that happens in the output as well, it's just not visible without a background). Then it would look something like this without the background:

Currently, there is no space:

[choumx]

What about the case when there is no background set and it's just text -- should there still be space between the text and the selection box in the editor? Thoughts?

I think that's actually desirable since it shows the user what the area of a fill background would be (before any is set).

[miina]

I think that's actually desirable since it shows the user what the area of a fill background would be (before any is set).

Sure! Note that this would influence snapping -- if the user would want to align/snap the text content to another element etc. very precisely they'd need to do that manually since snapping is based on the selection box, but perhaps that's acceptable.

[samitron7]

@miina Before we do this, can you make sure that
1) Text sets are not badly impacted. If they are, we'll need to file a ticket to fix them.
2) Marcin's work to remove the trailing whitespace is not in conflict with this fix. The work was originally started when I reported that setting line height to 5 on a text box pushed the text down leaving a very large white gap at the top of the text box.

[miina]

@samitron7 Thanks for bringing up these points. We'd need more precise UX guidelines for the change before doing this and there are some concerns indeed, including the text sets. Depending on the requirements, we might still need to adjust the preview/output as well, too.

• What would be the expected space/padding size between the content and the edges without fill bg? Would it be just from the top and the bottom as it is in the preview currently? Or would it match what's added in case of the fill / highlight background in #4980?
• If it's only from the top and bottom as currently in the output, would adding a border change that?
• Would this be related to the padding prop/input in the design panel or would this be added in addition to the padding the user can set in the design panel?
• Text Sets will be influenced indeed and might all need adjusting -- for maintaining the same gap between the text set contents as now, the text sets would probably overlap to make up for the extra space added. Is that acceptable -- overlapping elements upon inserting a text set?
• Text presets staggering would also be influenced and for maintaining the same 16px space between the contents, the text presets might also need to overlap to make up for the added space. The staggering considers the outer box for position calculation, meaning that after this change, the gap between the contents would be significantly larger, depending on how much space we'd leave between the selection box and the content. Thoughts?
• As you mentioned, we also need to make sure that Marcin's work will also remain and we would only add the extra required space and not more in case of line-height.

There are quite a few details and influenced parts here. For the purposes of just fixing the bug, it could be worthwhile to also look into fixing the issue in the preview/output instead (remove the extra space that came from the regression) -- if that's an easy fix then we could first do the fix and then follow up in a separate PR with added whitespace as suggested, addressing all the parts involved.

Thoughts?

[samitron7]

@miina what does fixing the issue in the preview/output entail? I want to get some clarity on that before I look into the other things.

[miina]

@samitron7 The original regression came from a change in the preview, not in the editor -- in the preview, previously, everything that was outside of the borders of the text element (any overflow) was hidden in the output. However, since some of the characters were partially cut off this way as well, then the overflow was made visible again within #3530. This caused a regression because previously, when removing the line-height extra whitespace (the work that Marcin did), some of the extra whitespace was hidden by this same overflow. Now when it was displayed again in #3530, the extra space appeared, too, causing the regression.

This means that the editor logic hasn't been influenced at all by this regression, it's just the output (preview) that has changed, and instead of fixing the regression by changing the editor view to match the output, we could instead at first look into fixing the preview that got "broken" in the first place.

[samitron7]

I'm in favor if of that since fixing the issues inside the editor will cause a slew of other issues that you pointed out. Can you we look into fixing the preview.

[miina]

@barklund @bmattb FYI Pulling this into Sprint to work on it.

[csossi]

verified in qa

[choumx]

Nice work fixing this @miina! 🎉

[samitron7]

Verified in UAT