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

lib: Redesign On/Off switch #11769

Merged
merged 1 commit into from May 14, 2019

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented May 7, 2019

Update the On/Off switch redesign to how PatternFly 4's Switch looks
like [1]. This makes the button more accessible, easier to click (as the
full button area can be clicked, not just half of it), and drops all
translatable text, thereby avoiding the overflow in translations.

While at it, invert the "enabled" property into the standard "disabled"
one. As the PF4 design differs between switches with and without extra
text, add a new "text" property. This isn't being used anywhere except
the "React Patterns" page right now, but may be in the future.

CSS (the hard part) is all credit to Garrett LeSage.

[1] https://pf4.patternfly.org/components/Switch/

Fixes #9408
Fixes #11225
Fixes #11226
Fixes #11814
https://bugzilla.redhat.com/show_bug.cgi?id=1650942

TODO:

  • Port non-React instances over, drop on/off switches on the jquery patterns page: PR Move On/Off switches to React component #11773
  • Convert last remaining jQuery On/Off button on Networking page: PR networking: Convert switchbox() to React  #11784
  • Fix spacing on Firewall page
  • Port remaining enabled -> disabled properties from recently Reactified pages
  • Fix bad alignment at network interface actions (delete button and on/off)
  • Fix font size in buttons on <h1> at Firewall page
  • Adjust tests
  • Add aria annotations (?)
  • Keyboard focus
  • Focus style
  • Hover style
  • test on firefox desktop
  • test on firefox mobile
  • test on chromium desktop
  • test on chromium mobile
  • test on edge
  • test on Safari

@martinpitt
Copy link
Member Author

@garrett: I took your CodePen and turned it into an actual implementation. This works rather well already, except on the Firewall page. I made a bunch of TODOs in the description.

On the playground page it looks like this now:

onoff

onoff2

@martinpitt
Copy link
Member Author

martinpitt commented May 10, 2019

@garrett: For this TODO item "Fix font size in buttons on <h1> at Firewall page", which looks like this:

onoff-h1

I fixed the font size to its usual default:

--- a/pkg/lib/cockpit-components-onoff.less
+++ b/pkg/lib/cockpit-components-onoff.less
@@ -64,6 +64,7 @@ label.onoff-ct {
         &:last-child::before {
           content: "";
           font-family: FontAwesome;
+          font-size: 12px;
           color: var(--switch-dot);
           position: absolute;
           left: 6px;

This feels a bit unsatisfactory, though. Is there some better way, like "inherit-from-grandparent", or some "page-default" size? Or should we move the button out of the <h1> and add CSS to align them horizontally? I marked it with a "FIXME" in the current patch.

@martinpitt martinpitt force-pushed the on-off-redesign branch 2 times, most recently from 63a9667 to a349f13 Compare May 10, 2019 09:24
@martinpitt
Copy link
Member Author

I adjusted the majority of tests now, but not yet the networking ones -- we should land PR #11784 first.

@martinpitt
Copy link
Member Author

Rebased on top of #11784 and adjusted tests.

@martinpitt martinpitt changed the title WIP: lib: Redesign On/Off switch [no-test] WIP: lib: Redesign On/Off switch May 12, 2019
@martinpitt martinpitt force-pushed the on-off-redesign branch 2 times, most recently from f1f0f55 to b9151b8 Compare May 12, 2019 12:38
@martinpitt
Copy link
Member Author

Now all tests succeed, and the new switch is being used everywhere. What remains is some CSS fixups (mis-alignments mostly and the fixed font size issue above). Do we need to set any aria properties? (These could be a follow-up?)

@marusak
Copy link
Member

marusak commented May 13, 2019

I think that this following aria needs to be used:
role="switch"
aria-checked="true" for on state
aria-checked="false" for off state
and a label for each switch (hidden when we don't show it).

Also when navigating by keyboard it should be toggled by the space key.

And also when the switch is disabled, aria-readonly should be set to true otherwise set to false.

For more info see this doc

@garrett
Copy link
Member

garrett commented May 13, 2019

I fixed alignment and checked how it works across all of Cockpit (at least everywhere I found an onoff switch). I pushed up the change to this PR.

I think font size of 12px for the check icon is fine. I'd have a different opinion if it were hardcoded to a pixel size for general text... but this is an icon and we understand how it's used. I could rework it to be a CSS variable like the rest of the sizing, however. (That might be a little better.)

@garrett
Copy link
Member

garrett commented May 13, 2019

In addition to making sure there's better labeling support for screen readers (which might or might not include various uses of ARIA), the onoff switch should have keyboard navigatability and focus and hover states.

@garrett
Copy link
Member

garrett commented May 13, 2019

I've added focus indicators, but am unsure of the style. I have two different concepts in mind, but have pushed up the dotted ring version. It's easy to switch between the two styles — I should post screenshots... tomorrow. (It's already evening here.)

Also, I added a way to have clicked focus rings disappear, so only keyboard focus would activate the rings... but it's untested as no browser supports it yet. (All of them are supposed to support it "soon". 🙄)

Meanwhile, the focus rings are not shown when directly hovered with a mouse.

@garrett
Copy link
Member

garrett commented May 13, 2019

@marusak: The example you linked to needs those ARIA values. Our implementation does not, as we're piggybacking on a checkbox to do all the hard work.

That said, we need to make sure our switches are labeled with ARIA when they do not contain text labels. (Either that or we add some for-screen-reader-only text labels for the pure on/off states.)

@martinpitt martinpitt changed the title WIP: lib: Redesign On/Off switch WIP: lib: Redesign On/Off switch [no-test] May 13, 2019
@martinpitt
Copy link
Member Author

Nice, thanks @garrett! My gut feeling is that the focus line should be more dotted than dashed and have some margin around the actual button, to be a little more similar to other objects - but this is utter fine-tuning. Keyboard navigation works great now.

Since we talked about it, I did another fixup commit to show different descriptions on the React Patterns page for "on" and "off" states.

Settig no-test for now while we work on the styling.

@garrett
Copy link
Member

garrett commented May 14, 2019

My gut feeling is that the focus line should be more dotted than dashed and have some margin around the actual button, to be a little more similar to other objects - but this is utter fine-tuning.

I tried all of that. I don't think I can make the outline go outside of the widget much more than this, due to it being a border and not an outline. It has to be a border to conform to the shape, as the outline is rectangular. Also dotted border line looked really rough, as this widget is very curvy.

I also was experimenting with having the focus ring be on the inside of the dot. 🤷‍♂️

Regardless, this is all kind of tricky to get it to do what I want... and I still should check it cross browser. (It, at least, works in Firefox. And, at least, also worked at one point in Chrome too.)

@martinpitt
Copy link
Member Author

I dropped the "aria annotations" TODO, seems we don't need that after all. I added TODO items for browser tests.

@garrett
Copy link
Member

garrett commented May 14, 2019

Another focus style could be something like this:

Screenshot_2019-05-14 React Patterns - Sunny

I'm not sure if a 1 pixel wide stroke is enough; that's why I made it 2 here.


Another styling approach, as mentioned on IRC: I think we could ditch the ✔ icon (as an icon) inside the area and I could make a very simple SVG as a background to make a check in those cases. Then I could re-use that pseudo element to make an absolutely positioned (so it's outside of the page flow) focus ring.


Either way, it's all just CSS. The rest would stay the same.

@garrett
Copy link
Member

garrett commented May 14, 2019

Here's what keyboard focus states look like:

Screenshot_2019-05-14 React Patterns - Sunny(2)

Screenshot_2019-05-14 React Patterns - Sunny(3)

I made the check icon actually a background image. This let me rework the ::before into a focus ring that's absolutely positioned around the element. I have it offset by 2 pixels, as it looked better than touching the switch or offsetting just by 1 pixel.

When you click (or tap) on the switch, the focus ring doesn't show up. I did this with a hack that uses transition and transition-delay. Technically, the focus rings are there, but transparent... and they do show up, but 10 minutes after clicking on the item. 😁

Tabbing to the item with a keyboard makes the rings show up instantly.

Hiding the focus ring on click works everywhere but Edge. It also doesn't animate the position of the switch's knob. Oh well. 🤷‍♂️ Everything else about the switches work as expected on Edge, however. I consider it perfectly acceptable.

@garrett
Copy link
Member

garrett commented May 14, 2019

The new switches:

  1. Are clickable everywhere
  2. Automatically have a check icon when there's no label text
  3. Are focusable via keyboard
  4. Are activatable via keyboard
  5. Look similar to PF4 switches, but use PF3 colors (to match the rest of Cockpit right now)
  6. Work correctly in every browser
  7. Support text labelling
  8. Are friendlier for additional languages
  9. Are accessibility-friendly

(The old custom switches had none of the above list.)

@martinpitt martinpitt changed the title WIP: lib: Redesign On/Off switch [no-test] WIP: lib: Redesign On/Off switch May 14, 2019
@martinpitt martinpitt changed the title WIP: lib: Redesign On/Off switch lib: Redesign On/Off switch May 14, 2019
Update the On/Off switch redesign to how PatternFly 4's Switch looks
like [1]. This makes the button more accessible, easier to click (as the
full button area can be clicked, not just half of it), and drops all
translatable text, thereby avoiding the overflow in translations.

While at it, invert the "enabled" property into the standard "disabled"
one. This will also make migration to PF-React easier.

As the PF4 design differs between switches with and without extra text,
add a new "text" property. This isn't being used anywhere except the
"React Patterns" page right now, but may be in the future.

CSS (the hard part) is all credit to Garrett LeSage.

[1] https://pf4.patternfly.org/components/Switch/

Fixes cockpit-project#9408
Fixes cockpit-project#11225
Fixes cockpit-project#11226
Fixes cockpit-project#11814
https://bugzilla.redhat.com/show_bug.cgi?id=1650942
https://bugzilla.redhat.com/show_bug.cgi?id=1677593
@martinpitt
Copy link
Member Author

There was a leftover "FIXME" comment, I dropped that and squashed. All good now!

Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

Thanks for all your work on this!

@martinpitt
Copy link
Member Author

@KKoukiou , can you please do the code review? @garrett, can you please formally sign off (PR review) on the design now?

Great work! I'm really happy to see this, this has bothered a lot of users (and myself..) 🎉

@garrett
Copy link
Member

garrett commented May 14, 2019

@jgiardino brought up a good point on IRC that the focus ring looks more like Firefox's native dotted ring (which Epiphany on Linux also does). Perhaps we should align the design more to Chrome's blue outline approach. I think Safari on macOS uses a blue ring too (although not by default, as you have to turn on tabbing for some odd reason).

It would be a little change, and probably should be in another PR. What do you think?

(I think it might even be possible to do some browser detection stuff, perhaps even check for Mozilla-specific property with @supports and serve up Chrome/Safari style by default and Firefox's for Firefox.)

@martinpitt
Copy link
Member Author

@garrett : Honestly I completely defer to your and @jgiardino's judgement there. Some tweaking in a follow-up PR seems perfectly fine, of course. (FTR, I'd just like to land this one soon as it's relatively intrusive, and will age quickly)

@garrett
Copy link
Member

garrett commented May 14, 2019

Yep, let's land this ASAP, as-is. 👍

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks

@KKoukiou
Copy link
Contributor

I went over all tests that failed and they are all known flakes, so if it's urgent for merging it's fine.

@martinpitt
Copy link
Member Author

@KKoukiou : Thanks! avocado/fedora is busted right now (@marusak is looking into this), I'll retry the others some more.

@martinpitt martinpitt merged commit a2b0d7d into cockpit-project:master May 14, 2019
@martinpitt martinpitt deleted the on-off-redesign branch May 14, 2019 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants