Page MenuHomePhabricator

Current checkbox hack doesn't provide <Enter> or <Space> as toggle action
Closed, ResolvedPublic3 Estimated Story Points

Description

Descoped prerequisite: T257075: Simplify the checkboxHack API

Current checkbox hack in action on latest Vector doesn't react to <Enter> or <Space>.
This should toggle the checkbox too.

Acceptance Criteria

  • Label that behaves as button is triggerable
  • With JavaScript enabled, Sidebar shows/hides in when focused and the <Enter> key is pressed or the <Space> key is released
  • With JavaScript disabled and before the asynchronous JS is loaded, the Sidebar continues to show/hide in response to clicking the sidebar button, but does not react on keypress

QA Results - Beta|Prod

ACStatusDetails
1T254851#6335131
2T254851#6335131
4T254851#6335131

QA additional notes

  • Holding the <Enter> key only changes the Sidebar state once, not repeatedly

It's been agreed on that we remove this edge case from the acceptance criteria in T254851#6349847

QA Results - Beta|Prod

ACStatusDetails
1T254851#6363104
2T254851#6363104
4T254851#6363104

Related Objects

StatusSubtypeAssignedTask
ResolvedGoalovasileva
OpenNone
Resolvedovasileva
ResolvedSpikeovasileva
ResolvedSpikephuedx
Resolvedovasileva
OpenSpikeVolker_E
OpenNone
Resolvedovasileva
OpenDemian
ResolvedSpikeovasileva
Resolvedovasileva
ResolvedBUG REPORTmatmarex
Resolvedovasileva
ResolvedJdlrobson
Resolvedphuedx
Resolvednray
ResolvedMayakp.wiki
ResolvedMayakp.wiki
Stalledovasileva
OpenNone
ResolvedEdtadros
OpenNone
OpenNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva triaged this task as Medium priority.Jun 9 2020, 5:46 PM
Jdlrobson added a comment.EditedJun 12 2020, 4:24 PM

Why does space work but not enter? Why is enter preferable to space?
From Volker (in another thread): "it's about the behaviour as a button" - as soon as we turn something into a button <Enter> is expected to work on it too

Got it.

@Jdlrobson For completion, even though the focus seems to sit on the label visually, it's actually on the checkbox and therefore <Space> works inherently.

Demian added a subscriber: Demian.EditedJun 12 2020, 6:54 PM

Although not focused, the<label> element also applies label:focus {...} default focus style when the <input> is focused. Reasonable, but can be misleading.

Although not focused, the<label> element also applies label:focus {...} styles when the <input> is focused. Reasonable, but can be misleading.

When would that be misleading in relation to this task?

By the expectation of which element receives the key event.

ovasileva raised the priority of this task from Medium to High.Jun 23 2020, 4:40 PM
nray renamed this task from Current checkbox hack doesn't provide <Enter> as toggle action to Current checkbox hack doesn't provide <Enter> or <Space> as toggle action.Jun 23 2020, 4:40 PM
nray lowered the priority of this task from High to Medium.
nray raised the priority of this task from Medium to High.
nray updated the task description. (Show Details)
Demian claimed this task.Jun 23 2020, 4:59 PM

Change 607164 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Add setCheckedState() function

https://gerrit.wikimedia.org/r/607164

Change 606538 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: Handle SPACE and ENTER key on button (label) to change checkbox state

https://gerrit.wikimedia.org/r/606538

Change 606541 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [modern] Handle SPACE and ENTER key on checkbox button (label)

https://gerrit.wikimedia.org/r/606541

Volker_E updated the task description. (Show Details)Jun 24 2020, 12:44 AM

Change 607164 merged by jenkins-bot:
[mediawiki/core@master] checkboxHack: Add setCheckedState() function

https://gerrit.wikimedia.org/r/607164

https://gerrit.wikimedia.org/r/606541 is finalized with different handling (trigger) for SPACE and ENTER that follows exactly the behaviour of buttons.
Minor difference: :active state is not set while SPACE is pushed. That would require a <button> element.

Volker_E updated the task description. (Show Details)Jun 25 2020, 2:43 AM

Change 607651 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] Put ARIA roles and attributes into context

https://gerrit.wikimedia.org/r/607651

@Demian We need to toggle a dynamic -active class for the button additionally to the keyboard treatment. Currently it's not completely off without through the button icon change, but it's also not very friendly UX.

In a related issue, the checked attribute doesn't get updated. You are targeting this with https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/606306/…?!

@Demian We need to toggle a dynamic -active class for the button additionally to the keyboard treatment. Currently it's not completely off without through the button icon change, but it's also not very friendly UX.

I don't understand this sentence.

In a related issue, the checked attribute doesn't get updated. You are targeting this with https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/606306/ ?!

No. The checked attribute represents only the default value when the page is loaded.

Change 607651 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Put ARIA roles and attributes into context

https://gerrit.wikimedia.org/r/607651

ovasileva set the point value for this task to 3.Jun 29 2020, 5:27 PM

Change 608744 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: only one CheckboxHack() call to initialize

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608744

Demian added a subscriber: Krinkle.EditedJul 1 2020, 10:16 AM

@Volker_E (re: 606541).
CC @Jdrewniak (for sidebar change hook).

EDIT: comment moved to its own ticket T257075: Simplify the checkboxHack API

Demian added a comment.EditedJul 1 2020, 2:13 PM

Re: 606306

EDIT: comment moved to T257075#6277950

@Demian the changes proposed to the checkboxHack API are quite substantial, and that amount of work certainly deserves it's own task,
so I made one here T257075 and added it to the code-review column on the Readers Web board.

I think continuing the conversation there will give the team greater visibility into these changes.

Demian added a comment.Jul 3 2020, 9:53 PM

@Demian the changes proposed to the checkboxHack API are quite substantial, and that amount of work certainly deserves it's own task,
so I made one here T257075 and added it to the code-review column on the Readers Web board.
I think continuing the conversation there will give the team greater visibility into these changes.

Thank you @Jdrewniak for making that ticket, I agree wholly.

Demian updated the task description. (Show Details)Jul 6 2020, 3:12 AM
Demian added a project: User-Demian.
Demian moved this task from Incoming to Awaiting Review on the User-Demian board.

Change 610053 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] checkboxHack: [squashed] Handle SPACE and ENTER key and simplify CheckboxHack() API

https://gerrit.wikimedia.org/r/610053

Change 613095 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/core@master] Remove keyup event from checkbox hack.

https://gerrit.wikimedia.org/r/613095

I found a bug with this implementation. When clicking the "jump to navigation" skip link, the sidebar changes state. The patch above provides a fix by removing the keyup event binding on the checkbox hack. I'm not 100% sure why that event binding causes this to happen, but removing it prevents this behaviour.

I think it was put in place to prevent scrolling when activating the sidebar menu button with the spacebar, but I don't see any scrolling happening without it. Maybe the role=button prevents the scrolling on spacebar keypress as well?

Jdrewniak removed Edtadros as the assignee of this task.Jul 16 2020, 9:14 AM
Jdrewniak added a subscriber: Edtadros.
Jdrewniak added a comment.EditedJul 20 2020, 10:23 AM

I did a bit of research as to why this happens. I wasn't aware of this browser behaviour, but apparently, when you click on an internal link <a href="#target">menu link</a> the target element <h2 id="target">over here</h2> actually gets the keyup event if it's focusable. Since our menu button uses tabindex=0 to force focus, it receives the keyup event that originates from pressing the "skip to navigation" skip link.

I made a codepen illustrating this behaviour: https://codepen.io/j4n/pen/KKVbzgV

Basically, the focus moves to the target on the keydown event, so I think the solution of removing the keyup event from the checkbox hack is still valid.

This comment was removed by Jdrewniak.

Change 613095 merged by jenkins-bot:
[mediawiki/core@master] Remove keyup event from checkbox hack.

https://gerrit.wikimedia.org/r/613095

in terms of testing before the javascript has loaded, the Chrome devtools provide a neat network throttling capability

that can be used to slow the page down so you'll have plenty of time to test the sidebar behaviour before JS loads. 3G plus disabling caching should provide an adequate scenario, and testing on very long pages will also help (since the JS loads only after all the html has finished).

Edtadros reassigned this task from Edtadros to Jdrewniak.EditedJul 25 2020, 5:25 AM

Test Result - Beta

Status: ❌ FAIL
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: Label that behaves as button is triggerable
See AC4.
✅ AC2: With JavaScript enabled, Sidebar shows/hides in when focused and the <Enter> key is pressed or the <Space> key is released


❌ AC3: Holding the <Enter> key only changes the Sidebar state once, not repeatedly

✅ AC4: With JavaScript disabled and before the asynchronous JS is loaded, the Sidebar continues to show/hide in response to clicking the sidebar button, but does not react on keypress

Edtadros updated the task description. (Show Details)Jul 25 2020, 5:27 AM
Volker_E added a comment.EditedJul 27 2020, 8:59 PM

It's unclear, if staying on the Enter button and it re-triggering is sufficient problem.
AFAIO there are no clear HCI guidelines that would support uninterrupted pressing of the key for triggering on/off even as requirement.
Think this should be put to "resolved" as is until further notice. cc: @Jdrewniak

@Edtadros We might have an acceptance criteria understanding problem: Button should not be triggered more than once even though you stick on the <Enter> key. This looks like the expected behavior from many user-interfaces and is fulfilled from my point of view with Jan's latest merged patch. Please confirm that understanding and let's move this on. :)

After further conversation with @Jdrewniak where he brought up for consideration to “[…] change line 266 button.addEventListener( 'keydown', onToggleOnSpaceEnter, true ); to keyup instead [in CheckboxHack.js]” we've agreed that

  • it's a too specific edge case without clear side-effects and
  • keydown is used widely in our interfaces; see for example Special:RecentChanges where practically all interaction elements are triggered on keydown

in order to accept more code changes.
We will change the acceptance criteria instead accordingly.

In case we gather more input or user-feedback is flagging this we might reopen for new consideration.

Volker_E updated the task description. (Show Details)

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: Label that behaves as button is triggerable
See AC4.
✅ AC2: With JavaScript enabled, Sidebar shows/hides in when focused and the <Enter> key is pressed or the <Space> key is released


⬜ AC3: Holding the <Enter> key only changes the Sidebar state once, not repeatedly
No longer in scope per T254851#6349847
✅ AC4: With JavaScript disabled and before the asynchronous JS is loaded, the Sidebar continues to show/hide in response to clicking the sidebar button, but does not react on keypress

Edtadros reassigned this task from Edtadros to ovasileva.Aug 5 2020, 3:45 PM
Edtadros updated the task description. (Show Details)
ovasileva closed this task as Resolved.Aug 7 2020, 10:08 AM

Looks good!

ovasileva updated the task description. (Show Details)Aug 7 2020, 10:08 AM