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

Tab order based on distance from center #596

Merged
merged 12 commits into from
Dec 17, 2021

Conversation

ahmadayubi
Copy link
Member

@ahmadayubi ahmadayubi commented Nov 24, 2021

2021-11-24.14-29-04_Trim.mp4
  • Add tests / adjust tests for new behavior
  • Update popup navigation button functionality
  • Prevent features off screen from being tab focused

Closes #385

@ahmadayubi
Copy link
Member Author

@prushforth
Assigning a tab index to features while the map has a taxindex = 0 means the feature gets focused before the map. Is this the intended/accepted behavior?

@prushforth
Copy link
Member

Is this the intended/accepted behavior?

My first impression is that the map should always come first. It would be acceptable to skip the controls and go straight to the features, maybe.

@ahmadayubi
Copy link
Member Author

ahmadayubi commented Nov 24, 2021

Is this the intended/accepted behavior?

My first impression is that the map should always come first. It would be acceptable to skip the controls and go straight to the features, maybe.

To get this behavior you either have to;

  1. set the tabindex of the map to 1 (other side effects)
  2. remove the tabindex values when the map is not focused and re-add tabindex only when the map is focused (more computation)

@Malvoz
Copy link
Member

Malvoz commented Nov 24, 2021

I think 2 is the only option, we don't want to break the tab order of elements on people's websites.

@Malvoz
Copy link
Member

Malvoz commented Nov 24, 2021

Would this be compatible with #571?

@ahmadayubi
Copy link
Member Author

Would this be compatible with #571?

No I believe that's getting shelved for now

@prushforth
Copy link
Member

Could we establish a private focus order index value that we follow, instead of setting the tabindex > 0? We could handle keystrokes while in a sequence of features and set focus to the next highest/ next lowest private focus order index, perhaps

@prushforth
Copy link
Member

I was looking for where @Malvoz mentioned using arrow keys instead of tab keys within a group of features. If we implemented that, we wouldn't be hacking the tabindex, I think.

@Malvoz
Copy link
Member

Malvoz commented Nov 24, 2021

I was looking for where @Malvoz mentioned using arrow keys instead of tab keys within a group of features.

That's #535.

@prushforth
Copy link
Member

So, I think we should be using tabindex=-1 and setting a private focus index value that we programmatically use to set the focus on a feature. The private value can be ordered ascending by distance?? Maybe the first feature in the DOM should be tabindex=0 and the rest -1? WDYT?

@ahmadayubi
Copy link
Member Author

That would work, does handling the tab event ourselves have any benefit over adding tabindex when the map is focused and removing it once the map is out of focus?

@prushforth
Copy link
Member

Good question, but if we don't change the tabimdex in an antisocial way, it won't matter if our program crashes and can't change it back? Maybe

@prushforth
Copy link
Member

Maybe the first feature in the DOM should be tabindex=0 and the rest -1? WDYT?

Maybe the first feature in each layer should be tabindex=0, with the rest =-1...

@ahmadayubi
Copy link
Member Author

2021-11-27.14-37-56.mp4

So I opted to use event handlers as this would also further improvements in the future brought up by @prushforth regarding arrow keys.

All the features in the map are considered rather than on a per layer basis. The highest tabindex value used is now 0, never exceeding this.

The final thing remaining is treating subparts (spans within coordinates) as independent when sorting, currently it treats subparts distance from the center as equal to the distance of the main component.

@ahmadayubi ahmadayubi marked this pull request as ready for review December 10, 2021 22:03
@ahmadayubi
Copy link
Member Author

The functionality should be reviewed before the tests are updated to reflect the new behavior. So if anyone has time please share any feedback or bugs.

@prushforth
Copy link
Member

prushforth commented Dec 11, 2021

Looking pretty nice! A couple of things I noticed.

  1. for https://maps4html.org/experiments/screenreader/restaurants/ page, if you build your branch and put the dist into a local clone of experiments, it sorts the restaurants very nicely, but if you zoom, you then have to tab twice to get past a marker. If you zoom twice, I think you have to tab 3 times to get past a marker, and the number of times you have to tab grows as you change zoom.

  2. in this page with the sorting build in dist https://maps4html.org/experiments/screenreader/closest-restaurants/ if you click the Restaurants button, you'll get the following error:

image

I'll continue to look for stuff....

@ahmadayubi
Copy link
Member Author

@prushforth The issues you mentioned should be fixed with the latest update, good finds!

@prushforth
Copy link
Member

@ahmadayubi the issues seem mostly fixed. I was able to reproduce the first problem:

  1. for https://maps4html.org/experiments/screenreader/restaurants/ page, if you build your branch and put the dist into a local clone of experiments, it sorts the restaurants very nicely, but if you zoom, you then have to tab twice to get past a marker. If you zoom twice, I think you have to tab 3 times to get past a marker, and the number of times you have to tab grows as you change zoom.

but not as bad, by first loading the restaurants, then tabbing through them, then zooming in and tabbing a few times, then right-click the restaurants layer entry and press "Zoom to layer". The multiple-tabs-to-get-past-a-marker problem occurs.

@ahmadayubi
Copy link
Member Author

but not as bad, by first loading the restaurants, then tabbing through them, then zooming in and tabbing a few times, then right-click the restaurants layer entry and press "Zoom to layer". The multiple-tabs-to-get-past-a-marker problem occurs.

Hmm okay, it's related to the layer removal, I thought I accounted for it but maybe different map updates run through different removal paths. Will take a look.

@ahmadayubi
Copy link
Member Author

@prushforth Turns out the reason it needed multiple tab presses is because the features are duplicated in the DOM. The bug is related to zoomToLayer in combination with TemplatedFeaturesLayer, where the _pullFeatureFeed is called twice (through the moveend handler) instead of once, not sure why yet.

@prushforth
Copy link
Member

I have a feeling that might be due to a change I made during template features processing. So it's an unrelated bug to this.

@ahmadayubi ahmadayubi mentioned this pull request Dec 15, 2021
prushforth added a commit to prushforth/experiments that referenced this pull request Dec 17, 2021
Hopefully won't mess up history too badly, should get overwritten when
that PR is merged.
prushforth added a commit to Maps4HTML/experiments that referenced this pull request Dec 17, 2021
Hopefully won't mess up history too badly, should get overwritten when
that PR is merged.
Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

LGTM!

This change on its own would make it difficult for screen reader users to discover that new features have become available after pan/zoom, but it's an important piece of the puzzle. And it all comes together with #636.

Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

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

Looks good to me. When tabbing through many features, we do need a way to skip to the end of the sequence so as to not trap the user. This may come into play with feature indexing too. I look forward to implementing arrow key feature navigation as a way of leaving tabs to inter-control/interactive element navigation.

@prushforth prushforth merged commit 4d57c2a into Maps4HTML:main Dec 17, 2021
@prushforth
Copy link
Member

Boom!

@Malvoz Malvoz mentioned this pull request Dec 28, 2021
2 tasks
prushforth added a commit to Maps4HTML/experiments that referenced this pull request Jan 12, 2022
…e linking example. (#92)

* Update dist to pre-integrate work from Maps4HTML/MapML.js#596
Hopefully won't mess up history too badly, should get overwritten when
that PR is merged.

* [AUTO] Sync MapML Build

* [AUTO] Sync MapML Build

* Share restaurants across examples. Add tabindex to building part example.

Update RTL experiment to use shared restaurants.mapml
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.

Assign tabindex=n in ascending order of distance of feature from map centre
3 participants