cockpit icon indicating copy to clipboard operation
cockpit copied to clipboard

shell: Add basic language RTL support

Open garrett opened this issue 4 years ago • 25 comments

Addresses #14058

Much more work is needed to fix the UI everywhere, but this is a start.

garrett avatar May 19 '20 07:05 garrett

Ideally, we'd want to have the RTL check in a function that both places used, but I wanted to get a very simple proof of concept started.

There are a bunch of issues all over Cockpit, mainly float-related (we shouldn't be using floats for layouts anymore anyway) and margins (one of the many reasons why it's better to use grid/flex and gap).

Additionally, some layouts are flipped that shouldn't be, but, until now, we didn't have a way to adjust for them in the CSS. (Nor did we have a reason to, without any RTL support, including RTL languages.)

For CSS, we can branch layout decisions with

[dir=ltr] .foo {
margin-right: 1rem;
}

[dir=rtl] .foo {
margin-left: 1rem;
}

Inside of SCSS, we can do things like:

.foo {
  [dir=ltr] & {
    margin-right: 1rem;
  }
  [dir=rtl] & {
    margin-left: 1rem;
  }
}

Which could help with keeping blocks together.

Not everything would need both LTR and RTL — many things "just work". And sometimes, we can get away with just having the RTL support to override the LTR one.

Additionally, our supported browsers all support unset too, so we could do:

.foo {
  margin-right: 1rem;
  [dir=rtl] & {
    margin-right: unset;
    margin-left: 1rem;
  }
}

(Or just set the margin, either via shorthand or by giving it a value... or consider flex or grid.)

Obviously, there's a lot more than setting margins too. :wink: This is just one very probable example.

We may want to also get rid of float: left and (especially) float: right, or at least evaluate their usage. (They also can easily break accessibility and are at least somewhat fragile.)

garrett avatar May 19 '20 07:05 garrett

@yarons: I hope this is a good start. Are you able to check out this branch and compile Cockpit locally to check things out? We have a guide @ https://cockpit-project.org/external/source/HACKING

garrett avatar May 19 '20 07:05 garrett

I changed var to let in the two places lang_dir is initialized. (I didn't change any of the other variables that are set in the same code blocks.)

As I assumed and is stated above, this is probably not the proper place within Cockpit to change the language direction. It's just a starter so we can see RTL in action. (And hopefully others will help fix this more properly.) Basically: this PR is using The Stone Soup model. :grin:

garrett avatar May 19 '20 10:05 garrett

I've added some login support as requested. It's also quite basic. I've used var instead of let, as the login page needs to support old browsers, such as Internet Explorer (minimally to tell them they're not supported). Throwing a JavaScript error would prevent that.

The JS modifications I've made work enough, but should really be replaced by someone else. :wink:

garrett avatar May 19 '20 11:05 garrett

Hey guys, sorry I'm not on it for the past couple of days but I'm still struggling to create a working dev environment using docker on mac, sadly this is the only computer I have available right now.

yarons avatar May 20 '20 21:05 yarons

@yarons: I was talking with @andreasn about this PR last week and suggested we set up a VM for testing, with this branch compiled in. Hopefully we can get something together soon for you (and others) to test, to start filing issues.

garrett avatar May 25 '20 12:05 garrett

@garrett this is awesome, I've opened a bug (#14128) regarding the Vagrant problem but nothing happened yet.

yarons avatar May 25 '20 13:05 yarons

@yarons: right, sorry -- none of us uses vagrant, we have more convenient and efficient ways to do development. (Personally I never got vagrant to work properly either, with cockpit or otherwise, so I never bothered)

martinpitt avatar May 25 '20 13:05 martinpitt

Nah, that's fine, VM is much better, what is the deployment policy? Is there a restricted branch with autopulling?

yarons avatar May 25 '20 14:05 yarons

@yarons: We don't do any auto-deployment, as Cockpit isn't a real web service. This is how to build a VM from a branch:

export TEST_OS=fedora-32
git clone -b language-rtl https://github.com/garrett/cockpit.git
cd cockpit
tools/make-bots
test/image-prepare $TEST_OS
bots/vm-run -s cockpit.socket $TEST_OS

and then point your browser to http://127.0.0.2:9091 .

You need the build and test dependencies installed for that.

For $TEST_OS you can also use "debian-stable" or "ubuntu-2004" or "centos-8-stream" depending on your preferences, but it pretty well doesn't matter for this purpose.

martinpitt avatar Jul 03 '20 06:07 martinpitt

yaron@machine:~/workspace/cockpit$ ./tools/make-bots
info: Could not add alternate for '/home/yaron/.cache/cockpit-project/bots': path '/home/yaron/.cache/cockpit-project/bots' does not exist
checked out bots/ ref d5f63f539aad2ba0a9622223b43dc06cf7cf259f
yaron@machine:~/workspace/cockpit$ ./test/image-prepare $TEST_OS
Traceback (most recent call last):
  File "./test/image-prepare", line 212, in <module>
    sys.exit(main())
  File "./test/image-prepare", line 74, in main
    results = os.path.join(testvm.get_temp_dir(), "build-results")
AttributeError: module 'testvm' has no attribute 'get_temp_dir'

Well, that's not good either :(

yarons avatar Jul 08 '20 14:07 yarons

@yarons : Ah, this branch is rather stale -- it needs to be rebased to current master.

martinpitt avatar Jul 09 '20 12:07 martinpitt

Rebased to master.

@yarons: I hope this compiles better for you now!

garrett avatar Jul 09 '20 12:07 garrett

