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

Implemented right-click context menu #1030

Merged
merged 9 commits into from Feb 18, 2017

Conversation

Projects
None yet
6 participants
@danstowell
Contributor

danstowell commented Aug 9, 2015

Here's a context menu implemented for the main map view. Fixes #251. I think a context-menu click is a nice way to interact with the (relatively new) directions and query-here tools.

The exact choice of items is a matter that can be debated, but that aside, I'd like to ask if this approach seems broadly acceptable.

Notes:

  • The browser's built-in context menu remains accessible, by holding shift while right-clicking.
  • The "Add note here" option adds it in not-quite the right place, unless you also merge #1028 in which case it works correctly.
  • I haven't yet done the i18n - wanted to get dev feedback before I do that.

screenshot - 090815 - 21 13 17

@HolgerJeromin

This comment has been minimized.

Contributor

HolgerJeromin commented Aug 9, 2015

Could you add an entry with no action but the text:
shift right click for browser's default menu.
Or is this no good idea?

@HolgerJeromin

This comment has been minimized.

Contributor

HolgerJeromin commented Aug 9, 2015

The same menu on a touch long hold would be nice.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Aug 9, 2015

I'd just drop the shift right click thing to be honest - nobody is ever going to find it and I'm not aware of there being any convention to do that when implementing a custom menu.

@danstowell

This comment has been minimized.

Contributor

danstowell commented Aug 10, 2015

@tomhughes thanks. Could you clarify why you'd drop the shift-click thing please? You said in #251 "The big problem with right click menus is that they prevent access to the normal browser right click menu" so I'm sorta surprised. I do understand why you say "nobody is ever going to find it" but on the other hand (a) Firefox uses this shift-click everywhere, so even though it's obscure it's partly conventional, and (b) I'm uncomfortable with fully preventing access to the normal browser menu (on non-firefox, that is). I don't think the shift-click thing has any notable drawbacks (e.g. CPU load), and it's a tiny amount of code.

@danstowell

This comment has been minimized.

Contributor

danstowell commented Aug 10, 2015

@HolgerJeromin thanks. Personally I don't favour the idea of adding documentation strings as fake menu entries, though maybe others would disagree with me. I don't have a touch device here but I believe the underlying library should already work with touch-long-hold. Maybe you were just suggesting, but if you tried it and it didn't happen, grateful for details.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Aug 10, 2015

@danstowell Can you explain what you mean about Firefox using it everywhere? Are you saying Firefox does this by default and always shows the normal menu on a shift click?

@tomhughes

This comment has been minimized.

Member

tomhughes commented Aug 10, 2015

The leaflet extension should be in vendor, not app, given that it is a third party component. Use Vendorfile and the vendorer command to manage it.

@danstowell

This comment has been minimized.

Contributor

danstowell commented Aug 10, 2015

@tomhughes yes that's correct, Firefox always goes straight to the default context-menu on shift-right-click. (It can be prevented with some ugly hacking, I think, if someone is deliberately trying to suppress the default.) Disable on shift is mentioned in passing here as an example: https://html.spec.whatwg.org/multipage/forms.html#context-menus "User agents may..."

@HolgerJeromin

This comment has been minimized.

Contributor

HolgerJeromin commented Aug 10, 2015

@danstowell right now a "long hold" does nothing on osm mainpage. But i agree with you, that the leaflet lib seems to do something with touchstart event. So probably is will just work as i suggested.

@danstowell

This comment has been minimized.

Contributor

danstowell commented Aug 17, 2015

(done "vendorer" fix, thanks)

@danstowell

This comment has been minimized.

Contributor

danstowell commented Oct 12, 2015

Just checking up on this PR, I notice that Travis is raising a Javascript issue which is caused by my "tidy" decision to refactor the context-menu callbacks into a separate file:

% bundle exec rake jshint
app/assets/javascripts/index.js: line 90, col 32, 'context_directionsfrom' is not defined.
app/assets/javascripts/index.js: line 93, col 32, 'context_directionsto' is not defined.
app/assets/javascripts/index.js: line 96, col 32, 'context_addnote' is not defined.
app/assets/javascripts/index.js: line 99, col 32, 'context_describe' is not defined.
app/assets/javascripts/index.js: line 102, col 32, 'context_queryhere' is not defined.
app/assets/javascripts/index.js: line 105, col 32, 'context_centrehere' is not defined.
app/assets/javascripts/index/contextmenu.js: line 1, col 7, 'context_describe' is defined but never used.
app/assets/javascripts/index/contextmenu.js: line 9, col 7, 'context_directionsfrom' is defined but never used.
app/assets/javascripts/index/contextmenu.js: line 19, col 7, 'context_directionsto' is defined but never used.
app/assets/javascripts/index/contextmenu.js: line 29, col 7, 'context_addnote' is defined but never used.
app/assets/javascripts/index/contextmenu.js: line 39, col 7, 'context_centrehere' is defined but never used.
app/assets/javascripts/index/contextmenu.js: line 43, col 7, 'context_queryhere' is defined but never used.

If anyone has any advice about the organisation of that bit of code, I'd be grateful. I could drop the refactor (omit commit 35fbcf2).
(IIUC the contextmenu needs to be added during the new L.OSM.Map() call rather than afterwards, which is what limited my refactoring choices.)

@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

@danstowell and @tomhughes : What's the reason that this pull request stalled? I'm getting regurarly user feedback asking for this right click function, especially "Show address" including WGS84 coordinates.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

Well the jshint errors to start with - the problem is that jshint doesn't understand the server side inclusion but we shouldn't be adding new functions at global scope anyway so a way needs to be found to avoid that before this can be merged.

@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

Actually, the feature which let me chime in was "Show coordinates" at mouse position. In fact that could be also implemented as part of an slightly enhanced "Query features" function (I'm probably going to open another issue on that).

Regarding this pull request, I agree with you: didn't realize the server side inclusion dependency. But what do you mean by "we shouldn't be adding new functions at global scope"? Is there a function freeze?

Actually I'd like to summarize if I (we) understand and agree on the same spec.: Actions are "Right click" on desktop and "Touch long hold" on touch screens/mobiles. Menu items are:

Directions from here
Directions to here
--
Add a note here
Show address
Query features
Centre map here
Default menu

?

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

No it's just bad style to create javascript variables and functions at global scope because you have no idea when they might clash with some other bit of javascript you're pulling in.

Good design is to isolate stuff in anonymous functions to control name visibility. I'd need to look at this in detail to see the best way to do it but I'm sure it's possible.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

A better design here is probably to add the items to the menu after the map is created, and use anonymous functions, like:

map.contextmenu.addItem({
  text: 'Label',
  callback: function (e) {
    .....
  }
});
@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

Sorry, my misunderstanding: I fully agree with you regarding good coding style and locally scoped functions. I still was on a "higher" level, trying to speak about the specification of the right click (or shift right click?) function in the website.

@danstowell

This comment has been minimized.

Contributor

danstowell commented Feb 12, 2017

Hi folks - I fully agree that it's best to avoid the global namespace where possible. I attempted (in 2015) to do it the way Tom describes but I remember it not working, for reasons I couldn't fathom.

At this point, I think my javascript skills are not up to the task of getting this tidy enough to be acceptable, though I personally think it's a valuable feature. If anyone would like to propose ways to get this polished up, or to take it over, I'd be happy to join in or to relinquish. Send PRs to my branch if you like, or anything.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

I've done the refactoring in tomhughes@30c2b41 and also a subsequent commit to make the menu entries translatable and that code is currently running at http://tomh.apis.dev.openstreetmap.org/ if people want to try it...

@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

@tomhughes That's great! I testet all right click menu items on Firefox 51.0.1 (32 bit, Win) and it worked (without being logged in). On Chrome Version 56.0.2924.87 (Win) the right click menu appeared, but then I got no action whatever item I tried.

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

I've updated Leaflet.contextmenu to the latest version which I think has fixed the Chrome issue.

@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

Works now on my two browsers mentined above.
Only one issue (on both): Given there's no (left) sidebar open, when clicking on "Add a note here" the map get's centered about 50 pixels aside to the right.
Side note: Centering map is unnecessary from users POV, but probably due to OSM note fn. ?

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

There's a comment in the source about that - it presumably needs some server side work to allow the location to be passed.

@sfkeller

This comment has been minimized.

sfkeller commented Feb 12, 2017

Cool stuff anyway! Thanks @tomhughes !!

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

Implemented in tomhughes@99b75f2 is the logic to allow a note to be placed at the cursor without panning.

It will still pan slightly if the note is very close to the edge, and in particular if it would otherwise have been underneath the side bar!

@danstowell

This comment has been minimized.

Contributor

danstowell commented Feb 12, 2017

Ace, thanks Tom. All working here - testing on Ubuntu with Firefox 51.0.1 and Chromium 55.0.

One thing I noticed: "Add a note here" is always available, even when zoomed out (when it's deliberately deactivated by the usual UI). What it does is pops up the add-a-note panel but with no marker icon, and the submit button greyed out. Would be better if "Add a note here" was only active when sufficiently zoomed.

Conversely, "centre map here" is unavailable at low zoom, and I'm not sure why that would be the case.

@dieterdreist

This comment has been minimized.

dieterdreist commented Feb 12, 2017

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

Hmm either your indexes were wrong then or I managed to reorder the menu items... I'll check....

@danstowell

This comment has been minimized.

Contributor

danstowell commented Feb 12, 2017

Yes, it appears my indexing was wrong back in a2cf9b5 when I added the zoom-level dependence!

@tomhughes

This comment has been minimized.

Member

tomhughes commented Feb 12, 2017

Should be fixed now.

@HolgerJeromin

This comment has been minimized.

Contributor

HolgerJeromin commented Feb 12, 2017

Working nice on Opera Mobile on Android

@tomhughes tomhughes merged commit 6b04511 into openstreetmap:master Feb 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.086%
Details
@sommerluk

This comment has been minimized.

sommerluk commented Mar 23, 2017

Great work. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment