Pages Editor: add Tasks page (UI only, non-functional) #6868

shaunanoordin · 2023-10-07T00:25:03Z

PR Overview

Part of: Pages Editor MVP project and FEM Lab super-project
Follow #6836
Staging branch URL: https://pr-6868.pfe-preview.zooniverse.org/lab/1982/workflows/editor/3711?env=staging

This PR adds the Tasks page to the Pages Editor, which should have allowed project owners to add and edit tasks. The original scope of this PR was much larger, with an attempt to make the add Tasks button actually working, but I've stripped that out due to being clunky (see Known Dev Flaw 1 below).

This PR is now primarily a UI/design-only update, with a focus on making sure the HTML works correctly.
Comments on accessibility very welcome
- I've tested with Mac VoiceOver but I'm not sure how a vision-impaired project owner would actually experience the page. e.g. the Edit icon has an aria label that says "Edit Task T1" etc but I couldn't get the voiceover to read the task instructions or task key (e.g. T0) which is the core of the whole thing.
- Maybe I need to make the <li.task-item> focusable?

⚠️ LIMITATIONS

Known dev flaw 1: the "steps list" isn't working AT ALL. It's supposed to list steps in workflow, and list task in each step, but instead at the moment it's listing all tasks in the workflow, assuming each task is one step. Blarp!
- I want to address this in the next PR, when I have to really focus on how "Add New Task" should also 1. add a Step (Page), and 2. add the newly created Task into the Step (Page).
- Follow by answering the question how do you delete Steps (Pages)?
Design feedback 1: the grip-bar (or drag handle, which is a better name that just occurred to me goshdangit) is insufficient for its task of re-arranging steps. An "move up/move down" button could be implemented - I'll need to discuss with Sean about this.
- (Design feedback 2: also to confirm with Sean, grip-bar/drag handle is for rearranging steps... and rearranging tasks should be done in the Edit Single Step modal... but what if user is interested in dragging one Task to another Step? Assume answer is "this isn't an option".)

Status

Ready for review

coveralls · 2023-10-07T00:27:39Z

coverage: 57.173%. remained the same when pulling fd99a46 on pages-editor-pt5 into 7a3c8db on master.

eatyourgreens · 2023-10-07T07:44:29Z

Some design feedback. The only icon here that's styled as a button is the T0, which isn't a button. How do I know, visually, which elements I can click (or tap)?

Visual icons that represent a task key and task type in the editor, along with SVG icon buttons to edit the task.

eatyourgreens · 2023-10-07T07:48:19Z

but I couldn't get the voiceover to read the task instructions or task key (e.g. T0) which is the core of the whole thing.

The list looks fine to me. Were you navigating the page in VoiceOver, or just tabbing through focusable elements? In the first case, VO will read through the list. In the second case, it won't, just as it will be skipped for a sighted keyboard user.

The BBC VoiceOver testing steps are a good guide. They're linked from all frontend monorepo PRs.

eatyourgreens · 2023-10-07T07:51:49Z

WCAG 2.2 came out on Thursday, with a more restrictive definition of accessible drag-and-drop. TPGi has a good summary.

Note that keyboard accessibility alone is not sufficient. Any drag and drop interface should be keyboard accessible (or an alternative interface provided which is), but that in itself is not sufficient to meet 2.5.7; this SC is only concerned with pointer interaction. However text-based input is sufficient to meet this SC, because text input is considered to be mode-agnostic.

Accessible drag-and-drop now needs to support point-and-click, as well as keyboard controls.

I think up/down buttons on individual items would pass the new criteria. I’m not sure if keyboard controls for the draggable regions would, but a keyboard alternative to order the list should still be available.

eatyourgreens · 2023-10-07T08:18:36Z

app/pages/lab-pages-editor/components/TasksPage.jsx

+            <option disabled={true}>Choose starting test</option>
+          </select>
+        </div>
+        <ul className="steps-list">


HTML lists can be labelled, so you can label this list with either aria-label or aria-labelledby.

Here's an example of a list with a linked heading, from project Team pages.

