Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[accessibility] Consider replacing aria-pressed with aria-expanded on the attribution control #359

Closed
Malvoz opened this issue Sep 18, 2021 · 6 comments

Comments

@Malvoz
Copy link
Contributor

Malvoz commented Sep 18, 2021

Regarding:

_toggleAttribution() {
if (this._container.classList.contains('mapboxgl-compact-show')) {
this._container.classList.remove('mapboxgl-compact-show');
this._compactButton.setAttribute('aria-pressed', 'false');
} else {
this._container.classList.add('mapboxgl-compact-show');
this._compactButton.setAttribute('aria-pressed', 'true');
}
}

aria-pressed is similar to aria-expanded in that it indicates the state of the button, but aria-expanded is more appropriate as it specifically:

Indicates whether the element, or another grouping element it controls, is currently expanded or collapsed.

The distinction is that with aria-expanded the (non-visual) user will expect other ("new") content to be available after pressing the button. This is mostly important as there is no focus delegation (which is fine if it's conveyed to the user that new content is available through aria-expanded).


(The same issue was also reported with MapBox: mapbox/mapbox-gl-js#11034.)

@astridx
Copy link
Contributor

astridx commented Oct 21, 2021

@Malvoz Great that you looked at a11y improvements. Regarding aria-pressed and aria-expanded, I would like to ask you something. Isn't it the case that aria is more of a workaround and ideally you would use correct HTML elements if they exist.

Wouldn't MapLibre use the HTML elements details and summary more correctly and in this case would not need aria at all?

Original with aria-pressed and aria-expanded

Closed

<div class="maplibregl-ctrl-bottom-right mapboxgl-ctrl-bottom-right">
    <div class="
            maplibregl-ctrl maplibregl-ctrl-attrib
            mapboxgl-ctrl mapboxgl-ctrl-attrib
            maplibregl-compact
            mapboxgl-compact
        ">
        <button class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" type="button"
            title="Toggle attribution" aria-label="Toggle attribution">
        </button>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner" role="list">
            Desiged with ♥️

        </div>
    </div>
    <div class="
            maplibregl-ctrl maplibregl-ctrl-attrib
            mapboxgl-ctrl mapboxgl-ctrl-attrib
        ">

        <button class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" type="button"
            title="Toggle attribution" aria-label="Toggle attribution">
        </button>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner" role="list">
        </div>
    </div>
</div>

Open

<div class="maplibregl-ctrl-bottom-right mapboxgl-ctrl-bottom-right">
    <div
        class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib maplibregl-compact mapboxgl-compact maplibregl-compact-show mapboxgl-compact-show">
        <button class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" type="button"
            title="Toggle attribution" aria-label="Toggle attribution" aria-pressed="true">
        </button>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner" role="list">Desiged with ♥️
        </div>
    </div>
    <div class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib">
        <button class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" type="button"
            title="Toggle attribution" aria-label="Toggle attribution">
        </button>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner" role="list">
        </div>
    </div>
</div>

Changed Version with details and summary

Closed

<div class="maplibregl-ctrl-bottom-right mapboxgl-ctrl-bottom-right">
    <details
        class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib maplibregl-compact mapboxgl-compact">
        <summary class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" title="Toggle attribution"
            aria-label="Toggle attribution">
        </summary>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner">Desiged with ♥️
        </div>
    </details>
    <details class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib">
        <summary class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" title="Toggle attribution"
            aria-label="Toggle attribution">
        </summary>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner">
        </div>
    </details>
</div>

Open

<div class="maplibregl-ctrl-bottom-right mapboxgl-ctrl-bottom-right">
    <details
        class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib maplibregl-compact mapboxgl-compact maplibregl-compact-show mapboxgl-compact-show"
        open="">
        <summary class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" title="Toggle attribution"
            aria-label="Toggle attribution">
        </summary>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner">Desiged with ♥️
        </div>
    </details>
    <details class="maplibregl-ctrl maplibregl-ctrl-attrib mapboxgl-ctrl mapboxgl-ctrl-attrib">
        <summary class="maplibregl-ctrl-attrib-button mapboxgl-ctrl-attrib-button" title="Toggle attribution"
            aria-label="Toggle attribution">
        </summary>
        <div class="maplibregl-ctrl-attrib-inner mapboxgl-ctrl-attrib-inner">
        </div>
    </details>
</div>

@Malvoz
Copy link
Contributor Author

Malvoz commented Oct 21, 2021

@astridx that's true, HTML is preferred over ARIA where possible.

For example, markers should ideally be <button> as opposed to <div role="button"> (#356). But such changes would require more effort (to reset the styles of these elements) which I don't want to impose, even though that's usually not very hard to achieve.

@astridx
Copy link
Contributor

astridx commented Oct 22, 2021

@Malvoz Thank you very much for your answer.

I have submitted this change here #557. Maybe you would like to have a look at it. I have included these changes as an example in the zip file attached here. Do you think the attribute control is now fine in terms of accessibility?

Maplibre.zip

@acalcutt
Copy link
Contributor

Do you think this aria-pressed with aria-expanded change may have broken the attributions when in full screen?

I am testing the Terrain3d branch that is being worked on here ( https://github.com/prozessor13/maplibre-gl-js/commits/terrain3d ) and notice my attributions are no longer showing when in full screen (or above a width of about 615px). they show properly as a button when the screen is smaller (below 615px), but full screen they don't show at all. Looking at the change log I think it might have started when the main branch with this change was merged in.

Latest version at above 615px, missing attributes
attrib_missing

Latest version below 615px, the attributes show up as a button like they should
attrib_smaller

This is how it looked when it was working on the previous version (which i compiled 10/19) above 615px, all my attribution show as a list at the bottom, until the screen gets below a certain size (around 615px) and it turns into a button

attrib_working

@acalcutt
Copy link
Contributor

I tested reverting the changes in attribution_control.ts here and it does in fact fix my full screen attribution issue

@Malvoz
Copy link
Contributor Author

Malvoz commented Nov 14, 2021

Do you think this aria-pressed with aria-expanded change may have broken the attributions when in full screen?

Just noting that the original suggested solution was not used.

Closing this issue as it is no longer relevant per #557 (and because you've raised #640 for the issue you're seeing).

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

No branches or pull requests

3 participants