Source Interface Redesign Omnibus #6315
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Took a quick first spin, coming together really nicely! A few quick notes from my end:
|
|
|
Feedback incorporated |
There was a problem hiding this comment.
Overall testing went well, no issues with functionality. I left inline comments about possible localization issues.
It somewhat bothers me that the shape/size redacted version of the codename doesn't actually match the underlying codename. I don't know how hard it would be to implement that though.
securedrop/source_app/main.py
Outdated
| @@ -47,11 +46,10 @@ def generate() -> Union[str, werkzeug.Response]: | |||
| elif tor2web_check != 'href="fake.onion"': | |||
| return redirect(url_for('info.tor2web_warning')) | |||
| if SessionManager.is_user_logged_in(db_session=db.session): | |||
| flash(gettext( | |||
| flash_msg( | |||
There was a problem hiding this comment.
Did you test if this pattern works with the pybabel extractor? IIRC it scans the Python source code looking for gettext() function calls.
There was a problem hiding this comment.
Specifically see https://github.com/python-babel/babel/blob/61109c4bd4d60a79d7cb5f216a8286ddff6fb30b/babel/messages/extract.py#L31 - I think we'd need a custom extractor if we want to support more function names.
There was a problem hiding this comment.
You're totally correct, I was overly optimistic/absent minded about pybabel. Changed the helper function and returned to the gettext() calls
| // Colors for "local" referencing. | ||
| // Please use descriptive variables stylesheets | ||
|
|
||
| // Blue-ish |
There was a problem hiding this comment.
I think this might be a pre-existing problem, but these variable names/colors are kind of useless to me because I would have no clue when to use cerulean vs cyan vs grimace blue, you get the idea.
I like what the Wikimedia Style Guide does with Red90, Red50, Red30, etc.: https://design.wikimedia.org/style-guide/visual-style_colors.html and https://design.wikimedia.org/style-guide/resources/WikimediaUI_color_palette.svg
There was a problem hiding this comment.
My kneejerk reaction to this was "absolutely, and functional descriptions would be even better", but looking ahead in _source_vars.sass, the actual variables using these colors are pretty well-named, and those vars are what actually get used. (I don't know if this modifies your postion at all).
There was a problem hiding this comment.
Yep, the color names are partially inherited, some are taken from Figma, some are new because they haven't had a name assigned but popped up often enough (for what it's worth it might be cool to introduce them in Figma as well but I'm better at reading Figma than changing stuff on there). source.sass should never reference those colors directly (which is what "local referencing" is intended to convey - maybe I should expand the comment?)
My general preference would be that developers wouldn't make the (final) choice for what color is used where. Ideally @tina-ux would be involved in the process before implementation happens, but even if that's not possible, a descriptive variable name should be used in source.sass and the final color can be adjusted later on. Does that make sense?
This throws out source.sass as well as quite a bit of HTML, separating
the source interface and journalist interface (temporarily?) so we don't
break the JI while refreshing the SI according to Nina's specs.
In this commit, in no particular order:
* Reuse flash message template, supporting "declarative" and icon only
messages.
In the future, every flash message ought to have a declarative bit
added to it. This change avoids introducing new strings, so leaving
this to the future.
* Refactor SI locale menu
The menu for locale selection has a lot of unused complex crud we
don't need to make it look/work the way it does. Less stuff to
wrangle: good!
* Bring flash message categories closer to SI redesign terminology
* Do not scroll to flashed message after submission
It's cumbersome and obscures some of the interface, and there isn't
all that much going on visually for this to help much with bringing
attention to the flash messages, especially since they're on top now.
* Change codename widget UI, and keep it in its own template
The codename widget should look and work the same in both places
where it is being served.
Regarding the implementation, we're emulating a checkbox visually
using ::before pseudo-elements in a label, while the actual checkbox
state is used to show/hide the codename, necessitating that the input
precedes the codename and label in HTML.
* Switch out icons
Nina designed some icons for us, and changed some others. Material
Design based icons live in their own folder with their own README,
referencing the license, plus the SVG files have a comment with an
SPDX short identifier.
There's a lot of files which haven't been used in a long while,
includes a first attempt at cleaning those up, but there's more left
to do, f.e. font-awesome icons are barely used.
The new icons are monochrome. This isn't primarily a space-saving
measure, but to enabling color tweaks by using them as mask-images.
Similarly, there's a couple of instances in which there's "negative"
icons, where a white image foreground sits on top of a CSS
background-color. Unfortunately, these do not (easily) support
replacing white foreground with a different color.
SVG files are tracked in case images need to be replaced but SVG
rendering is disabled in Tor Browser's "Safest" mode
* Refactor JS browser warnings
* Orfox no longer exists, it's now called Tor Browser for Android
* Update Tor Browser for Android UA regex
* Removed dead JS
* Removed dependency on font-awesome icon
* Removed animations - there's no other animations in the source
interface and adds very little value - if we want animations back
I'd suggest implementing them as much as possible with CSS
* Tab focus changes
Make sure that tab focus is properly visible and works like we want
to even on input elements in which what we actually interact with is
the label.
* Add most recent logos - colors were tweaked!
* RTL support
While RTL support is present in SD, some of the details like margins
in locale label weren't quite right. This should flip all relevant UI
elements including the codename widget etc.
* Revise button order
Button order for regular actions is `[ Escape ] [ Action ]` where
`[ Action ]` is "solid" / attention-grabbing
For destructive actions this is reversed
`[ Destructive Action ] [ Escape ]` and `[ Escape ]` is "solid"
For smaller screens, make the browse button bigger, also always have a proper tree of headings starting with h1
There was a problem hiding this comment.
Stepped through source flow in a staging environment, looks great, notes as follows:
- no issues with apparmor denies, but comparing the profile to what's installed shows some deleted resources still listed (mostly not introduced by this PR)
- (personal pref) the explanatory reply text is pretty tiny for me on Debian 11 and TBB, I don't see the rationale in having it be 3 points smaller than other page text.
With fixes requested by other reviewers and tweaks to the Apache apparmor profile, this looks good to go!
| /var/www/securedrop/source_templates/footer.html r, | ||
| /var/www/securedrop/source_templates/generate.html r, | ||
| /var/www/securedrop/source_templates/index.html r, | ||
| /var/www/securedrop/source_templates/locales.html r, | ||
| /var/www/securedrop/source_templates/login.html r, | ||
| /var/www/securedrop/source_templates/logout.html r, | ||
| /var/www/securedrop/source_templates/lookup.html r, | ||
| /var/www/securedrop/source_templates/next_submission_flashed_message.html r, | ||
| /var/www/securedrop/source_templates/notfound.html r, | ||
| /var/www/securedrop/source_templates/session_timeout.html r, |
There was a problem hiding this comment.
file was deleted, not needed in apparmor profile anymore. (also journalist_templates/delete.html was removed in 2.3.0, tho strictly speaking not in scope for this PR)
| /var/www/securedrop/source_templates/use-tor-browser.html r, | ||
| /var/www/securedrop/source_templates/utils.html r, | ||
| /var/www/securedrop/source_templates/why-public-key.html r, | ||
| /var/www/securedrop/static/.webassets-cache/** rw, | ||
| /var/www/securedrop/static/css/font-awesome.css r, |
There was a problem hiding this comment.
not introduced in this release, but looks like this and static/css/normalize.css has been gone for a long time!
| @@ -263,16 +263,19 @@ | |||
| /var/www/securedrop/static/gen/journalist.js rw, | |||
| /var/www/securedrop/static/gen/source.css rw, | |||
There was a problem hiding this comment.
cssmin was removed in #4227 so these are no longer present.
As far as I remember, there's two parts to this, though the second one I recalled from memory so take that with a grain of salt:
@tina-ux what do you think, as a sort of in-between stage, is it more appropriate to adapt font-sizes a tad? As for your requested changes, I updated the AppArmor profile accordingly 🙂 |
Codename is now not read if "Show Codename" is unchecked, pre-load images are also not read out at the end of every page. Also moving codename template to the utils.html, feels more appropriate if we already have that file anyway Changes a notification text that refers to the codename "above", but with these design changes the codename is not above anymore.
Increased font and icon size
|
Checked in with Tina and decided to increase the font size of the reply info note by 2 pixels! |
Codecov Report
@@ Coverage Diff @@
## develop #6315 +/- ##
===========================================
- Coverage 84.02% 83.79% -0.23%
===========================================
Files 61 62 +1
Lines 4312 4320 +8
Branches 524 525 +1
===========================================
- Hits 3623 3620 -3
- Misses 565 575 +10
- Partials 124 125 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Status
Ready for review
Description of Changes
Refactoring towards #6211 - changes in this PR are primarily visual, the vast majority of the interaction stays the same.
source.sassis dead, long livesource.sassA complete rewrite of
source.sass, plus quite a bit of the templates as well. So we're separating the source interface and journalist interface (temporarily?) to not break the JI while refreshing the SI according to Nina's specsRefactor locale menu
The menu for locale selection has a lot of unused complex crud we don't need to make it look/work the way it does. Less stuff to wrangle: good!
Reuse flash message template, supporting "declarative" and icon only messages
In the future, every flash message ought to have a declarative heading added to it, this change makes that less cumbersome to add in the future once we found the right language. Also: bring flash message categories closer to SI redesign terminology
Change codename widget UI, and keep it in its own template
The codename widget should look and work the same in both places where it is being served.
Regarding the implementation, we're emulating a checkbox visually using
::beforepseudo-elements with a label, while the actual checkbox input state is used to show/hide the codename, necessitating that it precedes the codename and label in HTML. (Similar to the locale menu implementation)Use descriptive/semantic variables for all use of colors
Any time a color is used, use a variable that describes the context of its use rather than the color itself. (Distant future possibilities: can help us implement dark mode or - more controversially - allow very simple white label design features beyond just replacing the logo.)
Switch out icons
Nina designed some icons for us, and changed some others. Material Design based icons live in their own folder with their own README, referencing the license, plus the SVG files have a comment with an SPDX short identifier.
There's a lot of files which haven't been used in a long while, so this PR includes a first attempt at cleaning those up, but there's more left to do (font-awesome icons are barely used, make a good candidate for getting cut).
The new icons are monochrome. This isn't primarily a space-saving measure, but it enables color tweaks by using them as mask-images. Similarly, there's a couple of instances in which there's "negative" icons, where a white image foreground sits on top of a CSS background-color.
SVG files of these icons are versioned in case images need to be replaced, but SVG rendering is disabled in Tor Browser's "Safest" mode, so they're not directly used in the UI.
Refactor JS browser warnings
Tab focus changes
Make sure that tab focus is properly visible and works like we want to even on input elements in which what we actually interact with is the label (codename widget & locale menu)
Add most recent logo - colors were tweaked!
RTL support
While RTL support is already present in SD, some of the details weren't quite right. This should flip all relevant UI elements including the codename widget & locale menu.
Revise button order
Button order for regular actions is:
[ Escape ] [ Action ], where[ Action ]is "solid" / attention-grabbingFor destructive actions this is reversed:
[ Destructive Action ] [ Escape ], and[ Escape ]is "solid"Testing
Design
The "Product Colors" logo from Figma is used (colors "pop" more than before)
Using the latest Tor Browser, nothing shows up distorted, overlaps when it obviously should not, or jumps unexpectedly (on hover, focus, or click)
Basic liquid layout works
The codename widget "weeble" icon is always vertically centered, even when the codename is long enough for a line break
UX
[ Cancel ] [ Action ][ Destructive Action ] [ Cancel ]Accessibility
Use the entire workflow of the SI by keyboard only
[ ] Show Codenamewidget checkbox can be interacted with by pressing the spacebarItems in (tab rather than mouse) focus show a 3 pixel big cyan border, text inputs a light blue border
Buttons "grow" a border on hover
Screen reader based tests TBD
I18n
Right-to-left languages (currently only Arabic)
Arabic specifics
Staging
Demo site usage
Checklist
make lint) and tests (make test) pass in the development containerKnown issues
/lookupComments
The most unusual (and arguably impactful) choice I made in this change (other than to opt for refactoring in the first place) is avoiding adding to HTML whenever possible/reasonable, instead of following BEM or a similar methodology. This intentional, as BEM duplicates a lot of information already implicitly present in HTML, while not providing us with enough benefits for our (web-tech wise rather niche) use case. Onion services bandwidth can be incredibly limited at times, which steers me away from redundant HTML especially because SecureDrop's frontend wouldn't benefit much (at all?) of moving to more discrete components. The main downside is that there are a few uses of complex CSS selectors that aren't particularly friendly to developers that don't do a lot of frontend design work (especially if/when Sass gets removed as would be my preference), but it is far from overwhelming because the SI doesn't have that many different screens to begin with.