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.

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

In T254851#6220117, @Demian wrote:

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.

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

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.

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/…?!

In T254851#6255405, @Volker_E wrote:

@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

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

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

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

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.

In T254851#6277851, @Jdrewniak wrote:

@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.

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?

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.

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).

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.

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

Change 610053 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] checkboxHack: [squashed] Handle SPACE and ENTER key and simplify CheckboxHack() API

Reason:

The functional changes have already been done in T254851, the API changes would need to be rewritten for the current code.

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

Change 606538 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] checkboxHack: Handle SPACE and ENTER key on button (label) to change checkbox state

Reason:

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