cockpit-project / cockpit

Cockpit is a web-based graphical interface for servers.
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
11.23k stars 1.11k forks source link

shell: Modals are broken in RTL #20553

Open garrett opened 5 months ago

garrett commented 5 months ago
  1. Change your language to Hebrew.
  2. Try to change it back.

Language modal:

Screen Shot 2024-06-05 at 15 40 11

About modal:

Screen Shot 2024-06-05 at 15 41 24

SSH key modal:

Screen Shot 2024-06-05 at 15 42 06

garrett commented 5 months ago

In-page modals seem fine, however:

Screen Shot 2024-06-05 at 15 43 23

Screen Shot 2024-06-05 at 15 43 54

So this seems shell-specific.

garrett commented 5 months ago

There's some specific text that's way off the page to the right, which causes the width of the pf-v5-c-backdrop to be much wider than the viewport, which causes this offset problem.

image

I added pointer-events: none to the backdrop and the shim and found out that text is "דילוג על הניווט הראשי", which is the skip link text.

image

The classes are screenreader-text skiplink desktop_v and its ID is #hosts-sel (for some reason).

garrett commented 5 months ago

Ah, the problem is this:

image

It uses negative margins, and since the right/left was changed to inline-start/inline-end, it must've changed the intend and is stretched in both ways instead of one. The intent is to visually hide it, but this makes it go way out of bounds.

garrett commented 5 months ago
  1. Why are we implementing our own .screenreader-text instead of using https://www.patternfly.org/utility-classes/accessibility/#screen-reader-only? (Answer: We have a focus style to show it too.)

  2. Also, more importantly, why are we implementing our own skip link instead of using PatternFly's? https://www.patternfly.org/components/skip-to-content

To fix this properly, I strongly suggest removing our own custom thing and using PatternFly's skip link instead.

garrett commented 5 months ago

Also: Why did our pixel tests not pick this up at all? I thought we tested these modals and on RTL as well?

garrett commented 5 months ago

This breaks basic functionality in Cockpit (as it moves content offscreen, including switching languages), and it should be simple to fix, so I'm marking it as a release blocker.

martinpitt commented 5 months ago

We do have at least one RTL shell dialog pixel test. I tried this with your recipe (Hebrew) on current Cockpit main (8769d417e5a4) in both firefox-125.0.2-1.fc40.x86_64 and chromium-125.0.6422.141-1.fc40.x86_64. I also changed the window width from ~ 1/4 screen to full screen width continuously, and the dialog keeps being perfectly centered and generally looks okay.

So this is a bit more subtle -- perhaps you have some extension, like a screenreader or so?

Both the skiplink and the screenreader-text stuff were introduced 4 years ago in commit 9e7209d154 (PR #13663) by @marusak. Do you perhaps still remember what all of this does? Why the -999, what is a skiplink, etc?

martinpitt commented 5 months ago

Plan:

garrett commented 4 months ago

I can't reproduce the issues now. We will probably still want to replace it with the PF skiplink, but everything seems better now as of #20561.