adaptlearning / adapt-contrib-narrative

A component that displays an image gallery with accompanying text
GNU General Public License v3.0
5 stars 39 forks source link

resolve page scroll when user selects last item #232

Closed chris-steele closed 1 year ago

chris-steele commented 1 year ago

To resolve #231

oliverfoster commented 1 year ago

Could you try removing these instead please?

https://github.com/adaptlearning/adapt-contrib-narrative/blob/268cf26e4a1f8a67a6841b4cea922acfba300d6c/js/NarrativeView.js#L273-L274

It's probably because the focus is going from the button. Assigning it elsewhere usually causes the screen reader to read the a11y-focuser element. It was admittedly pretty bad practise for me to include that element / suggest that technique in the first place.

guywillis commented 1 year ago

Both solutions resolve the page jump.

The suggestion from Ollie displays the previous button on the first slide and the next button on the last slide. Whilst selecting these buttons causes no errors within the build it is potentially confusing from a UI perspective and almost certainly confusing from a screen reader perspective - the buttons can be tabbed into and are read out.

Tested with NVDA.

oliverfoster commented 1 year ago

Both solutions resolve the page jump.

The suggestion from Ollie displays the previous button on the first slide and the next button on the last slide. Whilst selecting these buttons causes no errors within the build it is potentially confusing from a UI perspective and almost certainly confusing from a screen reader perspective - the buttons can be tabbed into and are read out.

Tested with NVDA.

They should be read out as disabled.

oliverfoster commented 1 year ago

@guywillis Let us know about the nvda disabled buttons when you get a chance pls

guywillis commented 1 year ago

NVDA does not read the buttons as disabled.

When testing, I have only removed these two lines as mentioned above:

https://github.com/adaptlearning/adapt-contrib-narrative/blob/268cf26e4a1f8a67a6841b4cea922acfba300d6c/js/NarrativeView.js#L273-L274

Screenshot of the narrative controls in the DOM for the first item of three.

Screenshot 2022-07-29 at 11 50 42
oliverfoster commented 1 year ago

@chris-steele can you replace

https://github.com/adaptlearning/adapt-contrib-narrative/blob/268cf26e4a1f8a67a6841b4cea922acfba300d6c/js/NarrativeView.js#L273-L274

with

    a11y.toggleEnabled($left, !isAtStart);
    a11y.toggleEnabled($right, !isAtEnd);

instead? narrative

guywillis commented 1 year ago

Works a charm.

Tested with NVDA

chris-steele commented 1 year ago

Done and thanks both

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 7.2.2 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: