New: Add option to make entire menu items clickable (fixes #157)

[swashbuck]

Fixes #157

New

• Adds the _areEntireItemsClickable option to make entire menu items clickable
  • The "View" button will be hidden when this is true
  • This is a course-level setting rather than content object level (although we could move it)
• Updated documentation and schemas

Testing

1. Add the _areEntireItemsClickable option to the _boxMenu object in course.json.
2. Click anywhere on the menu item to go to the content object.

Screenshots

[swashbuck]

Ok, after seeing this in action, I will close this issue and PR. The main problem I'm seeing is that you would have to add all of the menu item information to the same aria label (title, body, duration, all the statuses, etc.), which can make it excessively long and complex. With the View button, a screenreader user can tab through each element one at a time so as to not be overwhelmed with information. The body text is the real killer since it can be much longer than a typical title.

I think this could still work, but, yeah, probably better as a completely new menu plugin. Navigating through thumbnails on Hulu could be closer to what we actually would want since you can navigate to elements within each thumbnail.

[oliverfoster]

Ok, after seeing this in action, I will close this issue and PR. The main problem I'm seeing is that you would have to add all of the menu item information to the same aria label (title, body, duration, all the statuses, etc.), which can make it excessively long and complex. With the View button, a screenreader user can tab through each element one at a time so as to not be overwhelmed with information. The body text is the real killer since it can be much longer than a typical title.

I think this could still work, but, yeah, probably better as a completely new menu plugin. Navigating through thumbnails on Hulu could be closer to what we actually would want since you can navigate to elements within each thumbnail.

I'm not sure it has to all go in an aria-label. If you follow the a tag examples (<div role='link'></div>):

<a href="#/">This is readable</a>

Reference: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#technical_summary

Just need to pay attention to how it reads with a screen reader.

[swashbuck]

I'm not sure it has to all go in an aria-label. If you follow the a tag examples:

<a href="#/">This is readable</a>

Ok, maybe I'll keep tinkering with this one.

[swashbuck]

@oliverfoster Do you have any idea why the skip navigation focus bug is happening (see details above under "To-do (bug fix)")?

Bug: When _areEntireItemsClickable is set to true and the user clicks a menu item for the first time, the "Skip Navigation" button becomes focused on the next page

[oliverfoster]

Yeas, I have a theory. It's that a <div role=link> is not a natively focusable/clickable element, it really needs a tabindex="0" on it to make it focusable and behave more buttony.

This is roughly how the browser and Adapt's accessibility stuff determines what is focusable:
https://github.com/adaptlearning/adapt-contrib-core/blob/de6fcd9c58ad7ed0d793920b048fcb213bdcc980/js/a11y.js#L43-L50

https://developer.mozilla.org/en-US/docs/Web/Accessibility/Understanding_WCAG/Keyboard

[swashbuck]

Yeas, I have a theory. It's that a <div role=link> is not a natively focusable/clickable element, it really needs a tabindex="0" on it to make it focusable and behave more buttony.

Thanks, @oliverfoster . Adding a tabindex attribute seems to have fixed it.

Changes have been made

[oliverfoster]

It looks like a much smaller change than it appeared originally. What does everyone think?

[oliverfoster]

I really don't like this setting name. I have feelsies it should be something like "_useTilesNotButtons" or "_useTiles" but I can't quite put my finger on it. I don't suppose it matters that much.

[swashbuck]

Yeah, I'm not committed to this setting name. Could even be as simple as _hideItemButtons or _hasItemButtons.

[kirsty-hames]

I've used _boxMenuItemIsClickable when doing similar on projects.

[swashbuck]

@kirsty-hames @oliverfoster I like _boxMenuItemIsClickable. Shall I change it to that?

[oliverfoster]

No other property in box menu is prefixed with _boxMenu.

Summary:
The behaviour hides the button and makes the item clickable. It is a layout alteration. The proposed names so far only describe either removing the button or making the item clickable, not both, which is more important? Is there another concept that encapsulates both changes? We haven't provided a rationale as to why one would enable/disable this property and choose the layout change.

Suggestion:
_isItemClickable perhaps? To keep it simple.

[kirsty-hames]

Hey @swashbuck, I was giving this an accessibility test and I'm unable to enter a menu item using keyboard controls. I tested this across Mac Safari, Chrome and FF doing the following:

• No screen reader running. Keyboard tab to menu item and select enter.
• Running Mac VoiceOver. Keyboard tab to menu item and select enter.
• Running Mac VoiceOver. Command + option + keyboard arrows to navigate to menu item. Enter / space bar to select.

I'm unable to test if this is the case for Windows too.

I'm assuming this is down to the .boxmenu-item__inner being a <div> rather than a <button> element. When I've done this on past projects, I've made .boxmenu-item__inner a <button> with role="link" then you don't need tabindex="0". See snippet below.

{{#if _boxMenuItemIsClickable}}
<button class="menu-item__inner boxmenu-item__inner js-btn-click"{{#if _isLocked}} aria-disabled="true"{{/if}}>
  <span class="aria-label">
    {{#if _isComplete}}{{_globals._accessibility._ariaLabels.complete}}. {{else _isVisited}}{{_globals._accessibility._ariaLabels.visited}}. {{/if}}{{#if _isLocked}}{{_globals._accessibility._ariaLabels.locked}}. {{else}}{{linkText}} {{/if}}{{title}}. {{{compile _globals._menu._boxMenu.itemCount}}}.{{#if _isOptional}} {{_globals._accessibility._ariaLabels.optional}}
  </span>
{{else}}
<div class="menu-item__inner boxmenu-item__inner">
{{/if}}

Thoughts on doing something similar to above?

[guywillis]

Personally, I'm not really that keen on the change.

Adapt has generally been moving towards having clickable elements to buttons to give consistent identification, visual and non visual end users, and to create a consistent approach for development. This change adds a click functionality to the <div> element which seems counter to the general approach when we look towards examples already in Adapt - Accordion, Narrative controls, Hotgraphic pin etc.

In order for the change to not be counter to the previous examples we'd need to convert the <boxmenu-item__inner> element to a button and all children to inline elements, such as <span>, to adhere to the w3c validator.

Example of previous conversion work:
adaptlearning/adapt-contrib-hotgraphic#230
adaptlearning/adapt-contrib-hotgraphic#229

The required changes to achieve the desired outcome, personally, will combine to make a more time consuming and complicated template to maintain.

I do maintain that I think this feature is better suited as a separate plugin.

[swashbuck]

I was giving this an accessibility test and I'm unable to enter a menu item using keyboard controls.

@kirsty-hames The issue was that the only event was a click event. I've added a keydown event in 0310042, so it should now work when navigating to the item and hitting Space or Enter.

Will you retest to see if this resolves the issue?

[swashbuck]

Adapt has generally been moving towards having clickable elements to buttons to give consistent identification, visual and non visual end users, and to create a consistent approach for development. This change adds a click functionality to the

element which seems counter to the general approach when we look towards examples already in Adapt - Accordion, Narrative controls, Hotgraphic pin etc.

Thanks for your opinion, @guywillis . I would argue that we do not necessarily need to use semantic elements like <button> as long as we use a proper role="link" or role="button" attribute ("link" in this case). There was a lengthy discussion about this on #158 and I think that applies here.

[kirsty-hames]

I was giving this an accessibility test and I'm unable to enter a menu item using keyboard controls.

@kirsty-hames The issue was that the only event was a click event. I've added a keydown event in 0310042, so it should now work when navigating to the item and hitting Space or Enter.

Will you retest to see if this resolves the issue?

I was giving this an accessibility test and I'm unable to enter a menu item using keyboard controls.

@kirsty-hames The issue was that the only event was a click event. I've added a keydown event in 0310042, so it should now work when navigating to the item and hitting Space or Enter.

Will you retest to see if this resolves the issue?

Thanks @swashbuck. I've retested this and the focus is now trapped on the first menu item. This is also the case when "_areEntireItemsClickable": false, except focus is trapped on the 'View' button. I'm unable to tab backwards or forwards.

[oliverfoster]

Summary so far

Arguments against

1. The syntax. Using a div as a clickable region isn't nice and is contrary to other historic places where we've intentionally removed clickable divs in favour of buttons. If we were to exchange the clickable div with a button (ideal) we would need to replace all children elements with span (as button cannot contain divs), making this pr substantially more complicated and probably unviable. The implementation as it is will probably fail accessibility testing on technicality and html standards if rectified @guywillis comment @kirsty-hames comment
2. Code bloat, reduces the simplicity of boxmenu @guywillis comment1, comment2

Arguments for

1. Clickable divs and buttons are effectively identical, no span replacements are required @swashbuck comment
2. Allows more menu styles to be more easily derived from the boxmenu @swashbuck comment

Points of note

1. Alternate menu layouts (netflix, etc) built atop of these changes would need an amount of custom styling @swashbuck comment
2. This change is partly included to circumvent multimenu AAT instances and the maintenance of a second menu @swashbuck comment
3. Placement of status and information labels (completion, locked, duration, etc) are complicated by this pr @StuartNicholls

The alternative to this pr would be

To have a separate menu, based on this one, for a specific graphic/tile styles with the possibility for defined/further layout options and innate styling @swashbuck

[swashbuck]

This PR will now need updates to boxMenuItem.jsx now that the JSX conversion is complete.

[StuartNicholls]

I agree with Ollie, I'd much prefer these to be proper HTML buttons with span tags inside the button. I also agree that it would make the PR a little too complicated as I think it's a really good idea to keep the boxmenu simple. Happy to have a separate menu with this type of functionality.