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 a standard title attribute for the control buttons #2781

Merged
merged 2 commits into from
Oct 30, 2014

Conversation

fredj
Copy link
Member

@fredj fredj commented Oct 1, 2014

Instead of a (IMHO overcomplicated) custom span element.

As a bonus, it makes the use of the bootstrap plugin very easy (the plugin uses the title attribute by default)

@tonio
Copy link
Member

tonio commented Oct 1, 2014

Sorry, but I don’t like this change. Current implementation allows (without external plugins) the display of tooltip without delay (unlike title) & even if buttons are focused with the keyboard.

Those changes were made for accessibility, & are IMHO useful.
But I’d be happy to hear others opinions about it.

@adube
Copy link
Contributor

adube commented Oct 1, 2014

If I may way in: jqueryui has the same kind of feature, i.e. use the title attribute to display a tooltip. I has an issue on ios, when the element is an input. See http://bugs.jqueryui.com/ticket/9349.

@tschaub
Copy link
Member

tschaub commented Oct 1, 2014

Coincidentally, I was just going to open a ticket for discussion about the current control tooltips. My preference would be to have them be "opt in" instead of "opt out." I appreciate the accessibility benefits they currently bring, but I'd still rather see us provide an easy way to add control tips rather than make the decision that they should be shown by default.

And for me, "provide an easy way" could be "show an example that uses an external library or straight dom" (as we do for popups).

@fredj
Copy link
Member Author

fredj commented Oct 1, 2014

What I dislike about the current implementation is it's too opinionated.
For example the tooltips are immediately displayed, the zoom control has to be on the left side of the map otherwise the tooltips are not visible, etc ...

I know that it's relatively easy to customize this behavior with css but it implies too much of work.

My opinion is that we have to provide a very minimalist implementation and let the users use their own tools.

@fredj fredj force-pushed the button-title branch 3 times, most recently from 2119ef3 to c6e0a42 Compare October 8, 2014 10:04
@fredj fredj force-pushed the button-title branch 4 times, most recently from 8bd9c06 to 81e4466 Compare October 28, 2014 14:20
@fredj
Copy link
Member Author

fredj commented Oct 28, 2014

I've updated the custom-controls example to show how to customize the tooltips with bootstrap

@tschaub
Copy link
Member

tschaub commented Oct 28, 2014

@fredj how about creating a dedicated example demonstrating how to add tooltips to controls (instead of doing this in custom-controls)?

Curious what others think about this. I understand @tonio's perspective. But having just spent a lot of time undoing/redoing ol3's tooltip (and attribution control) handling, I also like the idea of limiting our DOM manipulation to the absolute minimum and letting people make choices that integrate well with the rest of their application.

@elemoine
Copy link
Member

I agree with @tschaub.

@fredj
Copy link
Member Author

fredj commented Oct 29, 2014

ok, dedicated example added

@elemoine
Copy link
Member

+1

@fredj
Copy link
Member Author

fredj commented Oct 30, 2014

Thanks for the review

fredj added a commit that referenced this pull request Oct 30, 2014
Use a standard title attribute for the control buttons
@fredj fredj merged commit 2d53eb7 into openlayers:master Oct 30, 2014
@fredj fredj deleted the button-title branch October 30, 2014 15:52
@tschaub
Copy link
Member

tschaub commented Oct 30, 2014

Looks good to me as well. I appreciate the effort @tonio put into the design and functionality on this. But I'm in favor of moving to just using the title attribute.

Curious if others have opinions about aria-label vs. text - but this can be handled separately.

fredj added a commit to fredj/openlayers that referenced this pull request Aug 3, 2015
It was a workaround added in openlayers#1761 to hide the button tooltip on
OSX / Chrome 32 and 33.

Because we're not using the tooltips anymore (see openlayers#2781) and this
version of chrome is deprecated this workaround can be removed.
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

5 participants