Rebased to current code & force-pushed

garrett avatar Jul 30 '20 08:07 garrett

Rebased to master.

  1. This still needs someone (@martinpitt, @marusak, etc.) to pick it up and polish the code to be better than my little hack. :wink:
  2. And then we'll need assistance from a native speaker (and reader) of Hebrew to help us find and fix the issues. (This can already be done with my hacky implementation.)
  3. And I'll probably need to go in and fix various CSS related issues too.

garrett avatar Sep 07 '20 15:09 garrett

To make it clear on how to use this (as I think it might not be clear enough from the above discussion):

  1. compile this branch based on the instructions at https://cockpit-project.org/external/source/HACKING
  2. visit Cockpit in your browser (probably at https://localhost:9090/)
  3. sign in
  4. switch the display language to Hebrew

The dialog looks like this when you're switching:

Screenshot_2020-09-07 Virtual Machines - garrett Rain

(It would be ideal if we could have a test VM already compiled and set up somewhere for anyone who is a native Hebrew speaker and wants to volunteer to help us fix up RTL, so they wouldn't have to compile Cockpit.)

garrett avatar Sep 07 '20 15:09 garrett

I have an Ubuntu VM running Cockpit from the repos, what can I do to use the relevant revision with Hebrew?

yarons avatar Sep 07 '20 20:09 yarons

@yarons: I've compiled Cockpit for Ubuntu 20.04 and have included the (non-debug) debs in a zip (to upload here on GitHub) for you to test in your VM:

🡇 ubuntu-20.04-24a587ffb.zip (23.8 MB)

Hopefully this helps! Please let us know here.

garrett avatar Sep 08 '20 08:09 garrett

I've rebased and rebuild the packages, for both Ubuntu and Fedora:

🡇 cockpit-ubuntu-20.04-gfd67bebaf.zip (18.7 MB) 🡇 cockpit-fedora-33-gfd67bebaf.zip (20.9 MB)

These packages are this branch, rebased on the current code we have in git as of today.

garrett avatar Nov 12 '20 10:11 garrett

OK, there are some glitches.

Screen Shot 2020-11-15 at 1 11 06 This is the user menu, the rest of it is out of the screen so I can't see it, I can only see half the menu.
Screen Shot 2020-11-15 at 1 10 01 I'm not sure this is the correct position for the buttons, they are too close to the label.
Screen Shot 2020-11-15 at 1 09 52 This is a mess, the arrows are pointing at each other and I'm not sure about their position.
Screen Shot 2020-11-15 at 1 09 41 The layout is a total mess, the edit is too close to the machine name and all the labels are reversed in terms of RTL.
Screen Shot 2020-11-15 at 1 09 23 The question mark button doesn't seem to be in place.
Screen Shot 2020-11-15 at 1 09 08 The warning symbols are cut in half, the hamburger menu is not in place, the arrows again.
Screen Shot 2020-11-15 at 1 08 19 It looks a bit misaligned.
Screen Shot 2020-11-15 at 1 08 05 The overall alignment is bad, from the direction of the labels to the warning symbol to the header, it all just looks incomplete.
Screen Shot 2020-11-15 at 1 07 53 Downwards arrow is misaligned. --- Sorry for the mess with the comments and the images, I hope it's clear enough.

yarons avatar Nov 14 '20 23:11 yarons

Rebased.

However, PF4 doesn't support RTL yet, so it might be difficult for us to do so properly.

garrett avatar Jan 19 '21 13:01 garrett

Rebased, just to keep this up-to-date.

garrett avatar May 04 '21 10:05 garrett

Rebased again, just to keep the branch from going stale.

garrett avatar Aug 17 '21 09:08 garrett

Rebased so this doesn't get too stale.

garrett avatar Feb 03 '22 14:02 garrett

Rebased again, to prevent staleness. Thankfully it still cleanly applies, so bringing it forward is easy.

garrett avatar Apr 20 '22 10:04 garrett

Rebased.

garrett avatar Oct 17 '22 16:10 garrett

Rebased again.

We still need:

  • The team to dedicate time for this.
  • As a team, fix the RTL layout problems. (It cannot just be me.)
  • Someone on the team to more properly implement the switching (and it shouldn't be just for Hebrew, although that's still OK for a first pass).
  • Someone to help with this who knows Hebrew and can spare the time during the process who can build this PR and test it on their machine. (This should be during the time that we work on this.)
  • Probably some PF fixes, where they're using margin instead of flex or grid (which is still a problem in PF for some odd reason, which also affects some layouts in LTR and mobile mode too). Even with some rough spots of RTL, we can generally fix a ton and make it mostly work, especially for a first official pass.

garrett avatar Nov 30 '22 11:11 garrett

I'll volunteer for the Hebrew testing, where can I find instructions for this? I'll prepare a VM for that.

yarons avatar Nov 30 '22 11:11 yarons

So Intl is not supported on firefox, so that is no go unfortunately. I though we could declare this in manifest or in cockpit but we don't have neither of these on login page. So I think using cookie will be the best approach as we already use it for the language. I set it up when the language is changed, so one corner case is that if someone is already using hebrew then the new version won't switch the order, they will have to force change the language. But honestly, doubt anyone is using it since it is wrong way round.

There are a lot of broken pieces. I would say, let's fix some obvious easy fixes and let's merge this and then do rest in followups.

marusak avatar Nov 30 '22 17:11 marusak

@marusak there's an open bug but it's pending for several other bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=1693576

yarons avatar Dec 01 '22 08:12 yarons