Community
Participate
Working Groups
We need to provide API to show the context menu in the editor area and rulers. This API should include a show callback passing the x,y location and the target (ruler or editor).
see bug 334590 for issues with clipboard action (that any context menu should have).
For Mozilla integration we need minimal support for custom context menus. In XUL environments we have our code that handles contextmenu behavior, we just need the Orion editor component to allow us to use it. For that purpose I made a change to Orion. Here is the pull request: https://github.com/eclipse/orion.client/pull/1 This bug report is about adding API for custom context menu support. This patch does that, but it's not as exhaustive, I believe, as planned in this bug report: - no support for rulers at the moment. I can add it, if it's a requirement for us to land this patch. - we give the event handler the original contextmenu DOM event object, which is probably something that was not intended. I presume you want a custom event object? For Mozilla's project I only needed the original DOM event with the screenX/screenY properties. If this is a requirement to land the patch, please let me know how you would like the custom event object to be (what properties and so on) - we'll obviously need screenX and screenY. I did not add custom contextmenu support for the rulers because currently the contextmenu event is not canceled on the rulers, which is good. Orion only cancels the event inside the editor widget. We can handle the ruler events, for now. Please let me know what further changes I need to make for the patch to be able to land into Orion. Thank you!
Hi Mihai, Custom events vs DOM events Up to this point we never needed to expose DOM events in the editor (we use Custom Events). That said, I believe that at some point we will have to offer key events and mouse events. And when that day comes using custom events will not be fun (we don't want to go in businnes of normalizing DOM events etc). So, for some events, sending DOM events might be okay. Ruler In ruler we are passing click and dblclick events to the application (see _handleRulerEvent. Yes, I lied a bit, we are exposing DOM events a bit already ;-) If you need contextMenu, that is the place to add. Note: We never really made up our minds how to grow dom events to the editor and ruler, maybe exposing the dom events and having the same event name is the correct design. What do you think ? Last question, why didn't you use _handleContextMenu that is there already to implement the new event ?
Hello Felipe! Thanks for your really quick reply! (In reply to comment #3) > Hi Mihai, > > Custom events vs DOM events > Up to this point we never needed to expose DOM events in the editor (we use > Custom Events). > That said, I believe that at some point we will have to offer key events and > mouse events. And when that day comes using custom events will not be fun (we > don't want to go in businnes of normalizing DOM events etc). > So, for some events, sending DOM events might be okay. That sounds fine. > Ruler > In ruler we are passing click and dblclick events to the application (see > _handleRulerEvent. Yes, I lied a bit, we are exposing DOM events a bit already > ;-) > If you need contextMenu, that is the place to add. Agreed, I saw that. We don't need custom context menus for the rulers, yet. Since the contextmenu event is not prevented for the rulers, I think we can add any custom context menu we want, without further changes. (at the moment this is an untested claim, hehe :) ). > Note: We never really made up our minds how to grow dom events to the editor > and ruler, maybe exposing the dom events and having the same event name is the > correct design. > > What do you think ? For custom events it makes sense to use different event names, different from the actual original event names. If you are exposing the real DOM events, using the original event name makes sense, but it begs the question: when you do editor.addEventListener("ContextMenu", context, handler) ... you get the contextmenu event for which element? Which part of the editor? The editor is not a single element, and so on. The layerX/layerY properties are almost "useless" - are they relative to what? And so on. I did stay away from that because we only need screenX/screenY, but if we go the custom event object route, we'll have to answer the question of coordinates relative to which element. > Last question, why didn't you use _handleContextMenu that is there already to > implement the new event ? I thought of doing that, but you have it for overlayDiv and/or clientDiv, and I need onContextMenu to fire only for one of the two elements - the topNode. So I did leave that as is, and added the new handler. Also, naming consistency - it looks like you have onEventName() methods that sendEvent(). Shall I make changes? Do I need to ask for feedback/review or ... something else to get the patch accepted and pulled into orion.client?
Yes, I remember now. We did not expose dom events because we didn't want to expose our internal elements (_clientDiv, etc). On the other hand, adding a event with the same name of a dom event (like contextmenu) and passing a custom event object to it that does not have all the same properties as the real dom event will be confusing to the user (plus events are quite different from browser to browser, specially key down and key press). I think that is where we got stuck last time. At this point I think custom event is probably safer in terms of API (sorry for changing my mind). The x,y in this event should be relative to the document. See Editor#convert() A x,y location can be relative to the page, document, or editor. Editor#convert() can be used to convert from one "coordinate system" to another. All our public API that is returning a location (getLocationAtOffset, etc) returns this location relative to "document". I expected that you will convert this x,y from the document to the page to find out where to place the custom menu, correct ? ------------------- I didn't understand why you couldn't use _handleContextMenu, are you getting it twice ? ------------------- Those onWhateverevent are there so that users can dojo.connect to listen to our events (bypassing our addEventListener framework). ------------------- Once you are happy with the code I will push the changes upstream. Unfortunately this area has some unanswer questions... sorry
(In reply to comment #5) > Yes, I remember now. > We did not expose dom events because we didn't want to expose our internal > elements (_clientDiv, etc). > On the other hand, adding a event with the same name of a dom event (like > contextmenu) and passing a custom event object to it that does not have all the > same properties as the real dom event will be confusing to the user (plus > events are quite different from browser to browser, specially key down and key > press). > > I think that is where we got stuck last time. > > At this point I think custom event is probably safer in terms of API (sorry for > changing my mind). The x,y in this event should be relative to the document. No problem. Sure, I can add x and y properties such that they are relative to the document. Which document? The document/page that holds the editor? Or the document in the editor iframe? (asking to make sure - the "document" word can be confusing, since the editor uses an iframe which also holds a document) > See Editor#convert() > A x,y location can be relative to the page, document, or editor. > Editor#convert() can be used to convert from one "coordinate system" to > another. > All our public API that is returning a location (getLocationAtOffset, etc) > returns this location relative to "document". > > I expected that you will convert this x,y from the document to the page to find > out where to place the custom menu, correct ? I saw the convert() method and I was about to use it, but determined that the screenX and screenY coordinates are better suited for our needs in the Mozilla codebase. We can open contextmenus relative to anything we want. So, I am going to update the patch to use a custom event. What name shall I give to the event? "ContextMenu" like now is fine? Or do you want a different name that doesn't collide with the original DOM event name? Please note that the custom event will have four properties: x, y, screenX and screenY. We need the latter two properties as well. Is this fine? > ------------------- > I didn't understand why you couldn't use _handleContextMenu, are you getting it > twice ? Yes, I feared I'll get it called twice. _handleContextMenu() is added to clientDiv and overlayDiv, so it really means the authors considered both needed, and that both might execute. > ------------------- > > Those onWhateverevent are there so that users can dojo.connect to listen to our > events (bypassing our addEventListener framework). Oh, thanks for the explanation. Do you want me to merge onContextMenu() into _handleContextMenu()? I am fine with this change, if you want it.
Okay, instead of guessing I tried the code, this works for me: Added to editor.js onContextMenu: function(contextMenuEvent) { this._eventTable.sendEvent("ContextMenu", contextMenuEvent); }, _handleContextMenu: function (e) { if (!e) { e = window.event; } var scroll = this._getScroll(); var editorRect = this._editorDiv.getBoundingClientRect(); var editorPad = this._getEditorPadding(); var x = e.clientX + scroll.x - editorRect.left - editorPad.left; var y = e.clientY + scroll.y - editorRect.top - editorPad.top; this.onContextMenu ({x: x, y:y}); if (e.preventDefault) { e.preventDefault(); } return false; }, added to demo.html editor.addEventListener("ContextMenu", window, function(evt) { log ("here", evt.x, evt.y); editor.convert(evt, "document", "page"); log ("here2", evt.x, evt.y); var m = document.getElementById("menu"); m.style.left = evt.x + "px"; m.style.top = evt.y + "px"; }, null); where menu is: <div id="menu" style="position: fixed; background: red; width:100px; height:100px; left=0px; top=0px;"></div> I will answer your question in the next comment.
"document" for us is relative to document model (text model). This means that the x,y won't change when the editor scrolls. We can talk about changing that. >So, I am going to update the patch to use a custom event. What name shall I >give to the event? "ContextMenu" like now is fine? "ContextMenu" is fine >Please note that the custom event will have four properties: x, y, screenX and >screenY. We need the latter two properties as well. Is this fine? Yes, that is fine. >Yes, I feared I'll get it called twice. _handleContextMenu() is added to >clientDiv and overlayDiv, so it really means the authors considered both >needed, and that both might execute. I tested, the code is correct. We need "contextmenu" on both clientDiv and overlayDiv. During click "contextmenu" goes to the overlayDiv, during keyboard (menu key) it goes to the clientDiv. The correct code really is putting the code in the _handleContextMenu. Note, the event does not happen twice as we stop it. >Do you want me to merge onContextMenu() into _handleContextMenu()? I am fine >with this change, if you want it. Yes please, like I did on my hack. Please provide the jsdoc, I'm going home now but I hope we can finish this by tomorrow. Thanks.
(In reply to comment #7) > Okay, instead of guessing I tried the code, this works for me: > Added to editor.js > > onContextMenu: function(contextMenuEvent) { > this._eventTable.sendEvent("ContextMenu", contextMenuEvent); > }, > _handleContextMenu: function (e) { > if (!e) { e = window.event; } > var scroll = this._getScroll(); > var editorRect = this._editorDiv.getBoundingClientRect(); > var editorPad = this._getEditorPadding(); > var x = e.clientX + scroll.x - editorRect.left - editorPad.left; > var y = e.clientY + scroll.y - editorRect.top - editorPad.top; Thanks for these bits of code! I would have certainly had to do some more code investigation before figuring out this. > added to demo.html > editor.addEventListener("ContextMenu", window, function(evt) { > log ("here", evt.x, evt.y); > editor.convert(evt, "document", "page"); > log ("here2", evt.x, evt.y); > var m = document.getElementById("menu"); > m.style.left = evt.x + "px"; > m.style.top = evt.y + "px"; > }, null); > > where menu is: > <div id="menu" style="position: fixed; background: red; width:100px; > height:100px; left=0px; top=0px;"></div> I have avoided adding a fake menu to the demo, and I am still hesitant of doing this, because the #menu element needs more work to behave like a context menu. (the user needs a way to dismiss the element and so on) I believe just logging the coordinates is sufficient for demo purposes. (In reply to comment #8) > "document" for us is relative to document model (text model). This means that > the x,y won't change when the editor scrolls. We can talk about changing that. That's fine, it makes sense. > >Yes, I feared I'll get it called twice. _handleContextMenu() is added to > >clientDiv and overlayDiv, so it really means the authors considered both > >needed, and that both might execute. > > I tested, the code is correct. We need "contextmenu" on both clientDiv and > overlayDiv. During click "contextmenu" goes to the overlayDiv, during keyboard > (menu key) it goes to the clientDiv. The correct code really is putting the > code in the _handleContextMenu. > Note, the event does not happen twice as we stop it. That makes sense. I didn't know why you had the _handleContextMenu for both elements. Thanks for pointing out the reason. > Please provide the jsdoc, I'm going home now but I hope we can finish this by > tomorrow. Thanks. Sure! I have updated the pull request with the changes you suggested. Please let me know if the code is fine for landing, or if I have to make further changes. Thank you!
>I have updated the pull request with the changes you suggested. Please let me >know if the code is fine for landing, or if I have to make further changes. Looks good, thank you. I'm in the processed to moving files around (see Bug 347826), once I done with that I'll release the patch and close this problem report.
Felipe, since this is the first time for us dealing with a pull request, here is the document describing how we should go about accepting contributions through Git: http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions
Mihai, per http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions could you please confirm that "I wrote all this code and have the rights to contribute it to Eclipse under the eclipse.org web site terms of use." ? Thanks!
Sure, i confirm that I wrote all this code and have the rights to contribute it to Eclipse under the eclipse.org web site terms of use.
http://git.eclipse.org/c/e4/org.eclipse.orion.client.git/commit/?id=b3ccb0ce770f03a5dbca3da7116c217003ff8ce1 Fixed in HEAD thank you Mihai
