Crop leading & trailing whitespace #4247

merapi · 2020-08-26T21:33:31Z

Summary

Relevant Technical Choices

To-do

User-facing changes

Text elements line-height works now more like a line-gap, there is no whitespace at the top of the first line and bottom of the last line.

Testing Instructions

Add text with 2+ lines of text and set line-height to 2+ (5+ to really see the difference with previous behavior).

Testing old stories:

How should it work?

Let's use the term beta-text for text added/modified using the beta plugin and stable-text for text modified with stable plugin and after (new cropping).
for beta-text that the user never touches after upgrading to stable -- nothing should change
for beta-text that the user modifies the text only, not the formatting -- nothing should change
for beta-text that the user changes styling (font family, font size, font weight, etc.) -- we recalculate cropping/etc using the new formula and in essence turn it into beta-text using their newly set formatting preferences.
for newly added beta-text, just use the new framework.

Create a story on the main branch (beta2) then switch to Crop leading & trailing whitespace #4247 (with cropping) and play around.
Open the old story on Crop leading & trailing whitespace #4247 and play around.

Fixes #2788
Fixes #3530

github-actions · 2020-08-26T21:43:21Z

Size Change: +6.56 kB (0%)

Total Size: 1.25 MB

Filename	Size	Change
`assets/js/edit-story.js`	499 kB	+1.84 kB (0%)
`assets/js/stories-dashboard.js`	589 kB	+2.22 kB (0%)
`assets/js/1-bbc6994c3dcd5c5a3380.js`	1.25 kB	+1.25 kB (100%)	🆘
`assets/js/9-a2ae9e4cb56dda97c498.js`	1.25 kB	+1.25 kB (100%)	🆘

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	296 B	0 B
`assets/css/stories-dashboard.css`	328 B	0 B
`assets/css/web-stories-embed-block.css`	515 B	0 B
`assets/js/chunk-web-stories-template-0-********************.js`	10.2 kB	0 B
`assets/js/chunk-web-stories-template-1-********************.js`	10.3 kB	0 B
`assets/js/chunk-web-stories-template-2-********************.js`	10 kB	0 B
`assets/js/chunk-web-stories-template-3-********************.js`	10.5 kB	0 B
`assets/js/chunk-web-stories-template-4-********************.js`	10.6 kB	+1 B
`assets/js/chunk-web-stories-template-5-********************.js`	6.86 kB	0 B
`assets/js/chunk-web-stories-template-6-********************.js`	9.91 kB	0 B
`assets/js/chunk-web-stories-template-7-********************.js`	9.78 kB	0 B
`assets/js/web-stories-activation-notice.js`	63.6 kB	0 B
`assets/js/web-stories-embed-block.js`	16.9 kB	0 B

_{compressed-size-action}

barklund

This works well. Just a few comments, as I know this is WIP.

It's still missing the migration, right? When working on an old story without changing the font, the text flies all over the place, but that's to be expected I guess?

assets/src/edit-story/components/canvas/singleSelectionMoveable/useResize.js

assets/src/edit-story/elements/text/edit.js

miina · 2020-09-03T14:49:57Z

Generally works well.

Some unexpected results from testing:
I

Had font size 35, font: Princess Sofia, partially regular, partially bold, line-height 3.5, rotated. When I changed the font size to 50, the text started overflowing.

II
When I then clicked on the Page (to leave the edit mode of the Text), this happened:

After doing a new action with the element (e.g. rotating), the frame corrected itself.

Also seeing this in the console:

merapi · 2020-09-07T11:10:03Z

Some unexpected results from testing:

@miina I tried to redo this (in edit mode and not) but can't get to this state, is it reproducible on your side? if yes - can you record a small video? (optionally attach story data dump[base64] right before that issue [before changing the font size to 50?])

miina · 2020-09-07T11:42:51Z

@merapi Just tested, it seems to be reproducible, at least partially.
Found some additional potential issues, too. Steps to reproduce:

