Move CSS to CSS files and add new tests for stylesheets

[sssoleileraaa]

Description

Towards #1078

Goals behind this PR:

• Start using .css files instead of strings or dictionaries of strings or dictionaries of dictionaries of strings
• Separate css stylesheets from Qt application logic
• Use standard CSS object names that correspond to Qt Widget class names
• Set the stylesheet for the application once, except for when we need to change the CSS of an object based on an action.
• Add integration tests to make sure the styles are what we expect
• Add regression tests to make sure if a class name changes then the CSS object name needs to be updated and vice versa

Test Plan

There are many integration tests that you'll see check to see that the widget styles are what we expect them to be. We are almost at 100% UI style coverage now. Since it's difficult to keep track of where we have coverage for stylesheets, I left comments in the test code for assertions that will get us to 100% coverage. The comments all relate to the following types of tests:

• border tests, including border-radius tests, see https://code.woboq.org/qt5/qtbase/src/widgets/styles/qstylesheetstyle.cpp.html#_ZN21QStyleSheetBorderDataC1EPiP6QBrushPN4QCss11BorderStyleEP5QSize
• padding tests
• text alignment tests
• menu-indicator tests
• image: none; and show-decoration-selected: 0; tests

There are also comments in the test code for unexpected test failures, e.g. a background color of a disabled button should return gray but instead returns white. There aren't that many of these and it could be because of test writing fatigue that these are broken, however, in order to make sure the client looks like what we expect it to look like, the test plan is to compare master to this pr branch by following this plan:

1. Run the client from master and take screenshots
2. Run the client from this PR branch and take screenshots
3. Use a tool to compare the screenshots or manually compare and pay close attention to borders, colors, and layout spacing/ margins.
4. Run the client from qubes and repeat steps 1-3

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 master and confirmed that the migration applies cleanly
• I have written a migration but have not upgraded a test database based on master and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[kushaldas]

This looks like a good idea. But we must start working on proper screenshot based testing of the application to make sure that they widgets look proper in the real application.

[sssoleileraaa]

This looks like a good idea. But we must start working on proper screenshot based testing of the application to make sure that they widgets look proper in the real application.

Absolutely! The tests in this PR aim to cover all our GUI component fonts and styles, but we still need tests to cover issues like text getting cut off when the window is its min size, or certain strings no longer wrapping because of a change in Qt or something. So far this PR focuses on getting full test coverage around fonts, colors, and widget sizes. Testing borders, margins, padding, and a few other styles might be easier to test with screenshots actually since it's not super obvious to me how to get these properties from the widget in Qt.

[eloquence]

For the 5/20-6/3 sprint, our goal is to get this PR to "Ready for review" (out of draft): tests pass, up-to-date with master, and the test plan is complete and enumerates manual regression test procedures for any missing coverage.

[sssoleileraaa]

adding a test for every UI element takes a long time which is why we decided to just add a manual test to this pr's test plan to fill in the gap (we still get a lot of new coverage here) and to continue to add automated UI tests in separate PRs.

[redshiftzero]

HoverEnter is the right event for to test the setting of the icon since that logic is explicit here, but it's not obvious to me where the application of the hover selector is happening since it's not done in the eventFilter, so I'm not sure how to test this properly (i.e. is HoverEnter the right event?).

One thought prior to spending more time on this: it seems like the easiest way to test more dynamic behavior would be to actually move the cursor to hover over the download_button (similar to this but using pyautogui.moveTo) and then assert the color on the widget (and ultimately take screenshots). This doesn't necessarily need to be part of this change, but what do you think @creviera @kushaldas ?

[sssoleileraaa]

I think it makes sense to test longer sequences of client behavior using pyautogui or some other similar tool. Here we just want to check the that a button's hover event or pressed event updates the button's palette color correctly. I'm surprised there isn't an easy way to do this without actually moving the mouse cursor. Just for a little background, here are two things I tried for the login button:

1. Creating a mouse event and triggering it.

   event = QMouseEvent(QEvent.MouseButtonPress, login_button.pos(), Qt.LeftButton, Qt.LeftButton, Qt.NoModifier)
   login_button.mousePressEvent(event)

   Or triggering it this way:

   event = QMouseEvent(QEvent.MouseButtonPress, login_button.pos(), Qt.LeftButton, Qt.LeftButton, Qt.NoModifier)
   QCoreApplication.postEvent(main_window, event)

2. Using Qt's QTest unit testing framework which offers functionality for mouse and keyboard simulation.

   QTest.mousePress(login_button, Qt.LeftButton)

   Or, for the download button scenario a mouse hover event:

   QTest.mouseMove(download_button)

But these simulations don't cause the buttons to update their palette color, even though it clearly works when we run the client. I'm starting to wonder if there is another color property somewhere... At this point I think it would be good to get @kushaldas's eyes on this PR and to move forward without full coverage. I'll create a couple followup issues for addressing the remaining work.

[sssoleileraaa]

In order to move this PR along I created the following followup issues:

• Add widget style border tests  #1096
• Finish CSS stylesheet tests #1097
• Add tests for miscellaneous widget styles #1098
• Fix any broken commented out integration tests  #1099

[kushaldas]

Main window looked good. But, there is issue with the export dialog. I used the attached patch to verify against the master branch. First, I took only the screenshots (using the commented lines), and then tried to match.

export_dialog.txt

First this what we see in the master branch.

The following is what I am getting in this branch.

[sssoleileraaa]

I'm unable to repro but will keep trying. Does this consistently happen for you? I'm curious if it's intermittent and if it could happen on master.

[eloquence]

I'm also unable to repro. I've tested both branches (master and refactor-css) in Ubuntu and Qubes. In both environments, the dialog is correctly styled as a modal. Kushal, I'm unclear from your report what environment you tested in, and why you tested using a patch rather than running on the branch. Note, however, that even your master screenshot is broken; it should be showing a window border.

Here's what it looks like to me in Qubes:

On master

On refactor-css (it says 0.1.6 because this branch hasn't been rebased since #1083)

[eloquence]

The screenshots show what may be a regression, though, note that the journalist designation in the "Compose a reply to" placeholder is cut off in the refactor-css branch screenshot. I've not done any further detailed before/after testing yet.

[kushaldas]

Kushal, I'm unclear from your report what environment you tested in, and why you tested using a patch rather than running on the branch.

The patch is not for the code, that patch is for automated screenshot based tests. I wanted to make sure that the UI looks the same in both branches while inside of the tests.

[sssoleileraaa]

"Compose a reply to" placeholder is cut off

I see this too and will fix.

@kushaldas - are you taking the screenshots via pyautogui.screenshot("first.png")? one thing I had to do in the integration tests you see here on this branch is make sure I initiate the application's main window, where the stylesheet is now set (I do not use a mock) before invoking the export or print dialog. I would like to be able to run screenshot tests so I can see why they are behaving this way - could you open a WIP PR so we discuss there? I'm also curious how the screenshots look in Qubes since window management is a bit different there.

[sssoleileraaa]

Notes about where we could use coverage via screenshot tests here: #1004 (comment)

[eloquence]

I took the current branch through its paces in Qubes, validating standard client functionality such as login, errors, export, and deletion, and comparing screenshots against the behavior in master. I didn't detect any noticeable discrepancies, and I was no longer able to reproduce the cropping bug (including in offline mode). I noticed one very subtle difference in the color of source messages:

Above: master
Below: refactor-css

(I almost need to use a color picker to even notice the difference.)

[sssoleileraaa]

thank you for the great review! moving this back into development and will have to address the issues found next week.

[kushaldas]

@creviera

I just now pushed the code to generate screenshots for the export dialog at https://github.com/freedomofpress/securedrop-client/tree/selfie_for_1082

You will have to install scrot tool for the screenshots.

sudo apt install scrot -y
xvfb-run  --server-args="-screen 0, 1680x1050x24" -a python3 -m pytest -v tests/functional/test_export_dialog.py

The above command will generate 4 screenshots.

[redshiftzero]

OK I just finished testing this a bunch in Qubes, comparing with the latest client deb (0.2.0). I cannot reproduce your report @kushaldas - I tested the USB export in Qubes and I do not see any style regressions: all works as expected.

I also don't really see any difference in the message bubbles - color bar is the same expected color and fonts weights seem to be the same in messages vs replies. However I did notice that on master we set color to #3b3b3b for the ReplyWidget only - I think this could be the source of the divergence (on this branch we set to #3b3b3b for SpeechBubble (parent of message and reply widgets)).

Otherwise, I would be in favor at this point of merging this given its size and then we can improve the tests in the followup issues @creviera created.

[eloquence]

(on this branch we set to #3b3b3b for SpeechBubble (parent of message and reply widgets)).

That explanation makes sense to me, looking closely at master I notice the subtle difference between messages and replies, which I'm guessing was unintentional to begin with. :)

[redshiftzero]

checking the reply behavior by logging in, sending a reply, logging back out, and logging back in. All works as advertised. Thanks!

@kushaldas if you agree this is ready, feel free to dismiss your review or re-review (whichever you prefer) - also want to post a link to your screenshot branch in #1004? we can continue that discussion there

[redshiftzero]

These strings are hardcoded, so OK to use QLabel here

[redshiftzero]

✅ this is much nicer than having the style inline

Will add more tests in future.

[kushaldas]

I did one more round of testing locally, looks good. Approving.