Skip to content

Conversation

@nicvagn
Copy link
Contributor

@nicvagn nicvagn commented Jun 25, 2025

The change of label to heading is to aid in navigation with screen reader.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

This is a improved version of my past NVUI Retro PR.

addresses - "Learn from your mistakes” feature is not accessible in Blind Mode (NVUI) #17621

@schlawg
Copy link
Contributor

schlawg commented Jun 25, 2025

ok, i just ask because it's an existing behavior we're changing and people get used to the element structure that exists. it can be jarring even when the new one is an improvement.

each additional heading will lengthen the element summary panel, which blind users do rely on. excessive h4-h6 tags may be discouraged for this reason.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

I understand that. I will revert.

My effort to aid in accessibility was misguided.
@schlawg
Copy link
Contributor

schlawg commented Jun 25, 2025

"request a computer analysis" seems to have gotten broke somewhere along the many curves of this marathon. there should be an announcement.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

image
There is OMM. Or am I misunderstanding you?

Just tested on lichess.org. You mean on asking, right?

@schlawg
Copy link
Contributor

schlawg commented Jun 25, 2025

i'm not sure what OMM is. there does not seem to be any confirmation upon selecting "request a computer analysis", so it's not clear whether anything is happening. if that behavior exists on lichess.org as well, apologies for blaming this PR but we still need to fix it.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

On my machine. Yes I thought initially you where referring to the acknowledgement of completion.

No I figure out the code that was responsible for sending it prior.


const requestAnalysisButton = (
  ctrl: AnalyseController,
  inProgress: Prop<boolean>,
  notify: (msg: string) => void,
 ): MaybeVNode =>
   ctrl.ongoing || ctrl.synthetic
     ? undefined
     : inProgress()
      ? h('p', 'Server-side analysis in progress')
      : h(

But I can not say that I understand it very well.

@schlawg
Copy link
Contributor

schlawg commented Jun 25, 2025

ah, that seems important yes. one other little thing is we prefer class based styling over inline style attribute. if you wanted to ensure that labels inside of buttons were always click through (for sighted testing), you could do so in ui/round/css/_nvui.scss with:

label {
  pointer-events: none;
}

i believe that file is included in this page so styles defined there should be available.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

Thanks for the expertise. This is the sorta incite I am surly lacking.

I was also including that encase some other assistive technology generated pointer-events.

I do not see a downside, and possible upside.

I am working on the notify.

@schlawg
Copy link
Contributor

schlawg commented Jun 25, 2025

we're getting closer. once you've addressed the issues above, please try to reduce the number of 'aria-live' directives. we have gone from 4 in the original page to 18 on this one, i don't think they're all necessary - especially the ones on container elements or headings/labels that don't change.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 25, 2025

Ok, yes I agree with that.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 26, 2025

Thank you sincerely for reviewing my code and helping me to contribute. @schlawg

@nicvagn nicvagn force-pushed the NVUI-learn-from-your-mistakes branch from a70b4da to fbad115 Compare June 26, 2025 00:15
@schlawg
Copy link
Contributor

schlawg commented Jun 26, 2025

cool, thanks. i'm going to put this up on a test server so ikrami can look it over without being rushed tomorrow.

thanks for working on it.

Copy link
Contributor Author

@nicvagn nicvagn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure you had good reason, but indulge me. Why the doubling back?

@schlawg schlawg force-pushed the NVUI-learn-from-your-mistakes branch from bf99868 to 2a3d14f Compare June 28, 2025 18:06
@schlawg
Copy link
Contributor

schlawg commented Jun 28, 2025

i wanted you to be able to continue to work without me constantly dumping updates into your repo, so i started a draft PR based on your work. it seems you may have been waiting for me and i'm done for now, so i've pushed my branch back into your PR.

i think we can merge this assuming yafred and allan sign off.

@schlawg schlawg requested review from allanjoseph98 and yafred June 28, 2025 18:13
@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 28, 2025

Yes, I think so. I code like I'm bipolar manic. I am either putting in 13 hour days, or silent.

I was waiting for some test feedback, or something. I can see, and have helped build the interface.

So I do not really know what works and does not, for usability.

This is exactly what caused me to put heading everywhere.

I didn't want to make an assume out of myself.

It would be great to get some feedback from screen reader users.

What works and what doesn't. What would make a better experience, etc.

Due to lack of data, I don't feel like I can make any changes confidently.

@nicvagn
Copy link
Contributor Author

nicvagn commented Jun 28, 2025

Did you get a longer term instance going?

@ikrami1
Copy link

ikrami1 commented Jun 28, 2025 via email

@schlawg
Copy link
Contributor

schlawg commented Jun 28, 2025

Did you get a longer term instance going?

it's live on https://testy.lichess.dev

today only!

@yafred
Copy link
Contributor

yafred commented Jun 28, 2025

Considering the feature is usable and the only things I don't understand in the code were there before, I'd say let's go for it!

@schlawg
Copy link
Contributor

schlawg commented Jun 29, 2025

we can wait until allan / thibault has a chance to weigh in. @ikrami1 were you able to check this with JAWS?

Copy link
Member

@allanjoseph98 allanjoseph98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Full support for the modularisation.

If ikrami says it's good enough then it's good enough for me.

@ikrami1
Copy link

ikrami1 commented Jun 29, 2025

@allanjoseph98 it is definitely good enough for me. i was complaining that the board doesn't flip automatically according to the color being reviewed— however, a currently draft PR by the wonderful @yafred is going to add this feature with a shortcut across the entire nvui.

@schlawg it works with jaws, i tested it briefly and anouncements were coming through nicely. usually, whatever works with nvda on the web works with jaws.

@yafred
Copy link
Contributor

yafred commented Jun 29, 2025

We can still do the automatic flip afterwards ... sighted users have both: flip on demand with f and automatic flip when analysing the opponent's mistakes.

schlawg and others added 4 commits June 29, 2025 15:10
Co-authored-by: Allan Joseph <53343585+allanjoseph98@users.noreply.github.com>
Co-authored-by: Allan Joseph <53343585+allanjoseph98@users.noreply.github.com>
Co-authored-by: Allan Joseph <53343585+allanjoseph98@users.noreply.github.com>
@schlawg
Copy link
Contributor

schlawg commented Jun 29, 2025

there is both manual flip 'f' and automatic flip (review other color mistakes) in this PR.

@schlawg schlawg merged commit 30a7ddc into lichess-org:master Jun 29, 2025
4 checks passed
@schlawg
Copy link
Contributor

schlawg commented Jun 29, 2025

thanks nicvagn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants