Replace hide/show Passphrase icon control, with checkbox control

[Er1kkkaaa]

Description

The following work is a custom checkbox control created as a replacement of the hide/show passphrase icon control present in the Sign-in and Export Dialog.

Fixes #1351

Test Plan

• Enter the passphrase in the password field.
• Hide/Show the passphrase by checking and and unchecking the checkbox.
• Hide/Show the passphrase by pressing the Space-bar key.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

• I have updated the AppArmor profile
• No update to the AppArmor profile is required for these changes
• I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

• I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
• I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[gonzalo-bulnes]

Hi @Er1kkkaaa! I added a few thoughts, with the caveat that it was a very quick glance, so I may be wrong in things! 🙂

[gonzalo-bulnes]

Does checkbox.isChecked() do anything in this context? Was it meant to be an assertion, or maybe a debug statement?

[sssoleileraaa]

I see a semgrep.untranslated-gui-string failure:

 securedrop_client/gui/base/checkbox.py 
     semgrep.untranslated-gui-string
        Untranslated GUI string


         35┆ self.label = QLabel("Show Passphrase")
         36┆ self.label.setFont(font)
...
ran 11 rules on 55 files: 1 findings

To translate this string you'll have to update QLabel("Show Passphrase") to QLabel(_"Show Passphrase").

[sssoleileraaa]

Actually, my bad, I just haven't pulled your most recent changes. Aaaaaand it looks like you already fixed that one!

[sssoleileraaa]

If you run make black you'll fix the other error. After that, it looks like all tests will pass. There are a lot of commits here, so you can squash them once you're ready for a final review.

[gonzalo-bulnes]

There are some changes in this branch that are not related to the checkbox work. I suspect you may have merged the main branch into 1351-checkbox-control @Er1kkkaaa. That's not a big deal, but it adds unrelated commits. I tried rebasing this branch onto main instead, and the resulting commit log is clearer 🙂 (For reference: git checkout main && git pull origin main && git checkout 1351-checkbox-control && git rebase main - exits without conflicts or errors and removes the unrelated commits.)

As @creviera thought, the linting/formatting tools do clean up the remaining errors automatically. (I tend to run them in bulk: make extract-strings black isort link mypy.) ✔️

I ran this code, and it seems fully operational to me, in that the standard keyboard navigation behaves as expected 🎉 (And I must say that as much as I liked the icon, I love this fix!)
The original issue made mention of additional highlights for the different focus / active states. While I think those are important, I also think this PR is a good first iteration, and I would advocate for shipping it and addressing further improvements as a separate PR. What do you think @creviera?

I'm happy to rebase / lint /squash commits to take this to the merging line if that helps, also happy to pair with you @Er1kkkaaa if you've got the time / want to do those little bits of "bureaucracy" yourself 😉

Edit: I just saw the hover state. The background highlight lowers the text contrast significantly, I'll see if there is an easy way to get it closer to Nina's intention. 👁️

[gonzalo-bulnes]

Here is what the branch looks like once rebased: https://github.com/freedomofpress/securedrop-client/compare/main...gonzalo-bulnes:1351-checkbox-control?expand=1
There is room to squash a few commits IMHO, but I didn''t want to do it before hearing from you(s) @Er1kkkaaa and @creviera 🙂

[sssoleileraaa]

@gonzalo-bulnes, is this PR something you think is worth continuing to rebase and keep updated, or is it in a state where it makes more sense to close for now (based on your review)?

[gonzalo-bulnes]

I think it might make sense to ship it. As mich as I likes the lillte eye icon, this is a significant accessibilty improvement. : )

[sssoleileraaa]

Okay, if it's close to ready for review, I'll add it to the project board so we can get someone to review it once it's fully ready (assuming it just needs a rebase and looks like there are failing tests)

[gonzalo-bulnes]

I've re-based this branch with no changes, then added two commits to adjust the highlight when the component is hovered. I'll force push this now. 🙂

[gonzalo-bulnes]

Note: I signed @Er1kkkaaa's commits as I was rebasing the branch, but didn't modify them. I've a mixed feeling on squashing them without @Er1kkkaaa, because I wouldn't want to mis-represent her intent. With that in mind, I would personally merge them as-is. Any thoughts @creviera ?

Besides that, I'll call this PR ready to go!
To that purpose, I'm attaching a few screenshots so that @tina-ux can chime in if needed.

---

The default is to keep the passphrase hidden (broader UI for context):

Hover state:

Focus state. Granted this one is barely visible, it is the default Qt decoration for focused checkboxes. I spent a bit of time looking for ways to bring it closer to @ninavizz intent, and ran out my time box. That's an improvement I'd leave for later on, as we progress standardizing the UI components.

Checked.

Checked + hover.

[tina-ux]

Thank you @gonzalo-bulnes! Quick question: is it possible to determine the font of the text inside form input fields on Qubes?

For this stage, things look good to me! Choosing a checkbox and a descriptive text label to "Show Passphrase" is indeed good practice both for usability and accessibility. Good call on limiting the time spent on the focus state checkbox, we can revisit that small detail down the road. There are some other design challenges in this log in form that I would like to revisit at a ulterior time such as the placement of "show passphrase" and some broader design decisions that dampen visibility, but for now it looks good :)

[gonzalo-bulnes]

@tina-ux We can control the font-size of the input fields (including the passphrase field). As far as I can tell, it currently defaults to 13px or something equivalent. (I'm not sure where the default comes from, but I believe that would be a value provided by Qt in a platform-specific way, likely based on the platform own defaults).

Basic font properties can be set using Qt styles sheets, which behave like a subset of CSS. I say basic to mean the equivalents of CSS font-family, font-size, font-weight and probably a few more, but letter-spacing is out of bounds for example.

[gonzalo-bulnes]

Note: we will get more styling options as we update Qt. For example, letter-spacing is available in Qt 5.15.6 (we use Qt 5.11.3).

Related to #1194

[tina-ux]

@gonzalo-bulnes Hah! Thank you for going above and beyond with the screenshots and the info! I was indeed asking about setting a font family for the input field. I'll keep this in mind for when we come back to this login window.

[sssoleileraaa]

Note: I signed @Er1kkkaaa's commits as I was rebasing the branch, but didn't modify them. I've a mixed feeling on squashing them without @Er1kkkaaa, because I wouldn't want to mis-represent her intent. With that in mind, I would personally merge them as-is. Any thoughts @creviera ?

I agree. No need to worry about squashing. I'll review the PR now that it's green and ready to review! Thanks @gonzalo-bulnes for prepping it.

[sssoleileraaa]

Just took a look at the small appended commits and based on @gonzalo-bulnes's approval, looks like this is good to merge! No need for me hold things up. The new way to show/hide the passphrase looks great- thanks @Er1kkkaaa :)

[gonzalo-bulnes]

Thank you so much @Er1kkkaaa for your diligent work on this!! 🚀