RTL stylesheets should be merged with LTR stylesheets

Bug #1681009 reported by Dan Scott on 2017-04-08
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned
2.12
Medium
Unassigned

Bug Description

In LP #1661661, the right-to-left stylesheet Open-ILS/src/templates/opac/css/style-rtl.css.tt2 was added as something that gets included in addition to the LTR style.css.tt2 when a RTL language is in play.

This approach isn't optimal, as browsers will have to interpret the rules from the first stylesheet and then reinterpret the rules that the RTL stylesheet overrides. It also sends more bytes over the network and involves one more network request. And it is likely that we will have more cases where the RTL styles are forgotten about if they are in a separate stylesheet.

Instead, I think we should be able to handle the RTL logic inline in a single stylesheet using TT2 conditional logic.

Dan Scott (denials) wrote :

WIP at http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbs/lp1681009_rtl_style_merge - not finished yet and hasn't been tested at all, but I believe it's worthwhile to share as it took about an hour to get this far and it would be unfortunate if someone else duplicated that effort.

tags: added: i18n opac
Kathy Lussier (klussier) on 2017-04-08
summary: - RTL spreadsheets should be merged with LTR spreadsheets
+ RTL stylesheets should be merged with LTR stylesheets
Ben Shum (bshum) wrote :

With some drift coming out of changes to the CSS in master, I rebased dbs' change first, and then finished up the remainder of the changes for merging the two files.

Top two commits in working branch: user/bshum/lp1681009_rtl_style_merge

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/bshum/lp1681009_rtl_style_merge

Changed in evergreen:
milestone: none → 3.0-alpha
status: New → Triaged
importance: Undecided → Wishlist
Ben Shum (bshum) wrote :

Applied this to a test server with the following resulting issue...

The RTL change is not taking effect immediately. On inspecting the style.css that gets included with the page, it is not updating to switch to using RTL values until after you perform a refresh of the style.css page itself. Once that happens, and then you refresh the catalog, it updates and begins performing as expected in RTL fashion. But changing back to LTR also does not work without a refresh on the style.css page directly.

Might be a caching issue of some sort? And we need to force it to update the CSS more frequently?

Dan Scott (denials) wrote :

I suspect that 'ExpiresByType text/css "access plus 50 minutes"' from eg.conf is the issue, because CSS normally doesn't change very often. That cache time should be more like 500 days, but we need to merge the cache-busting bug # 1681095 first.

For this branch, we could just append "?dir=rtl" or "?dir=ltr" to the style.css URL in src/templates/opac/parts/base.tt2

So the base.tt2 line would become something like:

<link rel="stylesheet" type="text/css" href="[% ctx.opac_root %]/css/style.css?dir=[%
  IF ctx.get_i18n_l(ctx.eg_locale).rtl == 't'
 %]rtl[%
 ELSE
 %]ltr[% END %]"

Then when the cache-busting branch is merged, we append the extra cache-busting GET param accordingly.

Dan Scott (denials) wrote :

Okay, I've force-pushed an updated branch to http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbs/lp1681009_rtl_style_merge that includes a sign-off of Ben's last commit and an extra commit with the required base.tt2 cache-busting GET param to make switching between locales with different directions actually work. Thanks Ben!

Ben Shum (bshum) wrote :

Tested Dan's fix for cache and works much better for me now. Adding my signoff to his last change here:
 user/bshum/lp1681009_rtl_style_merge_signoff

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/bshum/lp1681009_rtl_style_merge_signoff

In the interest of not holding up further work on CSS changes that are pending in other bugs/branches (there's at least two I can think of?), I'd like to recommend that we merge what we have for this combining of RTL and LTR for the main style.css as soon as possible and work on subsequent CSS merging for semiauto-css in a new bug to be filed. Adding pullrequest at this time.

tags: added: pullrequest signedoff
Galen Charlton (gmc) on 2017-04-28
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
Galen Charlton (gmc) wrote :

Tested and pushed to master. Thanks, Dan and Ben!

I've also pushed a release notes entry with an upgrade note for users of RTL locales.

Changed in evergreen:
status: Triaged → Fix Committed
assignee: Galen Charlton (gmc) → nobody
Kathy Lussier (klussier) wrote :

Looks good on 2.12 too. After brief discussion in IRC about backporting the branch, I've merged to release 2.12. Thanks Ben, Dan and Galen!

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers