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

Text: Unexpected line wraps in Safari with Fill mode #6323

Closed
swissspidy opened this issue Feb 11, 2021 · 19 comments · Fixed by #7938
Closed

Text: Unexpected line wraps in Safari with Fill mode #6323

swissspidy opened this issue Feb 11, 2021 · 19 comments · Fixed by #7938
Assignees
Labels
Browser Issues Elements: Text P2 Should do soon Type: Bug Something isn't working

Comments

@swissspidy
Copy link
Collaborator

swissspidy commented Feb 11, 2021

Bug Description

Original report: https://wordpress.org/support/topic/box-doesnt-wrap-text-correctly/
Story: https://www.acec.nl/web-stories/de-notenkrakerrrr/

There are unexpected line wraps on page 9 (the third to last one) in Safari, while the story displays correctly in Firefox and Chrome.

Expected Behaviour

No line wraps, same behavior everywhere.

Steps to Reproduce

  1. Add a text that has at least two words, e.g. "Mal maken" from the link reported.
  2. Set background mode to Fill
  3. Unlock the text element width/height and set the width so that it would be as small width as possible but still allow the 2 words to be displayed in one line.
  4. Save the story and open the story link to view it.
  5. Grab the bottom right corner of the browser and start resizing it slowly to smaller. At some point, the text will be displayed in 2 lines:
    Screenshot 2021-05-21 at 13 41 21
  6. Leave the same window size and go to the editor. In testing, it seems to display 2 lines in the editor now, too, perhaps this will be helpful when debugging.

Screenshots

Safari:
safari

Firefox:

firefox

Chrome:

chrome

Additional Context

  • Plugin Version: 1.3.0
  • WordPress Version:
  • Operating System:
  • Browser:

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

Acceptance Criteria

Implementation Brief

@swissspidy
Copy link
Collaborator Author

@barklund @miina Is this something one of you could look into? Do we have similar issues for this somewhere already? Or is it perhaps even an issue that we simply can't fix even with our font metrics?

@miina
Copy link
Contributor

miina commented Feb 12, 2021

@swissspidy I'm able to reproduce this at least in Safari, looks like it doesn't always appear, seems to depend on the window size (or ratio?). Is quite easy to reproduce when resizing the window. For example:
safari

@swissspidy
Copy link
Collaborator Author

Another case of unexpected wraps in Safari: https://adiabeticchef.com/web-stories/cakes-and-adc/

@miina
Copy link
Contributor

miina commented May 21, 2021

Huh, this is not a fun bug.

Moving it to the Sprint backlog for an initial investigation to figure out if we can fix this.

@miina miina added the P2 Should do soon label May 21, 2021
@miina miina self-assigned this May 26, 2021
@miina
Copy link
Contributor

miina commented May 26, 2021

Findings so far:
Initially, it seemed that perhaps Safari was not rendering fractional pixels, however, testing shows that it does seem to consider fractional pixels.

However, it looks like Safari is rendering the font a little bit differently than in chrome, it seems to be a tiny bit larger. Found some topics with similar issues and suggestions e.g. this and this and quite a few others, however, no solution so far.

Here are two screenshots, one Chrome and one Safari with exactly the same Page size. All the numbers (width/height, fontSize, etc.) match 100%, however, in case of Safari, the font seems to be larger (e.g. the word maken seems to take 1px more in Safari than Chrome according to some measuring).
Screenshot 2021-05-26 at 15 02 38
Screenshot 2021-05-26 at 15 02 45

Thoughts? @merapi You worked on the fonts quite a lot, perhaps you have any insights here?

@miina
Copy link
Contributor

miina commented May 26, 2021

Information from Pascal:

Apparently this is a long-standing issue with Safari where fractional font-sizes are not correctly handled: https://bugs.webkit.org/show_bug.cgi?id=46987

Should we close this issue as non-fixable?

@swissspidy
Copy link
Collaborator Author

@choumx Any thoughts on this one?

With Safari having issues with fractional font sizes (basically rounding up/down to nearest 0.5/1.0), there's not much we can do here without relying on JS to detect the wrapping.

<amp-fit-text> comes to mind here, but we don't use that on purpose because it doesn't quite fit our use case.

So really seems like a wontfix unfortunately 😞

@dreamofabear
Copy link
Contributor

Interesting bug. I think we can try deliberately reducing font sizes only on Safari by 0.5px to undo this Webkit behavior:

See FontDescription::computedPixelSize()
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/FontDescription.h#L66
unsigned computedPixelSize() const { return unsigned(m_computedSize + 0.5f); }

This can cause the opposite problem (underflow) on some text fields, but it might be desirable tradeoff in terms of UX.

@swissspidy
Copy link
Collaborator Author

Any suggestions on how to target Safari on the frontend then?

@dreamofabear
Copy link
Contributor

Looks like there isn't a clean way to do this in pure CSS, so I'd try user agent detection in JS. E.g.

if (navigator.vendor.includes('Apple')) {
  // Render CSS rule into <style amp-custom> to reduce fontSize in children of text elements, e.g.
  // p > span { font-size: calc(100% - 0.5px); }
}

@swissspidy
Copy link
Collaborator Author

We'd need to do this on the frontend though, not in the editor. So we can't just use JS, and I don't think amp-script works in stories.

@dreamofabear
Copy link
Contributor

Oh right. Can we do UA detection from the request header i.e. look for Safari/xyz? Or we can try the Safari CSS hack:

@media not all and (min-resolution:.001dpcm) { @media {
  p > span { font-size: calc(100% - 0.5px); }
}}

Seems to work: https://codepen.io/choumx/pen/dyvdqdV 🥴

@miina
Copy link
Contributor

miina commented Jun 3, 2021

Tested it quickly and it does seem to work, however, it does (sometimes) cause the issue mentioned above: underflow in Safari.

An example, editor vs FE in Safari.
Screenshot 2021-06-03 at 15 18 43
Screenshot 2021-06-03 at 15 18 36

This change would be for new stories only and if Safari would change their logic in the future then our fix would still be active for the previously saved stories.

Let me know if you think that's a better issue than overflow.

@dreamofabear
Copy link
Contributor

Thanks Miina! I think it looks better since at least it avoids text legibility issues due to overflow.

We could also consider only supporting integer (pixel) font sizes in the editor. 😱

Let's run this by UX and product and see what they think.

@miina
Copy link
Contributor

miina commented Jun 3, 2021

We could also consider only supporting integer (pixel) font sizes in the editor.

The font sizes are responsive in the FE and even if set in full pixels in the editor, it would still vary when viewing a story.

@dreamofabear
Copy link
Contributor

Yea sorry I meant applying font-size rounding to all stories in the FE, which would make text more deterministic across browsers but less deterministic between editor and FE. Not as excited about this.

@miina
Copy link
Contributor

miina commented Jun 10, 2021

@choumx Any updates on this / next steps?

On applying font-size rounding to all stories in the FE: do you mean rounding font size down for FE? The width and height are also responsive and in %, font-size would need to be in sync with that, too. I'd guess that rounding any values could cause unexpected results.

@dreamofabear
Copy link
Contributor

Confirmed with UX that the hack to reduce font-size by 0.5px in Safari is OK.

@miina miina mentioned this issue Jun 11, 2021
8 tasks
@csossi
Copy link

csossi commented Jun 21, 2021

Verified in QA

@csossi csossi removed their assignment Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Elements: Text P2 Should do soon Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants