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

use button elements instead of anchor elements and add tooltips to zoomcontrol #1708

Closed
wants to merge 2 commits into from
Closed

use button elements instead of anchor elements and add tooltips to zoomcontrol #1708

wants to merge 2 commits into from

Conversation

mprins
Copy link
Contributor

@mprins mprins commented Feb 15, 2014

TL;DR: if it is a button it should be a button.

Using the correct semantics serves accessiblity and adding a tooltip (which is screen reader accessible) makes the button easier to understand for everyone. Also proper focus handling is important for keyboard users since the have no :hover event

the tooltip can be easily disabled by removing the :focus and :hover pseudo classes or using some simple css to override those.

more background in: http://www.geodienstencentrum.nl/blog/accessibility/webmapping/2014-02-14/enhancing-openlayers-controls.html

@elemoine
Copy link
Member

Very nice. Thanks. @fredj also did some work on that a while back. IIRC we decided against using buttons because styling buttons consistently across different browsers was not an easy problem. I'll have a closer look at your patch.

@tonio
Copy link
Member

tonio commented Feb 17, 2014

I just take a quick look:

  • Zoom, ZoomToExtent, Fullscreen controls share (or at least should share, see my wip branch) their styles : I think if we go this way, all 3 must be treated the same way.
  • you set the border color for the IE background color, but not for standard browser (rgba color).
  • there are some unwanted dimension changes, see before/after:

capture decran 2014-02-17 a 09 20 09

@mprins
Copy link
Contributor Author

mprins commented Feb 17, 2014

@elemoine it seems an error to me to not use the proper element. Choosing to do that because it doesn't look exactly the same on every platform makes form subordinate to function.

@mprins
Copy link
Contributor Author

mprins commented Feb 17, 2014

@tonio I think you've got a good point here, but since this is about accessibility I think it's out of scope. (even though these controls need a work over since they are actually empty hyperlinks using :after)

Can you please describe what you mean with the dimension changes?

I think it would be nice to cascade (parts) of a style, but to make that easier I would look into something like Sass. Using a css pre-processor also gives the advantage of optimizing the css during build. For an example of what this could look like see: https://sourceforge.net/p/dokuwikispatial/code/HEAD/tree/trunk/openlayersmap/src/scss/

eg. shared.css for commonly needed layout and a .css per control that optionally extends the base class in the shared file. I really think the current css file is rather convoluted with all those :not[ie] selectors, I you want to have additional support for a single browser, specifically IE, it should go in a conditionally loaded style-sheet and not be carried as a burden across all other browsers...

@tonio
Copy link
Member

tonio commented Feb 18, 2014

@mprins I’m pursuing your work here

  • I rebase your work on my CSS refactoring branch,
  • made some appearance tuning,
  • switch to an «em» based button size,
  • make tip content configurable.

I’ll continue adapting the other controls the same way.

@tonio
Copy link
Member

tonio commented Feb 19, 2014

see #1725

@mprins
Copy link
Contributor Author

mprins commented Feb 19, 2014

OK, that's cool.

@mprins
Copy link
Contributor Author

mprins commented Feb 20, 2014

closing this in favour of #1725

@mprins mprins closed this Feb 20, 2014
@mprins mprins deleted the a11y-enhanced-zoomcontrol branch May 26, 2014 14:11
@mprins mprins restored the a11y-enhanced-zoomcontrol branch December 18, 2014 09:01
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.

None yet

3 participants