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

Disable compass button mouse/touch handlers when map#interactive is false #8643

Closed
wants to merge 1 commit into from

Conversation

ryanhamley
Copy link
Contributor

Launch Checklist

@ryanhamley ryanhamley requested a review from peterqliu August 15, 2019 18:32
@asheemmamoowala
Copy link
Contributor

@ryanhamley Will the compass button look differently when the map is not interactive? How would a user know that the button is not going to do anything?

@andrewharvey
Copy link
Collaborator

andrewharvey commented Aug 16, 2019

I think that when interactive is false:

  1. the button should be disabled, so that the cursor doesn't change to the pointer, and so that you can't tab select it

  2. We should reset the title="Reset bearing to north" aria-label="Reset bearing to north" since these are no longer true when you can't click on the button.

  3. We should disable its :hover effect. The hover effect indicates you can click on the button, but not when it's disabled.

@ryanhamley
Copy link
Contributor Author

I think @andrewharvey is right. I got a little ahead of myself on this PR. I'd say that this should apply to all 3 navigation control buttons (zoom in/out and compass) as well as geolocate, but it should not apply to the full screen control or any other built in control.

@peterqliu
Copy link
Contributor

peterqliu commented Aug 16, 2019

@ryanhamley there are many ways to do this and your current one totally works, but the most concise method is probably a new CSS class that conditionally appends to the whole navigation module with pointer-events:none;, overwrites hover states, and fades out/hides/somehow indicates the zoom buttons are inactive.

This way, all you have to do in JavaScript is toggle the class and set the title aria.

@peterqliu peterqliu added the breaking change ⚠️ Requires a backwards-incompatible change to the API label Aug 16, 2019
@andrewharvey
Copy link
Collaborator

We should reset the title="Reset bearing to north" aria-label="Reset bearing to north" since these are no longer true when you can't click on the button.

Except now I'm second guessing myself, maybe the title and aria-label should be left in, as they help tell the user what the button would do, if it were enabled. Otherwise you get this disabled button which sighted users can see as a compass/zoom in/zoom out and see that's it is disabled, but unsighted users see there is a disabled button there but don't know what it is for without the title/aria-label.

So maybe just disabled is enough and the label should remain.

@pathmapper
Copy link
Contributor

So maybe just disabled is enough and the label should remain.

Then there would be still a tooltip for the compass icon, right?

It could confuse sighted users if there is a tooltip "Reset bearing to north" but it's not possible to reset the bearing. So if a title/label should remain, it should be changed to something else.

There should be no fade out for the compass icon, because it still has a purpose when map#interactive is set to false (visualising bearing and pitch, the latter if the visualizePitch option is set).

@andrewharvey
Copy link
Collaborator

Then there would be still a tooltip for the compass icon, right?

It could confuse sighted users if there is a tooltip "Reset bearing to north" but it's not possible to reset the bearing. So if a title/label should remain, it should be changed to something else.

Yes that was my original thinking that the label was confusing since it says the button will reset the bearing, but if you try to click nothing happens so it doesn't really do that.

But on the other hand having no label isn't an option because sighted users can see the + - and compass icon, but even when disabled we need to communicate through text those symbols for accessibility.

There should be no fade out for the compass icon, because it still has a purpose when map#interactive is set to false (visualising bearing and pitch, the latter if the visualizePitch option is set).

But if it's a button then if you can't click on it or clicking does nothing, then it should be have the disabled attribute right?

@peterqliu
Copy link
Contributor

But if it's a button then if you can't click on it or clicking does nothing, then it should be have the disabled attribute right?

the compass still has value as an indicator -- the reset north functionality is arguably ancillary, even-- so visibility is still important. disabled would fade it out a bit

@pathmapper
Copy link
Contributor

disabled would fade it out a bit

Maybe this could be avoided using the :disabled CSS pseudo-class to style the disabled compass the same as the enabled one.

@ryanhamley
Copy link
Contributor Author

I'm going to close this for the time being while we consider #9036. This issue/PR would be moot if we simply removed the NavigationControl's interactivity entirely

@ryanhamley ryanhamley closed this Dec 16, 2019
@pathmapper
Copy link
Contributor

This issue/PR would be moot if we simply removed the NavigationControl's interactivity entirely

@ryanhamley agree, but if I understood it right, #9036 is about removing only the drag rotation on NavigationControl compass, not about removing the interactivity entirely.

So after #9036 the issue (#8618) would still exist with the Reset bearing to north functionality of the compass:
https://jsfiddle.net/2j7qp86h/

reset_bearing_to_north

@ryanhamley
Copy link
Contributor Author

ryanhamley commented Dec 17, 2019

@pathmapper Good point, I forgot about that functionality. I do think I'll keep this closed for the time being because we have a number of refactors planned in the near-to-mid-term around mobile gestures and accessibility so this may work better as part of that push. This probably should be revived when we do those refactors though so that the reset bearing functionality is disabled with map#interactive: false. cc @vakila

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Requires a backwards-incompatible change to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable compass interactivity for interactive: false maps
5 participants