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

refactors Source Interface for semantic HTML5/ARIA markup #6041

Merged
merged 6 commits into from
Aug 25, 2021

Conversation

cfm
Copy link
Member

@cfm cfm commented Jul 13, 2021

Status

Ready for review

Stylistic changes suggested for rebasing after review and before merge:

  1. "[Squash] each phase's commits into one"
  2. Rebase from develop
  3. Lint (beyond make lint) for indentation of HTML in all securedrop/source_templates/*.html

Description of Changes

Closes #5986 by:

  1. refactoring all securedrop/source_templates/*.html for semantic markup;
  2. adding ARIA annotations where HTML5 semantics are insufficient to make the Source Interface easily navigable by assistive tech, especially in the absence of JavaScript for changing state, polyfilling for undersupported HTML5 elements, etc.; and
  3. fixing SASS/CSS in light of (1)–(2), especially to move decorative images out of the DOM and accessibility trees.

Known issues

For future work:

  1. make BCP47/RFC5646 RequestLocaleInfo.language_tag property available in locale selectors #6056 (out of scope)
  2. invalid datetime attributes #6057 (out of scope)
  3. Some pages need h1s (address in re: WCAG 2.4.2)

Testing

Click through the Source Interface and inspect in terms of:

  1. Markup, by inspecting and validating HTML. This need not take place in Tor Browser; I used the Web Developer extension in Chrome.
  2. Accessibility in assistive tech, by listening to the interface as announced from Tor Browser by a screen-reader. It should sound and "feel" conspicuously more fluent. (I also audited pages in WebAIM's Wave Evaluation Tool and Deque's axe DevTools, but these will yield mixed results until the conclusion of this accessibility push.)
  3. Visual appearance, by comparing the interface against develop. Changes are intended to be "pixel-perfect" except where there were preexisting idiosyncrasies that more-semantic markup tended to make naturally more consistent. Of course, suggestions for fine-tuning are welcome!

Deployment

No special considerations.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Let me know if this is necessary.

@cfm cfm force-pushed the 5986-semantic-source-interface branch 2 times, most recently from 484cbd7 to 3ad7b2e Compare July 15, 2021 06:34
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #6041 (80547cd) into develop (0589168) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #6041   +/-   ##
========================================
  Coverage    85.11%   85.11%           
========================================
  Files           55       55           
  Lines         3863     3863           
  Branches       479      479           
========================================
  Hits          3288     3288           
  Misses         463      463           
  Partials       112      112           
Impacted Files Coverage Δ
securedrop/source_app/forms.py 94.73% <100.00%> (ø)
securedrop/source_app/main.py 92.26% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0589168...80547cd. Read the comment docs.

@cfm cfm force-pushed the 5986-semantic-source-interface branch 2 times, most recently from b0ef502 to 80547cd Compare July 19, 2021 04:27
@cfm cfm marked this pull request as ready for review July 19, 2021 04:35
@cfm cfm requested a review from a team as a code owner July 19, 2021 04:35
@cfm cfm requested a review from zenmonkeykstop July 19, 2021 04:35
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

This is amazing work @cfm !!! Love to see efforts towards making the HTML more semantic and accessible. I have left some small comments/questions that caught my eyes. Few are probably bigger changes which might need their own separate issues.

securedrop/source_templates/flashed.html Outdated Show resolved Hide resolved
securedrop/source_templates/base.html Outdated Show resolved Hide resolved
securedrop/journalist_templates/_source_row.html Outdated Show resolved Hide resolved
securedrop/source_templates/index.html Outdated Show resolved Hide resolved
securedrop/source_templates/index.html Outdated Show resolved Hide resolved
securedrop/source_templates/locales.html Show resolved Hide resolved
securedrop/source_templates/generate.html Outdated Show resolved Hide resolved
securedrop/source_templates/lookup.html Outdated Show resolved Hide resolved
securedrop/source_templates/locales.html Outdated Show resolved Hide resolved
securedrop/source_templates/lookup.html Outdated Show resolved Hide resolved
cfm added a commit that referenced this pull request Jul 27, 2021
Corrects 876bfa2's removal of these decorative IMGs without replacing
their semantic content.

thread: #6041 (comment)
squash/fixup: 876bfa2
@cfm cfm force-pushed the 5986-semantic-source-interface branch from e8c4073 to c6cd5af Compare July 27, 2021 18:50
cfm added a commit that referenced this pull request Jul 27, 2021
Adds explicit ARIA-LABEL attributes to elements whose strings are in
all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)
cfm added a commit that referenced this pull request Jul 27, 2021
Adds explicit title-case ARIA-LABEL attributes to elements whose strings
are in all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)
@cfm cfm force-pushed the 5986-semantic-source-interface branch from 79b5924 to 17eb17a Compare July 27, 2021 19:17
cfm added a commit that referenced this pull request Jul 27, 2021
Refactors MAIN.index-row rather than wrapping the whole DIV.grid in
MAIN.

thread: #6041 (comment)
squash/fixup: 80547cd
cfm added a commit that referenced this pull request Jul 27, 2021
Adds explicit title-case ARIA-LABEL attributes to elements whose strings
are in all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)
cfm added a commit that referenced this pull request Jul 27, 2021
Refactors MAIN.index-row rather than wrapping the whole DIV.grid in
MAIN.

thread: #6041 (comment)
squash/fixup: 80547cd
@cfm cfm force-pushed the 5986-semantic-source-interface branch from 3ac7810 to 92a22c6 Compare July 27, 2021 21:33
cfm added a commit that referenced this pull request Jul 27, 2021
Adds explicit title-case ARIA-LABEL attributes to elements whose strings
are in all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)
cfm added a commit that referenced this pull request Jul 27, 2021
Refactors MAIN.index-row rather than wrapping the whole DIV.grid in
MAIN.

thread: #6041 (comment)
squash/fixup: 80547cd
cfm added a commit that referenced this pull request Jul 27, 2021
@cfm cfm force-pushed the 5986-semantic-source-interface branch from 92a22c6 to 07cd655 Compare July 27, 2021 21:48
cfm added a commit that referenced this pull request Jul 27, 2021
LABEL[for="codename"] was marked HIDDEN to prevent redundant narration
in screen readers.  Refactoring the LABEL to an H2 that ARIA-labels
the containing section streamlines the narration and makes for a
more-logical outline.

thread: #6041 (comment)
squash/fixup: ae56f49
cfm added a commit that referenced this pull request Jul 28, 2021
::{before,after} pseudo-elements must be used to style the
expand/collapse links ("Show"/"Hide", respectively) because under DIR="ltr"
the ::marker pseudo-element will be displayed to the left of the
SUMMARY.

thread: #6041 (comment)
squash/fixup: 1f69640
cfm added a commit that referenced this pull request Jul 28, 2021
::{before,after} pseudo-elements must be used to style the
expand/collapse links ("Show"/"Hide", respectively) because under DIR="ltr"
the ::marker pseudo-element will be displayed to the left of the
SUMMARY.

thread: #6041 (comment)
squash/fixup: 1f69640
@cfm cfm force-pushed the 5986-semantic-source-interface branch from 66ab8ba to c8baf3d Compare July 28, 2021 00:25
@rmol
Copy link
Contributor

rmol commented Aug 9, 2021

The changes of output to mark or p worked, as did the fix for the replies section. Once the merge conflicts are resolved and CI is passing, I think this is good to go. Nice work, @cfm.

cfm added 6 commits August 9, 2021 13:59
reconciles JavaScript-free locale selector with ARIA best practices

Accessible checkbox-based menus (e.g., hamburger menus[1]) tend to
rely on JavaScript to update the menu's ARIA attributes in response to
changes in state.  Here we reconcile the script-free and accessibility
requirements so that:

1. the current locale and "Choose locale" menu are announced logically;
   and

2. the menu itself is annotated to be announced by screen-reader and
   navigable by keyboard.

As a bonus, each locale is now announced *in* that locale, via the A
LANG and HREFLANG attributes.

[1]: http://www.ashleysheridan.co.uk/blog/Making+an+Accessible+Hamburger+Menu

first pass through "/generate"

restore div#content and div#container

first pass through "/lookup"

pass 1.5 through /lookup with replies

sets focus to #flashed when POST /submit redirects to /lookup

As recommended by <https://webaim.org/techniques/formvalidation/#error>.

first pass through "/logout"

first pass through "/login"

first pass through /

first pass through ancillary templates
second pass through /: validate/audit

second pass through /lookup: validate/audit

second pass through /login: validate/audit

second pass through ancillary templates: validate/audit
third pass through /: fix "footer.html" styling

Introduces a change in vertical spacing which could be corrected.

Journalist Interface breakage:  Adds a gratuitous BORDER-TOP to the
FOOTER.

third pass through /: fix H1 logo styling

I've been persuaded[1] that:

1. the logo image is content, rather than decoration, after all, and
   so can remain an explicit IMG, with an ALT attribute that describes
   the image rather than offers a tooltip-style TITLE; and

2. on the home page the logo *is* the top-level heading, reflected in
   both the H1 and the page TITLE.

[1]: https://csswizardry.com/2010/10/your-logo-is-an-image-not-a-h1/

third pass through /: fix #js-warning styling

third pass through /: fix "locales.html" styling

third pass through /generate: "{base,generate}.html" styling

third pass through /generate: "Refresh Codename" button styling

third pass through /lookup: fix "lookup.html" styling

third pass through /lookup: fix "flashed.html" styling

third pass through /: fix "{first,next}_submission_flashed_message.html" styling

third pass through /logout

third pass through /login: fix "login.html" styling

third pass: fix "banner_warning_flashed.html" styling

third pass through /why-public-key: fix "why-public-key.html" styling
Replaces STRONG elements with B, according to the latter's new HTML5
semantics[1].

Removes STRONG elements in headings, where they are superfluous both
semantically and visually.

[1]: https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-b-element
addresses review feedback

Corrects 876bfa2's removal of these decorative IMGs without replacing
their semantic content.

thread: #6041 (comment)
squash/fixup: 876bfa2

addresses review feedback

Adds explicit title-case ARIA-LABEL attributes to elements whose strings
are in all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)

addresses review feedback

Refactors MAIN.index-row rather than wrapping the whole DIV.grid in
MAIN.

thread: #6041 (comment)
squash/fixup: 80547cd

removes stray ARIA-LABELS (per review feedback)

thread: #6041 (comment)

addresses review feedback

LABEL[for="codename"] was marked HIDDEN to prevent redundant narration
in screen readers.  Refactoring the LABEL to an H2 that ARIA-labels
the containing section streamlines the narration and makes for a
more-logical outline.

thread: #6041 (comment)
squash/fixup: ae56f49

refactors SECTION#codename-hint as DETAILS (per review feedback)

::{before,after} pseudo-elements must be used to style the
expand/collapse links ("Show"/"Hide", respectively) because under DIR="ltr"
the ::marker pseudo-element will be displayed to the left of the
SUMMARY.

thread: #6041 (comment)
squash/fixup: 1f69640

adds "button" role to non-navigating links (per review feedback)

thread: #6041 (comment)

undoes autoformatting

squash/fixup: 1f69640

fixes tests for refactored DETAILS#codename-hint

squash/fixup: c8baf3d

fixes unclosed ARIA-LABEL attribute (per review feedback)

thread: #6041 (comment)
squash/fixup: d6116a2

refactors TIME as H3 of reply ARTICLE (per review feedback)

Each reply now consists of an ARTICLE automatically labeled by its H3 TIME.  These implicit ARIA semantics are more fluent than those marked explicitly in 1f69640.

thread: #6041 (comment)
squash/fixup: 1f69640

refactors OUTPUTs as MARKs or Ps (per review feedback)

Some "browser / screen reader pairings may not announce or make
available the accessible name of the output, even though its a labelable
element"[1], apparently including Orca (i.e., on Tails).  These elements
give the next-best semantics without implying a live region.

[1]: https://www.scottohara.me/blog/2019/07/10/the-output-element.html

thread: #6041 (review)
squash/fixup: 1f69640
squash/fixup: f37fc17
squash/fixup: ae56f49
A refactoring like #5986 seems like a good opportunity for this kind of
reformatting.
@cfm
Copy link
Member Author

cfm commented Aug 9, 2021

Thanks, @rmol. CI is running now with everything squashed down to one commit per phase of the refactoring. Feel free to pop off the 8f2b503 head if you don't like this formatting.

@eloquence eloquence added this to Ready for Review in SecureDrop Team Board Aug 12, 2021
cfm added a commit that referenced this pull request Aug 16, 2021
addresses review feedback

Corrects 876bfa2's removal of these decorative IMGs without replacing
their semantic content.

thread: #6041 (comment)
squash/fixup: 876bfa2

addresses review feedback

Adds explicit title-case ARIA-LABEL attributes to elements whose strings
are in all caps to support idiomatic translation (cf. #5821, #6003).

thread: #6041 (comment)

addresses review feedback

Refactors MAIN.index-row rather than wrapping the whole DIV.grid in
MAIN.

thread: #6041 (comment)
squash/fixup: 80547cd

removes stray ARIA-LABELS (per review feedback)

thread: #6041 (comment)

addresses review feedback

LABEL[for="codename"] was marked HIDDEN to prevent redundant narration
in screen readers.  Refactoring the LABEL to an H2 that ARIA-labels
the containing section streamlines the narration and makes for a
more-logical outline.

thread: #6041 (comment)
squash/fixup: ae56f49

refactors SECTION#codename-hint as DETAILS (per review feedback)

::{before,after} pseudo-elements must be used to style the
expand/collapse links ("Show"/"Hide", respectively) because under DIR="ltr"
the ::marker pseudo-element will be displayed to the left of the
SUMMARY.

thread: #6041 (comment)
squash/fixup: 1f69640

adds "button" role to non-navigating links (per review feedback)

thread: #6041 (comment)

undoes autoformatting

squash/fixup: 1f69640

fixes tests for refactored DETAILS#codename-hint

squash/fixup: c8baf3d

fixes unclosed ARIA-LABEL attribute (per review feedback)

thread: #6041 (comment)
squash/fixup: d6116a2

refactors TIME as H3 of reply ARTICLE (per review feedback)

Each reply now consists of an ARTICLE automatically labeled by its H3 TIME.  These implicit ARIA semantics are more fluent than those marked explicitly in 1f69640.

thread: #6041 (comment)
squash/fixup: 1f69640

refactors OUTPUTs as MARKs or Ps (per review feedback)

Some "browser / screen reader pairings may not announce or make
available the accessible name of the output, even though its a labelable
element"[1], apparently including Orca (i.e., on Tails).  These elements
give the next-best semantics without implying a live region.

[1]: https://www.scottohara.me/blog/2019/07/10/the-output-element.html

thread: #6041 (review)
squash/fixup: 1f69640
squash/fixup: f37fc17
squash/fixup: ae56f49
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM, thanks to @cfm for the work and for your patience!

@zenmonkeykstop zenmonkeykstop merged commit d3b0d36 into develop Aug 25, 2021
SecureDrop Team Board automation moved this from Ready for Review to Done Aug 25, 2021
@zenmonkeykstop zenmonkeykstop deleted the 5986-semantic-source-interface branch August 25, 2021 18:33
cfm added a commit that referenced this pull request Sep 8, 2021
Fixes #6081.  Spacing is still *different* than pre-#6041, but as
intended.
@eloquence eloquence added this to the 2.1.0 milestone Sep 28, 2021
@cfm cfm mentioned this pull request Oct 8, 2021
26 tasks
@conorsch conorsch mentioned this pull request Oct 19, 2021
1 task
cfm added a commit that referenced this pull request Nov 3, 2021
In #6041 we added the "aria-label" attribute in the "render_kw"
dictionary passed to each wtforms.fields.Field.  Here we add them
these attributes directly to the raw INPUT elements.
cfm added a commit that referenced this pull request Nov 5, 2021
In #6041 we added the "aria-label" attribute in the "render_kw"
dictionary passed to each wtforms.fields.Field.  Here we add them
these attributes directly to the raw INPUT elements.
@cfm cfm mentioned this pull request Mar 10, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

semantic (HTML5/ARIA) page structure in Source Interface
6 participants