Add a Text element
Start typing in `Qué está pasando aquí??" without changing anything else (Using this sentence for the accents on top of the characters)
The first issue comes out here: the text frame is not updating itself and the text goes out of the frame (no line-height set yet)
Set Fill as the background mode
Resize the element to larger
Another issue came out here -- there is empty space below the last line (no line-height set):
Set the line-height to 3.5
Set the font-family to Princess Sofia
Set width to 302 and font-size to 30
Now set the font-size to 50.
The issue of the text going over appears:

Note that now when testing the issue doesn't appear with all the width/font-size values. If you try the following, it should eventually appear:
Resize the text element to small again and then set to 50 again.
Also got this result with font-size 17, width 167, and then setting 50 as the font-size.

Let me know if you can reproduce it.

merapi · 2020-09-07T12:20:42Z

It's still missing the migration, right? When working on an old story without changing the font, the text flies all over the place, but that's to be expected I guess?

For a good start, I did a small change: we don't crop anything if there are no font metrics available - tested that on a few old stories and it looks good (text elements are not exploding when resized). In that scenario only newly added text elements will behave differently (will crop whitespace).

merapi · 2020-09-07T20:08:44Z

@merapi Just tested, it seems to be reproducible, at least partially.
Found some additional potential issues, too. Steps to reproduce:

Hopefully, both are resolved (our Slack chat).

# Conflicts: # assets/src/edit-story/elements/text/display.js

codecov · 2020-09-09T12:20:55Z

Codecov Report

Merging #4247 into main will decrease coverage by 0.14%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##             main    #4247      +/-   ##
==========================================
- Coverage   83.49%   83.34%   -0.15%     
==========================================
  Files         856      861       +5     
  Lines       14981    15140     +159     
==========================================
+ Hits        12508    12619     +111     
- Misses       2473     2521      +48

Flag	Coverage Δ
#karmatests	`71.82% <70.58%> (-0.04%)`	⬇️
#unittests	`66.34% <50.98%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
assets/src/edit-story/app/font/defaultFonts.js	`100.00% <ø> (ø)`
...src/edit-story/components/panels/textStyle/font.js	`100.00% <ø> (ø)`
...c/edit-story/elements/text/updateForResizeEvent.js	`62.50% <0.00%> (-8.93%)`	⬇️
assets/src/edit-story/utils/textMeasurements.js	`67.39% <11.11%> (-4.71%)`	⬇️
...onents/canvas/singleSelectionMoveable/useResize.js	`87.87% <50.00%> (-1.19%)`	⬇️
assets/src/edit-story/elements/text/output.js	`91.89% <60.00%> (-5.08%)`	⬇️
assets/src/edit-story/elements/text/edit.js	`93.47% <62.50%> (-3.04%)`	⬇️
assets/src/edit-story/elements/shared/index.js	`100.00% <100.00%> (ø)`
assets/src/edit-story/elements/text/display.js	`90.90% <100.00%> (+4.24%)`	⬆️
assets/src/edit-story/elements/text/frame.js	`97.50% <100.00%> (ø)`
... and 26 more

assets/src/edit-story/utils/textMeasurements.js

assets/src/edit-story/elements/text/karma/edit.karma.js

miina · 2020-09-09T13:03:37Z

Shouldn't previously existing elements get updated to the new logic when they're changed? Currently, this can happen:

One of the elements still has the space between the frame and the text -- line-height was set before (doesn't update even if changing the line-height again later). The other element has it updated -- line-height and the font was set afterward. Thoughts?

merapi · 2020-09-09T13:40:47Z

Shouldn't previously existing elements get updated to the new logic when they're changed? Currently, this can happen:

One of the elements still has the space between the frame and the text -- line-height was set before (doesn't update even if changing the line-height again later). The other element has it updated -- line-height and the font was set afterward. Thoughts?

Migration possibilities were briefly discussed here - PTAL.

miina · 2020-09-09T13:55:25Z

Migration possibilities were briefly discussed here - PTAL.

Ah, alright -- didn't realize that re-selecting the font family was the accepted solution -- thanks!

merapi · 2020-09-10T12:57:22Z

@csossi We still have an issue with some extra space at the bottom that disappears (corrects itself) when entering edit mode (it rarely happens), but there is so much more to test, so I'm handing that to you (and I'm doing the fix in the meantime).

First prototype

22f8de9

google-cla bot added the cla: yes label Aug 26, 2020

merapi added 7 commits August 27, 2020 12:46

Shorter css rule

5b8df21

Fix output offset

5907e6f

Fix small canvas

29176f3

Solution for resize event on small/big canvas

aa2ec4c

Edit mode resize fixes for small canvas

bcdc381

Fix big text after insertion (not pure function)

9e24d6a

Remove debug window

8d0153c

barklund reviewed Sep 3, 2020

View reviewed changes

assets/src/edit-story/components/canvas/singleSelectionMoveable/useResize.js Outdated Show resolved Hide resolved

assets/src/edit-story/elements/text/edit.js Outdated Show resolved Hide resolved

assets/src/edit-story/elements/text/edit.js Show resolved Hide resolved

merapi self-assigned this Sep 3, 2020

merapi linked an issue Sep 4, 2020 that may be closed by this pull request

Text highlight vertically centers differently inside in editor versus preview #2788

Closed

Remove border-radius from output

4a12d9e

Don’t crop anything if no font metrics are available

e8feda8

Attempt to fix the weird issue

4a0340b

Better resizing from corner

0d8a8ea

swissspidy added Elements: Text Type: Enhancement New feature or improvement of an existing feature Group: Fonts labels Sep 9, 2020

merapi added 5 commits September 9, 2020 11:03

Apply comments

1c30ea9

Merge branch 'main' into try/text-cropping

a943908

# Conflicts: # assets/src/edit-story/elements/text/display.js

Fixing tests

89863e4

Fix nested p tags

c77cb40

Eslint fix

c252e66

merapi added 2 commits September 9, 2020 13:50

Fix tests

b468385

Fix tests

01871fa

merapi marked this pull request as ready for review September 9, 2020 12:28

merapi requested review from barklund and miina September 9, 2020 12:32

miina reviewed Sep 9, 2020

View reviewed changes

assets/src/edit-story/utils/textMeasurements.js Show resolved Hide resolved

miina reviewed Sep 9, 2020

View reviewed changes

assets/src/edit-story/elements/text/karma/edit.karma.js Outdated Show resolved Hide resolved

Switch to toBeCloseTo

2ff3e2b

Add comment about font metrics math

f150c81

miina approved these changes Sep 10, 2020

View reviewed changes

swissspidy merged commit fdaf4a7 into main Sep 10, 2020

swissspidy deleted the try/text-cropping branch September 10, 2020 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crop leading & trailing whitespace #4247

Crop leading & trailing whitespace #4247

merapi commented Aug 26, 2020 •

edited

github-actions bot commented Aug 26, 2020 •

edited

barklund left a comment

miina commented Sep 3, 2020

merapi commented Sep 7, 2020 •

edited

miina commented Sep 7, 2020 •

edited

merapi commented Sep 7, 2020

merapi commented Sep 7, 2020

codecov bot commented Sep 9, 2020 •

edited

miina commented Sep 9, 2020 •

edited

merapi commented Sep 9, 2020

miina commented Sep 9, 2020

merapi commented Sep 10, 2020 •

edited

Crop leading & trailing whitespace #4247

Crop leading & trailing whitespace #4247

Conversation

merapi commented Aug 26, 2020 • edited

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

github-actions bot commented Aug 26, 2020 • edited

barklund left a comment

Choose a reason for hiding this comment

miina commented Sep 3, 2020

merapi commented Sep 7, 2020 • edited

miina commented Sep 7, 2020 • edited

merapi commented Sep 7, 2020

merapi commented Sep 7, 2020

codecov bot commented Sep 9, 2020 • edited

Codecov Report

miina commented Sep 9, 2020 • edited

merapi commented Sep 9, 2020

miina commented Sep 9, 2020

merapi commented Sep 10, 2020 • edited

merapi commented Aug 26, 2020 •

edited

github-actions bot commented Aug 26, 2020 •

edited

merapi commented Sep 7, 2020 •

edited

miina commented Sep 7, 2020 •

edited

codecov bot commented Sep 9, 2020 •

edited

miina commented Sep 9, 2020 •

edited

merapi commented Sep 10, 2020 •

edited