https://github.com/zooniverse/front-end-monorepo/blob/76d0d94d2aa348b3a5ded97b76cff050ef67d3c3/packages/app-project/src/screens/ProjectAboutPage/ProjectAboutPage.js#L100-L119

HTML landmarks can also be labelled, which can be useful for navigating complex pages, like these project builder pages.

Use ctrl-opt-U to open the rotor in VoiceOver, then navigate to Landmarks to see a navigable list of page landmarks.

Noted; (1) I'll read up on the MDN article, and (2) add labels to the lists.

Thanks again - I wasn't aware that lists could/should be labelled.

Depends how complex the page is, to be honest. Sometimes, adding extra labels to lists can make them more confusing.

Here, labelling the list as ‘edit tasks’ or ‘order tasks’ might help explain the purpose of the list.

app/pages/lab-pages-editor/components/TasksPage.jsx

eatyourgreens · 2023-10-07T08:56:19Z

app/pages/lab-pages-editor/components/TasksPage.jsx

+
+// WARNING/TODO: this should be handling steps, not tasks
+function StepItem({ task, taskKey }) {
+  if (!task || !taskKey) return null;


Return an error message, so that a future user isn't looking at a blank component slot, wondering why it didn't load?

eatyourgreens · 2023-10-07T08:58:22Z

app/pages/lab-pages-editor/components/TasksPage.jsx

+function StepItem({ task, taskKey }) {
+  if (!task || !taskKey) return null;
+
+  const text = task.instruction;


Future work: task.instruction is deprecated in favour of the Panoptes translations API.

The APIs for individual tasks are also inconsistent. Some use task.instruction, others use task.question. This has been a pain point in the past.

workflow.strings['tasks.T0.instruction'] is how we get the instruction text now, but that would involve updating the lab to load workflow strings.

👌 Noted, I'll add Panoptes translations to the ToDo. At the very minimum for this PR though, I should at least make that const text = task.instruction || task.question || '???' so we can see more task types when reviewing/testing.

eatyourgreens · 2023-10-07T09:01:51Z

app/pages/lab-pages-editor/components/TasksPage.jsx

+              <i
+                aria-label="Task type: text"
+                className="fa fa fa-file-text-o fa-fw"
+              />


I don't think  can be labelled. Only elements with roles, like images, can be labelled.

Does VoiceOver read this label? Does it show up in the accessibility tree?

Font Awesome recommends  nowadays. Spans can't be labelled either, so I'm not sure what they mean by 'Accessibility' here:

https://fontawesome.com/docs/web/add-icons/how-to

I ran some tests, and on macOS + Chrome:

mac VoiceOver seems to detect "Task type: text" label

The Accessibility panel sees this as a generic element. (Hmm, thinking I should add role=img for this)

Noted that a  may make more sense.

All said though, I'm not too fond of going back to FontAwesome - I dislike how they're gating a lot of icons behind a subscription.

Testing update:

 and  appear to be functionally the same.

Oh, and one thing I missed - the voiceover calls it "Task type: text, EMPTY GROUP"

Adding role="img" sets this to "Task type: text, image"

Adding role="presentation" causes the task key and task question to be read together, e.g. "T0 This is the first task"... which actually sounds good, but misses out on the task type information.

Generic elements can't be labelled and won't be read out by JAWS or NVDA, the two biggest screen readers. I'm pretty sure that's in the ARIA spec, so VO is ignoring the standard here.

This relates to what I was saying about lists. Basically, labels are valid on elements that have a role.

Because the generic role is nameless, the aria-labelledby and aria-label attributes are prohibited. Because the role is generic, the aria-roledescription and aria-brailleroledescription attributes are also prohibited.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/roles/generic_role

eatyourgreens · 2023-10-07T09:05:30Z

app/pages/lab-pages-editor/components/TasksPage.jsx

+      <div className="grip-bar flex-row spacing-bottom-XS">
+        <GripIcon color="#A6A7A9" />
+      </div>


This should be a button, if it’s a drag handle.

You could add a keyboard handler to this button, so that the cursor keys move items up and down in the list, when the drag handle is focused.

Alternatively, make the whole StepItem a focusable, draggable element that responds to the cursor keys and the active pointer. That would be trickier, since each step item contains buttons and form inputs that also need to respond to the pointer and keyboard.

An example of how that can go wrong is zooniverse/front-end-monorepo#5422.

eatyourgreens · 2023-10-09T10:07:11Z

Here's a screen recording where I navigate through the page in VoiceOver on Safari, using ctrl-opt-right arrow. Safari has better integration with VoiceOver than Chrome or Firefox, so results may differ in those browsers. I tried to go slowly, so that you can follow what I'm doing. A real user would zip through the page, and would have the speed of the reader set to 500% or more.

Screen.Recording.2023-10-09.at.10.58.27.mov

A couple of observations:

Some of the text doesn't make a lot of sense without any context eg. #3711 or 'T0'.
The text inputs aren't labelled at all. When enter them I hear 'here' (the last word in the field) then an instruction to type without knowing that the field is already filled out. Labelling text inputs with a placeholder value, like this, is both confusing UX and very bad practice.

eatyourgreens · 2023-10-09T10:11:31Z

Here's a second video, where I bring up the rotor (ctrl-opt-U) then use that to list all the headings, links, form controls and landmarks on the page. Finally, I select the 'main' landmark and jump to it. Hopefully this is easy to follow but ask if you have any questions.

Screen.Recording.2023-10-09.at.11.07.29.mov

shaunanoordin · 2023-10-11T01:36:48Z

Awesome, thanks Jim! 👍 Those video walkthroughs have been very helpful - I now realise my problem was that I didn't know how to navigate properly. I was tab-navigating so I ended up only seeing the focusable elements.

Some follow up from your feedback:

The only icon here that's styled as a button is the T0, which isn't a button. How do I know, visually, which elements I can click (or tap)?

Noted, will bring this up in the design review with Sean this week.

WCAG 2.2 came out on Thursday, with a more restrictive definition of accessible drag-and-drop

Noted, Sean already proposed a new design for up/down arrows.

HTML lists can be labelled, so you can label this list with either aria-label or aria-labelledby.

👍 Steps/Pages list and Tasks list now have labels.

Future work: task.instruction is deprecated in favour of the Panoptes translations API.

Added this to the TODO

I don't think can be labelled. Only elements with roles, like images, can be labelled.

As noted in the conversation, I'm going to change  to 

Update: this seems to work! But I'm going to look at this again ~~tomorrow~~ Thursday morning to make sure I'm not abusing the use of role=img

Some of the text doesn't make a lot of sense without any context eg. #3711 or 'T0'.

Noted, I'll look into this; I think I'll bring it up on the design revision. I assume the assumption here is that Project Owners would understand those numbers/labels from experience... but let me check that, since that's two levels of assuming on my part.

The text inputs aren't labelled at all. When enter them I hear 'here' (the last word in the field) then an instruction to type without knowing that the field is already filled out. Labelling text inputs with a placeholder value, like this, is both confusing UX and very bad practice.

Noted; this is just a placeholder to test out the current design of a Text Task. (A pretty wonky placeholder at that - I think I should set it to disabled.) I have a feeling this will need to be revised further, as they're not supposed to be actual/functional <input> elements, but just presentational elements to indicate to the project owner what kind of Task they just made. 🤔

eatyourgreens

Small note about avoiding redundant text in titles and labels, and avoiding ARIA except where the page can't be used without it.

Overly Accessible is a good post about how adding too much extra text to a page can make it harder to use.

app/pages/lab-pages-editor/components/TasksPage.jsx

Co-authored-by: Jim O'Donnell <[email protected]>

…ards buttons

Co-authored-by: Jim O'Donnell <[email protected]>

shaunanoordin requested a review from a team October 7, 2023 00:25

eatyourgreens self-assigned this Oct 7, 2023

eatyourgreens reviewed Oct 7, 2023

View reviewed changes

app/pages/lab-pages-editor/components/TasksPage.jsx Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 7, 2023

View reviewed changes

eatyourgreens reviewed Oct 11, 2023

View reviewed changes

eatyourgreens approved these changes Oct 11, 2023

View reviewed changes

shaunanoordin and others added 16 commits October 13, 2023 13:36

PagesEditor: add TasksPage

9846f09

Tasks Page: style up

80f5893

TasksPage: more styling

ab6b1dc

TasksPage: add TaskItem, and list tasks

ed7b75d

TasksPage: TaskItem is now StepItem. Expand StepItem to list tasks

2b8ed62

TasksPage: add grip bar

8d416ef

TaskPage: steps is now a list

d46cc90

TasksEditor: tasks now ordered in list

7fe8ca7

TasksPage: markup with sections

82b6270

TasksPages: add proper label for Tasks section

3821757

Co-authored-by: Jim O'Donnell <[email protected]>

TasksPage: use more descriptive variable names

9b81bed

Co-authored-by: Jim O'Donnell <[email protected]>

TasksPage: add aria labels for Steps and Tasks lists

04328b2

TasksPage: add trash button. Rename copy/edit task to copy/edit step

433e8df

TasksPage: add image roles to FontAwesome icons

7cee2d5

TasksPage: add TODO note on task questions/instructions

e1344ff

TasksPage: re-code grip-bar (now grab-handle), add move upwards/downw…

81b9b42

…ards buttons

shaunanoordin force-pushed the pages-editor-pt5 branch from f2e511c to 81b9b42 Compare October 13, 2023 12:36

shaunanoordin and others added 3 commits October 13, 2023 14:41

TasksPage: minor cleanup. Remove unnecessary 'lists' in aria labels

6781aca

TasksPage: remove unnecessary image labels from button icons

555060e

Co-authored-by: Jim O'Donnell <[email protected]>

TasksPage: remove unnecessary role=img on some buttons

fd99a46

shaunanoordin merged commit f1276e1 into master Oct 13, 2023
5 checks passed

shaunanoordin deleted the pages-editor-pt5 branch October 13, 2023 13:55

shaunanoordin mentioned this pull request Oct 20, 2023

Pages Editor: implement basic "Add Tasks" functionality #6888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pages Editor: add Tasks page (UI only, non-functional) #6868

Pages Editor: add Tasks page (UI only, non-functional) #6868

shaunanoordin commented Oct 7, 2023 •

edited

Loading

coveralls commented Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 7, 2023

eatyourgreens commented Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023

shaunanoordin Oct 10, 2023

eatyourgreens Oct 10, 2023

eatyourgreens Oct 7, 2023

eatyourgreens Oct 7, 2023

eatyourgreens Oct 7, 2023

eatyourgreens Oct 7, 2023

shaunanoordin Oct 10, 2023

eatyourgreens Oct 7, 2023

eatyourgreens Oct 7, 2023

shaunanoordin Oct 11, 2023

shaunanoordin Oct 11, 2023

shaunanoordin Oct 11, 2023

eatyourgreens Oct 11, 2023

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 9, 2023 •

edited

Loading

eatyourgreens commented Oct 9, 2023 •

edited

Loading

shaunanoordin commented Oct 11, 2023

eatyourgreens left a comment

Pages Editor: add Tasks page (UI only, non-functional) #6868

Pages Editor: add Tasks page (UI only, non-functional) #6868

Conversation

shaunanoordin commented Oct 7, 2023 • edited Loading

PR Overview

Status

coveralls commented Oct 7, 2023 • edited Loading

eatyourgreens commented Oct 7, 2023

eatyourgreens commented Oct 7, 2023 • edited Loading

eatyourgreens commented Oct 7, 2023 • edited Loading

eatyourgreens Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

eatyourgreens commented Oct 9, 2023 • edited Loading

eatyourgreens commented Oct 9, 2023 • edited Loading

shaunanoordin commented Oct 11, 2023

eatyourgreens left a comment

Choose a reason for hiding this comment

shaunanoordin commented Oct 7, 2023 •

edited

Loading

coveralls commented Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens Oct 7, 2023 •

edited

Loading

eatyourgreens commented Oct 9, 2023 •

edited

Loading

eatyourgreens commented Oct 9, 2023 •

edited

Loading