Improve meaningful sequence in report page #5208

lucascumsille · 2024-10-07T08:31:01Z

Fixes: https://github.com/mysociety/societyworks/issues/4560

Currently users using keyboard might miss that they can provide an update for a report, because the keyboard jumps straight to the key-tools like "Get udpates" which is at the bottom of the page. Also depending on the length of the report all the update related section might be down the bottom and invisible make it even less obvious that a user has elements they can interact with.

Please let me know if the changes in the order might cause some problems.

As a side note, the TFL audit also suggested to do the same in the /around page, which at first seems sensible, but when you have an area with a lot of reports the amount of keyboard navigation through the whole list of reports to reach to the "key-tools" make it really annoying. As an alternative if we wanted to implement this in the /around page would be to add an invisible link right before the list of reports that takes you to the "key-tools" div.

Screen.Recording.2024-10-07.at.09.25.33.mov

[skip changelog]

codecov · 2024-10-07T08:33:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.11%. Comparing base (42b9d05) to head (f5c5dd3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5208      +/-   ##
==========================================
+ Coverage   82.39%   85.11%   +2.72%     
==========================================
  Files         409      366      -43     
  Lines       32005    26782    -5223     
  Branches     5096     5096              
==========================================
- Hits        26369    22796    -3573     
+ Misses       4129     2479    -1650     
  Partials     1507     1507

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dracos

This doesn't work on mobile - the positioning in the source is a row of buttons after the report itself, before its updates. Having these right at the bottom under the update form, they'll be lost, I think? I'm not saying their current position is perfect, perhaps we can improve on that, but it does at least seem a sensible place for them to go at present. (And the tabbing order is fine then, because they're not out of any order).

I don't think we should change an around page, no. Perhaps the whole drawer concept could be changed, but I imagine everyone is used to it... An argument could be made that for consistency (WCAG SC 3.2.4), this counts as surrounding navigation and thus should be consistent between the around page and a report page? However, I agree it's a bit odd tabbing on desktop (because it comes after the report description, before the updates, so 'in the middle') - we could have separate tools row on mobile/desktop, ugh, I can't really think of an easier solution, but perhaps we can think of something different entirely.

(I notice when testing a report page that the "Shortlist" button isn't tabbable to at all, why aren't they picking up actual problems like this?)

A file where you can find all the available icons to use as inline SVGs.

Currently users using keyboard might miss that they can provide an update for a report, because the keyboard jumps straight to the key-tools like "Get udpates" which is at the bottom of the page. Also depending on the length of the report all the update related section might be down the bottom and invisible make it even less obvious that a user has elements they can interact with.

lucascumsille · 2024-10-31T15:54:41Z

Thanks for the feedback @dracos I made some work in this PR:

Bug
In the report page I noticed a bug on mobile, when you press "get updates" it opens the drawer, but you wouldn't notice unless you scroll up. I wasn't too sure what was causing the problem, till later, so my workaround was to make a different effect.

Bug here

Screen.Recording.2024-10-31.at.15.24.44.mov

New effect here, basically fixed on the top on mobile:

Screen.Recording.2024-10-31.at.15.32.52.mov

But then when I found the cause of the problem:
$('#report-updates-data').insertAfter($('#map_box')); and I got rid of it, I realised that the previous effect is not needed, but let me know if you like it. Also let me know if you think erasing this line should break anything, but it all seems to be working.

Regarding the PR itself:
I made a few accessibility improvements:

I ended up having different key-tools for mobile and desktop
When we open the drawer using keyboard then we get automatically focus in the first focusable element. Before the user would have to keep tabbing and find the opened drawer.
For assistive devices I added a close button to the small_drawer
Added aria-expanded
Not sure if you have a preferred method but I created a file: inline-svg-list that should make it easier to see the list of available icon we currently have.
The inline icons now use the currentColor for the fill, so that should improve the contrast.

Preview here:

Report page

Desktop

Screen.Recording.2024-10-31.at.15.22.28.mov

Mobile

Screen.Recording.2024-10-31.at.15.44.28.mov

Around page

For the around page I ended up adding a "Skip report tools" so it takes you straight to the report list filters. These one felt more like ticking a box, in some cases we are only saving the user to skip one or two links,

Screen.Recording.2024-10-31.at.15.50.00.mov

Let me know what you think =)

lizettal · 2024-11-15T18:04:53Z

@dracos could you please look at this on Monday so Lucas can potentially get this finished before he does on leave from Tuesday?

dracos

Overall seems like the right approach, but some things are not working, I'm afraid.

"Added accessibility improvements to small_drawer" contains a lot more in that commit than that, and other commits appear to have got mixed up as well.
It would be good if we could break this down into:

A commit to switch from external to inline SVGs, no other changes (that would also involve e.g. fixing the issue where the Feed link is moved to sub_map_links).
A commit to add the close button to the small drawer (and anything else related to that, but nothing more).
The skip button on around page if needed.
Changes to have a separate key-tools for desktop than mobile (fixing the issue with placement, not working for non-staff, etc)
Let me know if you have any questions

dracos · 2024-11-18T12:25:28Z

templates/web/base/alert/_updates.html

    </p>

+    <button type="button" class="close-drawer screen-reader-only" aria-label="Close this dialog"></button>


Is it better to use aria-label or just have the button have the same text as its actual contents?

dracos · 2024-11-18T12:29:51Z

templates/web/base/reports/_rss.html

@@ -1,7 +1,7 @@
 <div id="key-tools-wrapper" class="shadow-wrap">
    <ul id="key-tools">
-        <li class="feed-list-wrapper"><a rel="nofollow" id="key-tool-updates-area" class="feed" href="[% rss_url %]">[%
-            SET monikers = ['bromley','hounslow'];
+        <li class="feed-list-wrappe"><a rel="nofollow" id="key-tool-updates-area" class="feed has-inline-svg" href="[% rss_url %]">


Lost an r on wrapper

dracos · 2024-11-18T12:31:19Z

templates/web/base/reports/_list-filters.html

@@ -27,7 +27,7 @@
  [% END %]
 [% END %]

-<div class="report-list-filters-wrapper govuk-fieldset-wrapper govuk-small">
+<div id="report-list-filters" class="report-list-filters-wrapper govuk-fieldset-wrapper govuk-small">


Don't think this ID is used?

dracos · 2024-11-18T12:38:32Z

templates/web/base/report/display.html

 [% END %]

+[% INCLUDE 'report/display_tools.html' %]
+[% TRY %][% INCLUDE 'report/sharing.html' %][% CATCH file %][% END %]
+[% INCLUDE 'report/updates.html' %]


The get updates and share things are appearing in the wrong place at mobile width for staff - you click either button and the extra stuff appears too far down, underneath the "Provide an update" button rather than above it. And for non-staff, the buttons aren't working at all, nothing happens when clicked.

dracos · 2024-11-18T12:38:50Z

templates/web/base/report/display_tools_mobile.html

@@ -0,0 +1,43 @@
+<div class="shadow-wrap">


This template seems very similar to the non-mobile one, I can combine them later.

dracos · 2024-11-18T12:44:21Z

web/cobrands/fixmystreet/fixmystreet.js

@@ -1293,12 +1323,11 @@ $.extend(fixmystreet.set_up, {

    if ($('.mobile').length) {
        // Make sure we end up with one Get updates link
-        if ($('#key-tools a.js-feed').length) {
+        if ($('#key-tools-mobile a.js-feed').length) {


This change shouldn't be made - the js-feed is still in key-tools on any page it is used (not a report page), and needs to moved from there to sub_map_links.
And I'm afraid that sub_map_links uses a background image, so you then end up with two RSS feed icons being shown.

lucascumsille requested a review from dracos October 7, 2024 08:31

dracos requested changes Oct 22, 2024

View reviewed changes

lucascumsille mentioned this pull request Oct 23, 2024

Shortlist button focusable #5228

Merged

lucascumsille force-pushed the 4560-report-page-meaningful-sequence branch 7 times, most recently from 6c1a42a to 10f277b Compare October 28, 2024 07:16

lucascumsille force-pushed the 4560-report-page-meaningful-sequence branch 6 times, most recently from f02d695 to ea875ee Compare October 31, 2024 12:21

lucascumsille added 3 commits October 31, 2024 15:31

[Icons] Added file with a list of inline SVGs icons

86d457f

A file where you can find all the available icons to use as inline SVGs.

Added accessibility improvements to small_drawer

e112225

lucascumsille force-pushed the 4560-report-page-meaningful-sequence branch from a3b40e3 to 8ea01af Compare October 31, 2024 15:31

lucascumsille added 4 commits October 31, 2024 15:34

fixup! Improve meaningful sequence in report page

1f1d2a5

[Bromley] tidy up report-tools.svg

e406cd3

[Oxfordshire] tidy up report-tools.svg

5d22f4d

Added skip report tools in around page

f5c5dd3

lucascumsille force-pushed the 4560-report-page-meaningful-sequence branch from 8ea01af to f5c5dd3 Compare October 31, 2024 15:35

lucascumsille requested a review from dracos October 31, 2024 15:55

dracos requested changes Nov 18, 2024

View reviewed changes

dracos force-pushed the master branch from d011e81 to e84e4a6 Compare November 20, 2024 09:36

dracos force-pushed the master branch from 5bf556d to bcce64b Compare December 4, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve meaningful sequence in report page #5208

Improve meaningful sequence in report page #5208

lucascumsille commented Oct 7, 2024

codecov bot commented Oct 7, 2024 •

edited

Loading

dracos left a comment

lucascumsille commented Oct 31, 2024 •

edited

Loading

lizettal commented Nov 15, 2024

dracos left a comment

dracos Nov 18, 2024

dracos Nov 18, 2024

dracos Nov 18, 2024

dracos Nov 18, 2024

dracos Nov 18, 2024

dracos Nov 18, 2024

		</p>

		<button type="button" class="close-drawer screen-reader-only" aria-label="Close this dialog"></button>

Improve meaningful sequence in report page #5208

Are you sure you want to change the base?

Improve meaningful sequence in report page #5208

Conversation

lucascumsille commented Oct 7, 2024

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

dracos left a comment

Choose a reason for hiding this comment

lucascumsille commented Oct 31, 2024 • edited Loading

Report page

Around page

lizettal commented Nov 15, 2024

dracos left a comment

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

dracos Nov 18, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2024 •

edited

Loading

lucascumsille commented Oct 31, 2024 •

edited

Loading