Refactor LoginOfflineButton as QPushButton

[gonzalo-bulnes]

Why now / motivation: The UI code can be made simpler to work with by embracing the ways and features of the Qt framework. For example: the 90% of the accessibility of the app should come for free from using Qt in the ways it's intended to. (Like 90% of a website accessibility comes from the use of semantic HTML markup.) Because of that, following the thread of accessibility issues is a good way to identify and apply bite-sized refactoring to our widgets, and improve the maintainability of the code as well as the user experience.
The less time we need to spend thinking about how the UI code works/fails, the more we can dedicate to what our users need. The framework's job is precisely to help us with that, and I think investing into aligning the existing code to the framework ways before the team grows can yield good return on investment.

---

Overview

This refactoring enures that LoginOfflineLink inherits from QPushButton. That will make the interface more consistent from the point of view of the operating system tools (e.g. accessibility tools). We'll build on that in the future. Concretely, this means that this PR enables journalists to action the USE OFFLINE button with their keyboard.

Addresses #274

Implementation

Introduces a new class SDPushButton that hold the styles of the button. That class will be extended to support the various button types that the SecureDrop Client requires.

Over time, building up a library of widgets is expected to make it easier to ensure that the user experience is consistent, but also make it easier to improve the behavior of UI elements across the board. Finally, I expect that progressively building a library of components will guide the collaboration between designers and engineers by surfacing gaps in communication or functionality (e.g. what should the focus states look like?)

As we go, the design constants (tokens) can be organized further. I did what I think makes sense for the usage that is currently given to SDPushButton, but YMMV! Please comment if you feel we should do more, or less! 🙂

Test plan

• Check that names make sense to you:
  • Namespaces (modules)
  • Classes
• Run the Client and:
  • Confirm that the login prompt looks the same as it used to
  • Confirm that the USE OFFLINE button is still functional as it used to be
  • Confim that the USE OFFLINE button can be focused and pressed using the keyboard (typically TAB, then Space)

References

• Material Guidelines: https://material.io/components/buttons
• SecureDrop Client Color definitions (Figma)

[sssoleileraaa]

Excited to see us start to split up widgets.py. Creating a buttons.py file with our button code makes a whole lot of sense. I just have one ask, which we discussed offline:

• I recommend using the setProperty method and setStyleSheet to separate the CSS from the Python code, e.g.

  securedrop-client/securedrop_client/gui/widgets.py

  Line 3754 in 5fdcd71

  self.reply_box.setProperty("class", "deleting_conversation")

The rest is just me trying to understand the motivation of your other change...

Currently, our SignInButton inherits from QPushButton, because we want it to look and feel like a button (see our QPushButton button circled in green below).

securedrop-client/securedrop_client/gui/widgets.py

Line 1601 in 5fdcd71

class SignInButton(QPushButton):

The "USE OFFLINE" link inherits from QLabel (see our QLabel link circled in green below).

securedrop-client/securedrop_client/gui/widgets.py

Line 1579 in 5fdcd71

class LoginOfflineLink(QLabel):

Your change in this PR makes it so that the link inherits from QPushButton for accessibility reasons. This sounds great, but I just want to understand why making the link a button improves accessibility. Which Qt features for QPushButtons do we need that we aren't getting from QLabel.

[gonzalo-bulnes]

On inheriting from QPushButton: that will allow to navigate and action the button using the keyboard. Independently of its appearance -which I think is a distinct topic- this specific instance is also a button. (Happy to do together a review of "button vs link" if useful.)

[gonzalo-bulnes]

With regards to the stylesheet, @creviera and I talked about the trade-off between the possibility of using variables (to make explicit and more reliable the relationship between different aspects of the styling) and the ease of use for people not familiar with Python.

I agree it's fair to trade in favor of the latter, and I'll restructure the stylesheet to allow that, then extract it to its own file. (There isn't much reason to keep it embedded if that's not necessary.) Note: I think there are significant maintainability benefits to keeping the stylesheet scoped to the component, so I won't change that for now, even if it lives a separate file. 🙂

[gonzalo-bulnes]

@creviera I updated the PR description to make the concrete a11y gain clearer.

[sssoleileraaa]

Your change in this PR makes it so that the link inherits from QPushButton for accessibility reasons. This sounds great, but I just want to understand why making the link a button improves accessibility. Which Qt features for QPushButtons do we need that we aren't getting from QLabel.

On inheriting from QPushButton: that will allow to navigate and action the button using the keyboard. Independently of its appearance -which I think is a distinct topic- this specific instance is also a button. (Happy to do together a review of "button vs link" if useful.)

Got it. Making the offline mode "link" a QPushButton instead of a QLabel allows a journalist to navigate to the offline mode "link" by tabbing, and I see you just updated the test plan to include this, which I was just about to recommend. :)

[sssoleileraaa]

With regards to the stylesheet, @creviera and I talked about the trade-off between the possibility of using variables (to make explicit and more reliable the relationship between different aspects of the styling) and the ease of use for people not familiar with Python.

I agree it's fair to trade in favor of the latter, and I'll restructure the stylesheet to allow that, then extract it to its own file. (There isn't much reason to keep it embedded if that's not necessary.) Note: I think there are significant maintainability benefits to keeping the stylesheet scoped to the component, so I won't change that for now, even if it lives a separate file. 🙂

👍 Thanks for putting so much thought into this. buttons.py should probably have a corresponding buttons.css file to make it easier to find.

[gonzalo-bulnes]

Thanks for the links to relevant issues @creviera !

[sssoleileraaa]

This could be a rabbit hole, so feel free to ignore for now, but it seems like there are times when we will want to be able to tab to QLabels, e.g. tabbing to an error message so a user can copy paste.

[gonzalo-bulnes]

We can provide a user action for that, I don't think it would be a problem 👍 (🐇 🌞)

[sssoleileraaa]

This could be a rabbit hole, so feel free to ignore for now, but it seems like there are times when we will want to be able to tab to QLabels, e.g. tabbing to an error message so a user can copy paste.

We can provide a user action for that, I don't think it would be a problem +1 (rabbit2 sun_with_face)

Wonderful. And, I agree with you that something that looks like a link should inherit from QPushButton and not QLabel, which TIL doesn't allow tabbing by default.

[sssoleileraaa]

This looks great! Super clear to review. 🚀

[sssoleileraaa]

"Besides that, they are regular Qt buttons" isn't very clear to me. It would be helpful to elaborate more on which classes belong in this file, or just remove this sentence.

[gonzalo-bulnes]

Ok, that was meant as a hint to all the benefits that come from being a regular Qt thing, e.g. accessibility. Let me make that clearer!

[sssoleileraaa]

I feel like this and the more recent update is a little reactionary to our conversation about the "offline model" link, and I could see it being confusing to newcomers who don't understand what you mean by:
"These buttons look and behave appropriately in the context of SecureDrop client,
but besides that, they are regular Qt buttons which means they benefit from
the full-range of accessibility features provided by the framework.
When adding or extending buttons in this file, please make sure the implementation
doesn't obstruct the default Qt buttons API."

It might be clearer to just say that classes added to this file should inherit from QPushButton. The part about not obstructing the default Qt buttons API could use a bit of elaboration if you're going to keep it. For instance, I'm not sure if what you're saying is that if you need to override any of the parent slots, to make sure to call the parent (not sure I 100% agree this belongs here). If you want to be opinionated about what a button is, you could say something about anything that should have a pressed event should be an SDPushButton or something. You could also elaborate on the accessbility benefits of QPushButtons, and versus what? But, again, I think keeping it more open-ended on how to implement the button behavior might be wise.

[gonzalo-bulnes]

It might be clearer to just say that classes added to this file should inherit from QPushButton.

That's a very good point.

[sssoleileraaa]

Well, I forgot that I left a comment about the "Besides that, they are regular Qt buttons" comment when I approved it. Would you like to fix that first @gonzalo-bulnes or leave it as is?

[gonzalo-bulnes]

I'll fix it @creviera, also I want to squash the commits together! (I only kept then separate for easy of seeing what's old/what's new.)

[gonzalo-bulnes]

@creviera Is the module documentation clearer this way?

[gonzalo-bulnes]

(rebased)

[sssoleileraaa]

• Check that names make sense to you:
  • Namespaces (modules)
  • Classes
• Run the Client and:
  • Confirm that the login prompt looks the same as it used to
  • Confirm that the USE OFFLINE button is still functional as it used to be
  • Confim that the USE OFFLINE button can be focused and pressed using the keyboard (typically TAB, then Space)