login: Split alert into title/description #21130

martinpitt · 2024-10-17T12:02:24Z

This makes it conformant to the current PF design.

Split the host and login alerts accordingly. Give some additional instruction for no-cockpit.

martinpitt · 2024-10-17T12:05:40Z

First version -- the spacing in the alert is all wrong. For an alert with a message, like trying to connect to host "nothing.local" (and user name a, empty password is ok) it has too much gap and margin (even though I already set them to 0):

And for a message without details, like trying to log in without an username, the message bit is visible as space. I think that's fixed easily enough with hiding it if it's empty, unless we can do that with CSS:

@garrett I'm happy to spend some more work on the CSS, unless you already have some hints. I also need to adjust quite a number of tests, so doing a first run to see which ones fail now.

@garrett 's initial design:

garrett

I'm surprised at PF's implementation. I explained why. But anyway, I made some simple grid CSS that would support an optional description (if the element is there) that should be mostly a drop-in replacement, and provided a link to the demo. (We might need to remove a little existing CSS to make it fit, but it's probably mostly fine?)

Hopefully this'll fix the spacing issues.

garrett · 2024-10-17T12:19:25Z

pkg/static/login.html

+      <p id="login-error-title" class="pf-v5-c-alert__title"></p>
+      <div class="pf-v5-c-alert__description">
+        <p id="login-error-message">
+          <span class="noscript" translate="yes">Please enable JavaScript to use the Web Console.</span>
+        </p>
+      </div>


This is incorrect. The heading should be a heading, not a paragraph. And it We shouldn't have an empty <p> (unless it's set to display: none while it doesn't have content).

For example: http://web-accessibility.carnegiemuseums.org/code/alerts/

What I had in mind is almost exactly what the US Gov't has (but with different class names) @ https://designsystem.digital.gov/components/alert/ for the error state, with a heading and role and everything.

<div class="usa-alert usa-alert--error" role="alert"> <div class="usa-alert__body"> <h4 class="usa-alert__heading">Error status</h4> <p class="usa-alert__text"> Lorem ipsum dolor sit amet, <a class="usa-link" href="javascript:void(0);">consectetur adipiscing</a> elit, sed do eiusmod. </p> </div> </div>

Oh, I made the mistake at looking at PatternFly's HTML, and they do use a paragraph tag. This is quite a bit wrong and I'm somewhat upset.

This is what PF does:

<div class="pf-v5-c-alert pf-m-danger pf-m-inline" aria-label="Inline danger alert" > <div class="pf-v5-c-alert__icon"> <i class="fas fa-fw fa-exclamation-circle" aria-hidden="true"></i> </div> <p class="pf-v5-c-alert__title"> <span class="pf-v5-screen-reader">Danger alert:</span> Danger inline alert title </p> </div>

It's not using a heading and it's not using ARIA or a role. I would've expected a heading (although I guess that's kept out of generic components as headings need to keep context based on siblings and parents and such), a role="alert", or some kind of aria like aria-live="assertive. This has none of that. It does have some parts that are specifically read for screenreaders (using a PF custom class for that), so I would hope they considered accessibility here.

In other words, I guess we "want" something incorrect? ☹️

As for layout, I would expect a block with this structure

alert

icon

title

description (optional)

Then a CSS grid like this:

.pf-v5-c-alert { display: grid; grid-template: "icon content" / auto 1fr; gap: 0.5rem 1rem; align-items: center; } .pf-v5-c-alert__title, .pf-v5-c-alert__description { grid-column: content; }

It basically sets up a grid to have two columns, where the first is automatically sized and the other column is sized to the rest of the space.

Details: The icon goes into the first available grid cell, which is sized to auto and is called "icon" (which doesn't matter really, as we're not referring to it otherwise; it's implicitly placed — we could explicitly tell it to go there, but it's fine as-is as long as it's the first child of the grid anyway). Then the title and description are told to go into the "content" column, which is the remainder space. Since there are only two columns, the third child of the grid is on another row (and would go to the icon column, except we told it to go to the content column instead). The gap adds space between everything (both vertical and horizontal gaps as shorthand), and that vertical gap it makes the description optional, as it's unused if there's no second row with the description element.

You can see a full demo @ https://codepen.io/garrett/pen/YzmVmdK

(This uses the default PF5 HTML for the alert, not adjusted or anything, so it should theoretically be mostly a drop-in to our CSS.)

martinpitt · 2024-10-21T09:14:19Z

Next round. I incorporated @garrett 's design/demo, and adjusted it a bit (actual PF colors and 2px instead of 3px top border width). The login failure alerts look quite fine now:

However, the icon is now centered with the description (or title) if it has multiple lines. Easily seen with setting

printf '[Session]\nBanner = /tmp/banner\n' | sudo tee -a /etc/cockpit/cockpit.conf
printf 'Hello from my laptop\nsecond line\n' > /tmp/banner
sudo systemctl stop cockpit
# so that cockpit can read /tmp/banner
sudo setenforce 0

On main:

This PR:

This also affects the changed host key alert in the same way.

I'll look at that next. But I wanted to do a full round of tests in between, as the previous round still had a failure on arch, and I changed some DOM structure.

garrett · 2024-10-22T09:03:21Z

However, the icon is now centered with the description (or title) if it has multiple lines. Easily seen with setting

Whoops! My fault. I think the vertical spacing is incorrect too. But these things should be easy to fix.

Reference screenshot from https://www.patternfly.org/components/alert/html/#inline-variations (yeah, it's success state instead of error, but it's inline with a description):

And for the long title, from https://www.patternfly.org/components/alert/html/#variations:

(The alignment isn't right in PF5 either, but it's closer.)

garrett · 2024-10-22T09:22:51Z

Changes that should fix the icon alignment:

.pf-v5-c-alert {
  align-items: start;
}

.pf-v5-c-alert__icon {
  margin-top: calc((1cap - 1ex) / -2);
}

https://blog.kizu.dev/cap-height-align/

cap is "relatively" "new"; this says to take the difference between the capital height and adjust it accordingly, and it's / 2 for just the top part (not bottom as well) and negative to make it negative margin upward
https://blog.kizu.dev/cap-height-align/ talks about the theory, but it's using vertical align
we're in a grid, so I shifted it up by half the difference, which isn't mentioned here, but it's the same principle overall

martinpitt · 2024-10-22T09:35:23Z

pkg/static/login.js

-                // always show what's going on
-                let message = format(_("A compatible version of Cockpit is not installed on $0."), login_machine || "localhost");
-                // in beiboot mode we get some more info
-                const error = JSON.parse(xhr.responseText);
-                if (error.supported)
-                    message += " " + format(_("This is only supported for $0 on the target machine."), error.supported);
-                login_failure(message);
+                login_failure(
+                    _("Cockpit not installed on remote host"),
+                    format(_("Install at least the cockpit-system package on $0 to connect."), login_machine || "localhost")


@garrett: I haven't quite made up my mind between "tell the user which remote OSes are supported" and "tell the user how to fix it" (install cockpit-system). "you can remotely connect to Fedora 40, Debian testing, and RHEL 9.6" seems "nice" at some level, but not in this particular situation: you want to connect to the given machine, not a random one which happens to have a compatible OS.

So I think like this it's more helpful. WDYT?

martinpitt · 2024-10-22T13:06:36Z

@garrett With your original formula, it looks oddly off:

Is your intent to align them at the bottom line, not at the top?

when I turn the -2 into 2 to move the icon down instead of up (which feels a bit nicer to me), it looks like this:

garrett · 2024-10-22T13:57:00Z

OK, that's weird.

I guess something's different in the sandbox vs. the live environment. The gap is also wrong weirdly. I'll need to look more.

The basic approach still looks fine, but the details need some adapting/tuning apparently. (The problem of in vitro vs. in vivo. My CSS was working fine in the "petri dish", but clearly not here in context.)

This makes it conformant to the current PF design. Split the host and login alerts accordingly. Give some additional instruction for `no-cockpit`. Bring the DOM structure closer to current PF5's alert, and adjust/simplify CSS accordingly. Thanks Garrett for working this out! Also tighten the error message checks in the tests, in particular for check-ws-bastion. Rework the "no-cockpit" message: Telling the user which OSes are supported is nice at some level, but doesn't help operationally when they want to connect to *that* server. Rather, suggest to install cockpit-system, which is the recommended way out of this error.

martinpitt · 2024-10-22T14:12:43Z

@garrett I pushed your suggestion for now (plus test adjustments). The red login errors are straightforward to reproduce. See #21130 (comment) if you also want to see the blue banner (do all this in a test VM preferably)

cockpituous · 2024-10-22T15:05:23Z

pkg/static/login.js

            /* OAuth failures are always fatal */
            if (oauth) {
-                fatal(msg);
+                fatal(title);
