Band-aid solution to make edit buttons focusable

[Ryuno-Ki]

Partially addressing #19769

These will need more work to get done properly.
However, this will require more communication, which might mean
that a fix may or may not make it into the next release.

To improve the situation at least a little bit, allow for
keyboard usage.

I noticed that the :focus styling is lost on the cancel button here.
Inspecting the root cause led to a regression caused by #19763
I'd like to have @silverwind look into this if possible, as I lack the
context that led to the mentioned PR. (namely: why !importants?)

Regarding the usage of <div>s: That was never changed here.
I would assume, there is no specific reason. Should I change the
markup to a proper <button type="button"> instead? Or are there
side effects I should be aware of?

Signed-off-by: André Jaenisch [email protected]

[delvh]

Regarding the usage of <div>s: That was never changed here.
I would assume, there is no specific reason. Should I change the
markup to a proper <button type="button"> instead? Or are there
side effects I should be aware of?

As @silverwind once told me:

Keep it simple. If something is not a <button>, it will have a11y problems and should be changed to such.

So it is most likely unintended that they are not buttons.
But keep one thing in mind:

According to my quick search (ag 'class=".*button.*"' | grep -v '<button' | wc -l), there are 335 of those... Have fun fixing all of them.

(Both quotes have been copied from #19330)

[silverwind]

I noticed that the :focus styling is lost on the cancel button here.

Probably just an oversight. Suggesting to add this CSS to restore/improve focus/hover styles. !important is to ensure it override fomantic's styles):

.ui.basic.secondary.buttons .button:hover,
.ui.basic.secondary.button:hover,
.ui.basic.secondary.buttons .button:active,
.ui.basic.secondary.button:active {
  color: var(--color-secondary-dark-8) !important;
  border-color: var(--color-secondary-dark-4) !important;
}

.ui.basic.secondary.button:focus,
.ui.basic.secondary.buttons .button:focus {
  color: var(--color-secondary-dark-8) !important;
  border-color: var(--color-secondary-dark-6) !important;
}

Should I change the
markup to a proper instead? Or are there
side effects I should be aware of?

Yes, <button> would be preferred (not sure if type is needed) and it's better for accessibilty too. If it works, go ahead. You might need to change some JS selectors.

[Ryuno-Ki]

Thank you for the quick response.
The evening is already late here, so I will update my PR in the next days (likely tomorrow).

[wxiaoguang]

According to https://fomantic-ui.com/elements/button.html

Could these code be used instead of role or div?

<button class="ui primary basic button">Primary</button>
<button class="ui secondary basic button">Secondary</button>
<button class="ui positive basic button">Positive</button>
<button class="ui negative basic button">Negative</button>

[silverwind]

According to https://fomantic-ui.com/elements/button.html

Could these code be used instead of role or div?

<button class="ui primary basic button">Primary</button>
<button class="ui secondary basic button">Secondary</button>
<button class="ui positive basic button">Positive</button>
<button class="ui negative basic button">Negative</button>

Yes that will work. role=button is unnecessary on <button>, the only case where it should be used is <a role="button">.

[silverwind]

BTW I think if you make it a <button>, adding tabindex will no longer be necessary as the element is naturally focusable then.

[Ryuno-Ki]

Took me a while longer (managing three kids during holidays is stressful, let me tell you …).

@delvh

Have fun fixing all of them.

That is another opportunity to onboard contributors :-)

@silverwind

Suggesting to add this CSS to restore/improve focus/hover styles.

Thanks, picked them up and included them. I also removed the tabindex.

Could these code be used instead of role or div?

They could and they should.
<button> naturally have an implicit aria-role="button", so should not be added.
Using anything else would require to implement these patterns: https://www.w3.org/WAI/ARIA/apg/patterns/button/

[Ryuno-Ki]

While testing 5f999d8 manually I observed, that the cancel button seem not to react to return presses. It did that before.

Debugging it yielded that the registered event handler gets executed twice (spacebar only once). I was not able to fix that.

[wxiaoguang]

Maybe it could be shorten to:

Suggested change

	<button id="cancel-edit-title" class="ui basic secondary in-edit button" style="display: none" type="button">{{.i18n.Tr "repo.issues.cancel"}}</button>
	<button id="cancel-edit-title" class="ui basic secondary in-edit button" hidden>{{.i18n.Tr "repo.issues.cancel"}}</button>

And there is a similar <div class=button> on line 5

[Ryuno-Ki]

How strong do you think about this?

[wxiaoguang]

Personally I would choose the better / more ideal solutions as much as possible.

Since these code is being refactored and these buttons (including the line 5) work together, so they could be improved together.

[wxiaoguang]

While testing 5f999d8 manually I observed, that the cancel button seem not to react to return presses. It did that before.

Debugging it yielded that the registered event handler gets executed twice (spacebar only once). I was not able to fix that.

Maybe related to the legacy code (which is not ideal ....)

gitea/web_src/js/features/common-global.js

Lines 70 to 76 in edf1420

	export function initGlobalButtonClickOnEnter() {
	$(document).on('keypress', '.ui.button', (e) => {
	if (e.keyCode === 13 || e.keyCode === 32) { // enter key or space bar
	$(e.target).trigger('click');
	}
	});
	}

[Ryuno-Ki]

Maybe related to the legacy code (which is not ideal ....)

I would expect the same behaviour before then as well. But it was executed only once with the <div> implementation.

[wxiaoguang]

Maybe related to the legacy code (which is not ideal ....)

I would expect the same behaviour before then as well. But it was executed only once with the <div> implementation.

Yes, because the button has a native event handler for Enter. So the Enter will trigger the native event handler and the customized event handler -- twice.

[wxiaoguang]

Replaced by #22807