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

Experiment with pure CSS alternative to TextTruncator for simpler use cases #8464

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Sep 24, 2021

Summary

Introduces TextTruncatorCss as a simpler pure CSS solution to our current JavaScript based TextTruncator. It is meant to be temporary or additional to the JS solution and for shorter texts like titles on cards.

Motivation

While working on the new home page in the hybrid learning project, I need to truncate cards titles to two lines. We have some performance issues with TextTruncator that are caused by ResizeSensor and shave.js where shave.js seems to be even bigger bottleneck than ResizeSensor. To avoid risking introducing similar problems to new pages, I used this task as an opportunity to play with some CSS alternatives that could be potentially used for simpler use cases, at least until we decide on what to do with TextTruncator issues.

Although my primary focus was on cards since there can be many instances of them on one page and risk of performance problems is thus higher, I also thought that having the CSS solution available might be useful for other simple use-cases like the one-line resource title in the learning activity bar and similar.

Solution

The component is built with a progressive enhancement approach, using three CSS truncation techniques. Please check the comments in the component for details.

TextTruncator and TextTruncatorCss comparison

Tested on a small sample of texts resembling cards titles in Firefox, Chrome, and IE11. I tried to locate differences and take screenshots of TextTruncatorCss results that looked the worst.

(1) One line

<TextTruncatorCss
  :maxLines="1"
  text="Idiopathic pulmonary fibrosis - causes, symptoms, diagnosis, treatment, pathology"
/>

<TextTruncator
  :maxHeight="23"
  text="Idiopathic pulmonary fibrosis - causes, symptoms, diagnosis, treatment, pathology"
/>

TextTruncator and TextTruncatorCss seem to behave identically in all Firefox, Chrome, and IE11.

230px-css-firefox,chrome,ie

(2) Multiple lines

<TextTruncatorCss
  :maxLines="2"
  text="Idiopathic pulmonary fibrosis - causes, symptoms, diagnosis, treatment, pathology"
/>

<TextTruncator
  :maxHeight="46"
  text="Idiopathic pulmonary fibrosis - causes, symptoms, diagnosis, treatment, pathology"
/>

Similar behavior in Firefox and Chrome, however TextTruncatorCss seems to truncate by word rather than by character more often than TextTruncator. TextTruncator works well in IE 11. TextTruncatorCss is giving bad results in IE11 since in some cases, there can be a gap between ellipsis dots and the last character of a text. Seems to be a major problem with the CSS-based solution.

Width 200px - Firefox, Chrome Width 230px - Firefox, Chrome Width 230px - IE11
TextTruncatorCss 200px-css-firefox,chrome 230px-css-firefox,chrome 230px-css-ie
TextTruncator 200px-js-firefox,chrome 230px-js-firefox,chrome 230px-js-ie

(3) Multiple lines - a long word with no breaks

<TextTruncatorCss
  :maxLines="2"
  text="Idiopathicpulmonaryfibrosiscausessymptomsdiagnosistreatmentpathology"
/>

<TextTruncator
  :maxHeight="46"
  text="Idiopathicpulmonaryfibrosiscausessymptomsdiagnosistreatmentpathology"
/>

TextTruncator doesn't respect height settings and only shows one line in all Firefox, Chrome, and IE11. However, I guess that it's just a bug rather than the JS solution limitation. TextTruncatorCss works well in Firefox and Chrome, but has the same problem like in (2) in IE11.

Firefox, Chrome IE11
TextTruncatorCss 03-css-firefox,chrome 03-css-ie
TextTruncator 03-js-firefox,chrome,ie 03-js-ie

References

Comments

There's an issue in IE11 with three ellipsis dots not showing in some situations. It seems to be a font issue. The problem disappears after unchecking this rule

.partial-fonts-loaded body, .partial-fonts-loaded button, .partial-fonts-loaded input, .partial-fonts-loaded select, .partial-fonts-loaded textarea {
	font-family: noto-subset,noto-common,sans-serif;
}

and falling back to sans-serif. I haven't included it in comparison because I saw it happen for both TextTruncator and TextTruncatorCss in some scenarios.

Reviewer guidance

I moved this logic from the home page branch to get some feedback from the team if this could be an acceptable way forward for us, so please review the advantages and disadvantages of both approaches for shorter texts. For more background on reasons to use TextTruncator, please see @indirectlylit's comment. An alternative solution for home page cards could be to use the existing TextTruncator. In that case, I'd suggest prioritizing refactoring of TextTruncator performance-wise.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob
Copy link
Member Author

MisRob commented Sep 24, 2021

Regarding a11y, I haven't tested it myself, but found some information here. For most techniques, it says that the whole text would be read. My assumption is that for shorter texts like resource titles, it wouldn't be a problem. @radina is that right?

@MisRob MisRob requested a review from radinamatic September 24, 2021 14:17
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! The CSS implementation degrades gracefully on IE11 and otherwise the behaviors are quite similar, so I would support migrating to it entirely rather than maintaining two parallel implementations.

Shorter-term the main question is cost/risk of migration – if either are high, we might want to use both at least until 0.15. Longer-term once things are consolidated, we could even move it to the design system as KTextTruncator.

Multiple lines - a long word with no breaks

The behavior for the CSS implementation looks good here. Flagging that this case has been a source of many bugs over the years, and warrants a careful eye:

There's an issue in IE11 with three ellipsis dots not showing in some situations. It seems to be a font issue.

Would you mind filing a separate issue for this? For some context, here is an overview of the intended behavior:

# Modern behavior
Newer browsers have full support for the unicode-range attribute of font-face
definitions, which allow the browser to download fonts as-needed based on the text
observed. This allows us to make _all_ font alphabets available, and ensures that
content will be rendered using the best font possible for all content, regardless
of selected app language.
# Basic behavior
Older browsers do not fully support the unicode-range attribute, and will eagerly
download all referenced fonts regardless of whether or not they are needed. This
would have an unacceptable performance impact. As an alternative, we provide
references to the full fonts for the user's currently-selected language, under the
assumption that most of the content they use will be in that language.

and the client-side font loading occurs here:

https://github.com/learningequality/kolibri/blob/6903afec0ca3ebdf65fbf0eb96ef5348ed9fc2d2/kolibri/core/assets/src/utils/setupAndLoadFonts.js

Regarding a11y ... for most techniques, it says that the whole text would be read.

I've always assumed that the best behavior would be to only read the visible text: if we've made a design decision to truncate it visually, that should probably apply to audible text too? That said, I don't think that either choice is "wrong" and both will result in a reasonable experience for all users.

If we wanted to get really fancy here, we could use aria-hidden="true" and make a separate off-screen copy of the text for screen readers and truncate that based on some metric like maxLen = maxLines * 50.

kolibri/core/assets/src/views/TextTruncatorCss.vue Outdated Show resolved Hide resolved
@marcellamaki
Copy link
Member

Hi @MisRob - thanks for working on this and layout out the comparison so clearly. I like the idea of moving towards a single option, and maybe even moving the component into KDS ultimately, as @indirectlylit suggests.

As to @indirectlylit's point about migration - right now, it looks like the original TextTruncator is only used in 5 files, and I think we could swap this out fairly easily. If we choose to do that, I think it would be good to merge this promptly and swap out the components right away, so there is plenty of time for QA (especially seeing the edge cases with long text), and then we can revert or adjust as needed. To me, this seems simpler and a better use of time than refactoring the existing TextTruncator separately, especially if we plan on replacing it in the near future, as long as no one finds the IE11 spacing issues to be a problem.

@radinamatic
Copy link
Member

I've always assumed that the best behavior would be to only read the visible text: if we've made a design decision to truncate it visually, that should probably apply to audible text too? That said, I don't think that either choice is "wrong" and both will result in a reasonable experience for all users.

I did not find a clear recommendation re. screenreader output full text vs. only the visible text for long card titles, but if we assume that the goal is to have comparable experience for all users, reading only the visible text should be kosher.

Text truncation is an issue for when the screenreader output is cut on some important actionable element like a button label, but I don't see it as a blocker for truncation of long sentences - all users can always click the card to open and read the full title sentence.

@MisRob MisRob mentioned this pull request Sep 29, 2021
9 tasks
@MisRob
Copy link
Member Author

MisRob commented Oct 1, 2021

Thank you everyone for your feedback. I've just pushed all updates so this should be ready for the final review and eventually merge.


Regarding replacing TextTruncator completely, originally I had three areas of concern:

(1) gap between the last word and ellipsis in IE - according to the feedback, doesn't seem to be a blocker

(2) wasn't sure about what should be proper accessibility behavior - reading the feedback here it seems to be quite open and most importantly, with @indirectlylit's suggestion :

we could use aria-hidden="true" and make a separate off-screen copy of the text for screen readers and truncate that based on some metric like maxLen = maxLines * 50

we should be able to tweak it as needed at any point in the future

(3) when compared to TextTruncator it doesn't have "View more/less" functionality - however, I checked all occurrences of TextTruncator and I didn't see TextTruncator's showViewMore prop actually being used. If we need it at some point, we can something similar to TextTruncatorCss.

All of these issues seem to be resolved so I feel fine about migrating completely. I don't know if we have enough time to switch it now since as @indirectlylit pointed out, there are relatively tricky edge cases we need to be cautious about. However even if we get to it, I'd suggest keeping TextTruncator component around because it has "View more/less" strings - just in case they might be handy before the next release (e.g. it looks that if we get to adding "View more/less" to the new TextTruncatorCss, it might be helpful in @nucleogenesis 's truncation in side panels)

@MisRob
Copy link
Member Author

MisRob commented Oct 1, 2021

@indirectlylit I am putting opening the font issue on my TODO's list - if that's fine I'd like to do it as soon as I open a PR with the finished home page that uses TextTruncatorCss for cards titles so I have simple and straightforward steps to reproduce for the issue (even though I saw it for the TextTruncator too it might not be relevant to report it when it seems we might stop using it soon and I discovered it only by using it temporarily in the new home page code where it won't be used so I wouldn't have good steps to reproduce right now)

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @MisRob!

@marcellamaki marcellamaki merged commit 97fd943 into learningequality:develop Oct 7, 2021
@indirectlylit
Copy link
Contributor

hooray!

@MisRob MisRob deleted the text-truncator-css branch October 20, 2021 08:58
@MisRob
Copy link
Member Author

MisRob commented Oct 25, 2021

I am putting opening the font issue on my TODO's list - if that's fine I'd like to do it as soon as I open a PR with the finished home page that uses TextTruncatorCss for cards titles so I have simple and straightforward steps to reproduce for the issue

@indirectlylit I wasn't able to reproduce this particular font issue in the home page PR anymore. There's another IE11 text ellipsis problem though that I'm reporting in the PR now.

@indirectlylit
Copy link
Contributor

indirectlylit commented Oct 25, 2021

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants