STR: 1. install a simple SDK add-on (1.16 beta) with a sidebar, example: const { Sidebar } = require('sdk/ui/sidebar'); const { data } = require('sdk/self'); const sidebar = Sidebar({ id : 'sidebartest', title : 'Sidebar Test', url : data.url('index.html') }); 2. open the Australis menu and use the sidebar widget to enable your sidebar. Expected result: The sidebar opens. Actual result: Nothing happens. View → Sidebar → <your add-on sidebar> works as expected. Tested with current Nightly and Aurora (new profile), Beta has no sidebar widget.

Matteo?

This is because the sidebar module uses a "command" event listener instead of an "oncommand" attribute, and because we can't copy event listeners in a sensible way, nothing is around to do anything with the clicks/commands in the sidebar widget.

(In reply to :Gijs Kruitbosch from comment #2) > This is because the sidebar module uses a "command" event listener instead > of an "oncommand" attribute, and because we can't copy event listeners in a > sensible way, nothing is around to do anything with the clicks/commands in > the sidebar widget. To be clear: we manually "copy" the elements in the view > Sidebars menu into the subview widget when it is shown. The same happens for the devtools and help subviews, and they have the same problem (see bug 941244).

AFAIK the only way to us to use `oncommand` attribute is using the global `toggleSidebar` and therefore creates a <xul:broadcaster> element for that. Erik, do you know if there is any reason because the current implementation of sidebar doesn't create the broadcaster, or it was just avoided because an overkill in our case? How complicate would be to implement it? Because that would solve the issue, and it seems complicated to fix on australis side, 'cause there is no way in JS to clone / copy a DOM node with the listeners too.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #4) > AFAIK the only way to us to use `oncommand` attribute is using the global > `toggleSidebar` and therefore creates a <xul:broadcaster> element for that. > > Erik, do you know if there is any reason because the current implementation > of sidebar doesn't create the broadcaster, or it was just avoided because an > overkill in our case? How complicate would be to implement it? Because that > would solve the issue, and it seems complicated to fix on australis side, > 'cause there is no way in JS to clone / copy a DOM node with the listeners > too. I doubt it would be complicated to use a <xul:broadcaster> here if that is the question. I've mentioned the advantages to this approach before: https://groups.google.com/forum/#!topic/mozilla-labs-jetpack/5U656BpfaJw

Sounds like this needs a fix on the SDK side? May I be so bold as to assign it, then? (Feel free to reassign to a more appropriate owner.) Looks like bug 960198 isn't going to be uplifted to 29, so untracking from the first Australis release. (Otherwise I'd had to decide if it's a P2 or P3).

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5) > > Erik, do you know if there is any reason because the current implementation > > of sidebar doesn't create the broadcaster, or it was just avoided because an > > overkill in our case? How complicate would be to implement it? Because that > > would solve the issue, and it seems complicated to fix on australis side, > > 'cause there is no way in JS to clone / copy a DOM node with the listeners > > too. > > I doubt it would be complicated to use a <xul:broadcaster> here if that is > the question. > > I've mentioned the advantages to this approach before: > https://groups.google.com/forum/#!topic/mozilla-labs-jetpack/5U656BpfaJw I remember the conversation, but it was a more generic approach: here I'm talking specifically only about sidebar, to implement the sidebar as any other in Firefox (so create a broadcaster and a menuitem element for each sidebar). Erik, do you mind to take this bug? You're more familiar with the whole sidebar code than me, it would probably take less to fix it.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7) > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5) > > > > Erik, do you know if there is any reason because the current implementation > > > of sidebar doesn't create the broadcaster, or it was just avoided because an > > > overkill in our case? How complicate would be to implement it? Because that > > > would solve the issue, and it seems complicated to fix on australis side, > > > 'cause there is no way in JS to clone / copy a DOM node with the listeners > > > too. > > > > I doubt it would be complicated to use a <xul:broadcaster> here if that is > > the question. > > > > I've mentioned the advantages to this approach before: > > https://groups.google.com/forum/#!topic/mozilla-labs-jetpack/5U656BpfaJw > > I remember the conversation, but it was a more generic approach: here I'm > talking specifically only about sidebar, to implement the sidebar as any > other in Firefox (so create a broadcaster and a menuitem element for each > sidebar). Erik, do you mind to take this bug? You're more familiar with the > whole sidebar code than me, it would probably take less to fix it. I think I have too much on my plate right now.

I don't think we can ship Firefox 30 with this issue.

I would tend to agree, but it would need to be prioritized by the SDK folks, as that's where any code fixes would need to go. It's already a P1. Matteo, are you making progress on this issue, do you need help, ...?

(In reply to :Gijs Kruitbosch from comment #11) > I would tend to agree, but it would need to be prioritized by the SDK folks, > as that's where any code fixes would need to go. It's already a P1. Matteo, > are you making progress on this issue, do you need help, ...? one temporary workaround would be to trigger the command event on the menu item. I think it's possible to trigger events, but I don't think it's best to do so.

(In reply to :Gijs Kruitbosch from comment #11) > I would tend to agree, but it would need to be prioritized by the SDK folks, > as that's where any code fixes would need to go. It's already a P1. Matteo, > are you making progress on this issue, do you need help, ...? No, if fix that on SDK side, we need to rewrite part of the sidebar code and because I didn't write that it takes a bit more time than expected. I'm also trying to check if it would be possible having a not-so-bad workaround in order to ship with Fx30 and then a proper fix with the code of sidebar rewrote.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #13) > > I would tend to agree, but it would need to be prioritized by the SDK folks, > > as that's where any code fixes would need to go. It's already a P1. Matteo, > > are you making progress on this issue, do you need help, ...? > > No, if fix that on SDK side, The "No" was about the help part. :) If we're going to fix that on SDK side I don't think there is anything to do on your side.

This is the last week in FF30 Beta where we can take more speculative fixes. If you have a safe workaround patch that we can try on FF30 to deal with this until a more proper fix in later versions, that sounds like a lower risk option at this time.

getting @canuckistani to comment on whether a fix is necessary for 30

(In reply to Lukas Blakk [:lsblakk] from comment #15) > This is the last week in FF30 Beta where we can take more speculative fixes. > If you have a safe workaround patch that we can try on FF30 to deal with > this until a more proper fix in later versions, that sounds like a lower > risk option at this time. Sidebar is a relatively new api, and this interaction between it and the sidebar button is a really new thing. From my perspective uplift to Beta is a nice-to-have, but happy to defer to others if you feel it is higher priority.

(In reply to Jeff Griffiths (:canuckistani) from comment #17) > (In reply to Lukas Blakk [:lsblakk] from comment #15) > > This is the last week in FF30 Beta where we can take more speculative fixes. > > If you have a safe workaround patch that we can try on FF30 to deal with > > this until a more proper fix in later versions, that sounds like a lower > > risk option at this time. > > Sidebar is a relatively new api, and this interaction between it and the > sidebar button is a really new thing. From my perspective uplift to Beta is > a nice-to-have, but happy to defer to others if you feel it is higher > priority. FWIW, a lot of the stuff in the add-ons contest that we organized for 29/30 created sidebars, presumably because it was easier, more familiar and simpler UI to deal with than panels. Having the primary UI for toggling those sidebars being broken when we've just told everyone to go use this is... not cool, IMO. I would argue that we should try to have a fix for beta. It really shouldn't be terribly hard (famous last words, of course...).

I took a stab at fixing this yesterday but couldn't see any easy path to fixing this, in large part due to a lack of understanding the addon sdk code. I kind of see how it could be fixed, but would need help from someone who understands addon sdk internals.

(In reply to Shane Caraveo (:mixedpuppy) from comment #19) > I took a stab at fixing this yesterday but couldn't see any easy path to > fixing this, in large part due to a lack of understanding the addon sdk > code. I kind of see how it could be fixed, but would need help from someone > who understands addon sdk internals. Same here. In particular, it seems that for whatever reason, the sidebar code has a weakmap from models to views, and vice versa, but no way to look up either based on an identifier like an ID, which is what is needed here. (and because they're weakmaps, you can't trivially iterate them) Once you have that, all you really need is to: 1) check for and, if it doesn't exist, assign a function to the window's global scope in the windowtracker that can handle clicks on the SDK's menu elements 2) setAttribute("oncommand", "window._sdkfunctiontohandleclicksonmenuelements(event)") instead of the current addEventListener call (which is never removed? That might well leak?) in sidebar.js 3) make the aforementioned function determine the sidebar ID (inverse of makeId in view.js) and look up the model/view 4) do the same thing as the event listener did based on that model/view

Just to re-iterate I *DO* think this should be fixed. What I don't see is a pressing need to do this for Beta 30 given that the sidebar widget is not shown by default.

Indeed, no need to get this to a specific version, we can take an uplift depending on where we are in the cycle once one is ready.

Gijs, is there any reason why the Tim's proposal in comment 12 is not applicable? I digged a bit in the sidebar code, and the couple of approaches I tried seems too messy compare to that solution. Also, it would avoid to inject functions in the `window` object, and it will works no matter if the command listener is set by attribute or by JS.

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #23) > Gijs, is there any reason why the Tim's proposal in comment 12 is not > applicable? > > I digged a bit in the sidebar code, and the couple of approaches I tried > seems too messy compare to that solution. Also, it would avoid to inject > functions in the `window` object, and it will works no matter if the command > listener is set by attribute or by JS. I'm pretty hesitant to start re-firing events, yes. Getting them to be "right" as compared with the 'real' ones (ie also firing the right click and/or keypress events that caused the command events) seems much harder than just copying all the attributes. I would also imagine that retriggering events would cause focus issues, might not work the same way on OS X (because the real menubar is natively implemented), might not work because the menu is closed (and if you're saying "messy", wait until we have to manually force the menu to be open...) and might also be async (honestly not sure if the created event would be queued or fired sync) which would introduce lag in the reaction of clicking and having something happening. I'm not sure why the solution I outlined would be "messy", or what's wrong with having a global function - Firefox's core has tons of them, one more or less isn't going to hurt. If you desperately want to avoid a global function, you could create custom events that fire based on the oncommand attribute. In any case, I would be very surprised if fixing this in the SDK was messier than re-triggering events across the browser.

(In reply to :Gijs Kruitbosch from comment #24) > (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #23) > > Gijs, is there any reason why the Tim's proposal in comment 12 is not > > applicable? > > > > I digged a bit in the sidebar code, and the couple of approaches I tried > > seems too messy compare to that solution. Also, it would avoid to inject > > functions in the `window` object, and it will works no matter if the command > > listener is set by attribute or by JS. > > I'm pretty hesitant to start re-firing events, yes. Getting them to be > "right" as compared with the 'real' ones (ie also firing the right click > and/or keypress events that caused the command events) seems much harder > than just copying all the attributes. I would also imagine that retriggering > events would cause focus issues, might not work the same way on OS X > (because the real menubar is natively implemented), might not work because > the menu is closed (and if you're saying "messy", wait until we have to > manually force the menu to be open...) and might also be async (honestly not > sure if the created event would be queued or fired sync) which would > introduce lag in the reaction of clicking and having something happening. Events are fired synchronously. You don't need to force the menu open, you're just firing an event to DOM nodes here. Firing an event shouldn't affect focus. We don't need to fire anything except the command event for now (if we do then the new widget is already broken for those cases). This would give compatibility for any add-on that decides to attach a JS event rather than using a XUL attribute, I'd be surprised if that were just SDK add-ons. Conversely I'd expect copying the oncommand attribute to be breaking some cases too unless you're altering the event's target parameter and the |this| variable when calling it somehow.

Do we have any tests for the oncommand/onclick copying? I made a quick patch that switches to dispatching events to the original elements and it seems to pass all browser chrome tests and my manual tests on OSX. This works for the default sidebars, SDK sidebars and the web developer menu. Try is running for other platforms at https://tbpl.mozilla.org/?tree=Try&rev=23da69f254c0. I'm probably not going to be around to work anymore on this after tomorrow but this seems like a straightforward patch that gives us more compatibility than we had previously.

(In reply to Dave Townsend (Gone till June 16th) [:mossop] from comment #26) > Created attachment 8425852 [details] [diff] [review] > patch > > Do we have any tests for the oncommand/onclick copying? I made a quick patch > that switches to dispatching events to the original elements and it seems to > pass all browser chrome tests and my manual tests on OSX. This works for the > default sidebars, SDK sidebars and the web developer menu. Try is running > for other platforms at https://tbpl.mozilla.org/?tree=Try&rev=23da69f254c0. > > I'm probably not going to be around to work anymore on this after tomorrow > but this seems like a straightforward patch that gives us more compatibility > than we had previously. You should probably remove the event listeners when the panel/subview closes.

I don't think we want accumulated event listeners, unless that won't happen ?

(In reply to Tim Nguyen [:ntim] from comment #28) > I don't think we want accumulated event listeners, unless that won't happen ? Looks like when the subview closes for both the cases we call clearSubview which removes and discards all the elements. That will destroy the event listeners too.

Comment on attachment 8425852 [details] [diff] [review] patch Review of attachment 8425852 [details] [diff] [review]: ----------------------------------------------------------------- This won't work. You're no longer copying the command attribute, which is still necessary because they cause command events to fire on the command element instead of on the menuitem (ie your listener never gets invoked). The reason this didn't break all of devtools is that you're still copying the observes attribute, and the devtools items observe their own broadcaster, which have command/oncommand attributes, which means the copies will get that attribute despite your code no longer copying it. However, this breaks third-party things that go into these menus (e.g. DOM Inspector) that don't use an observer. Hence r-. Interestingly, I thought that this would mean that for items that observe a broadcaster with an "oncommand" attribute, the event is actually observed twice, that is, both the original and the copy's oncommand listeners fire. However, in my debugging (breakpoint in ToggleToolbox for devtools, clicking "Toggle Tools" in the developer tools subview), I'm only seeing the code being called from the copy's oncommand listener. I checked and your event listener is firing and creating and sending a copy. So it seems that the event you're manually sending to the original menuitem fails to trigger the corresponding command event listener code (possibly because it isn't visible onscreen), which is what I was afraid of in my previous comment here. Tested on Win8.1. I'm also worried that the click event coordinates you're passing are wrong, or, conversely (perhaps only if you correct them), that the click event might trigger a command event (leading to still more duplication). This was why I was concerned about the open state of the menus and so on in my previous comments - generally, IME the handling of command events and whether or not clicks/keypresses trigger command events is highly magical (cf. bug 977217). We sadly don't have tests about the items in these menus working correctly, or the various builtin/add-on cases for how menuitems can receive/forward/deal with events. I think an easier, more upliftable solution here is to only forward the events if there is no onclick, oncommand or command attribute to copy. ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +100,5 @@ > subviewItem = doc.createElementNS(kNSXUL, "toolbarbutton"); > CustomizableUI.addShortcut(menuChild, subviewItem); > + > + subviewItem.addEventListener("click", event => { > + let newEvent = doc.createEvent("MouseEvent"); Nit: you should be using the new event constructor syntax instead. Ditto below.

(In reply to :Gijs Kruitbosch from comment #30) > Interestingly, I thought that this would mean that for items that observe a > broadcaster with an "oncommand" attribute, the event is actually observed > twice, that is, both the original and the copy's oncommand listeners fire. > However, in my debugging (breakpoint in ToggleToolbox for devtools, clicking > "Toggle Tools" in the developer tools subview), I'm only seeing the code > being called from the copy's oncommand listener. I checked and your event > listener is firing and creating and sending a copy. So it seems that the > event you're manually sending to the original menuitem fails to trigger the > corresponding command event listener code (possibly because it isn't visible > onscreen), which is what I was afraid of in my previous comment here. Tested > on Win8.1. Actually, maybe it's being sent to the wrong menu element. I'm seeing clicking on "Toggle Tools" with this patch also toggling the "work offline" state...

(In reply to :Gijs Kruitbosch from comment #30) > Comment on attachment 8425852 [details] [diff] [review] > patch > > Review of attachment 8425852 [details] [diff] [review]: > ----------------------------------------------------------------- > > This won't work. You're no longer copying the command attribute, which is > still necessary because they cause command events to fire on the command > element instead of on the menuitem (ie your listener never gets invoked). > The reason this didn't break all of devtools is that you're still copying > the observes attribute, and the devtools items observe their own > broadcaster, which have command/oncommand attributes, which means the copies > will get that attribute despite your code no longer copying it. Actually as long as you're dispatching real XUL command elements to a XUL element the command attribute will cause it to get forwarded to the command element. We can use this fact to our advantage though. If we copy the command attribute then listen for command events we can dispatch a customevent called "command" to the original element and trigger any command listeners there without double-triggering listeners on the command element. > However, this breaks third-party things that go into these menus (e.g. DOM > Inspector) that don't use an observer. Hence r-. > > Interestingly, I thought that this would mean that for items that observe a > broadcaster with an "oncommand" attribute, the event is actually observed > twice, that is, both the original and the copy's oncommand listeners fire. > However, in my debugging (breakpoint in ToggleToolbox for devtools, clicking > "Toggle Tools" in the developer tools subview), I'm only seeing the code > being called from the copy's oncommand listener. I checked and your event > listener is firing and creating and sending a copy. So it seems that the > event you're manually sending to the original menuitem fails to trigger the > corresponding command event listener code (possibly because it isn't visible > onscreen), which is what I was afraid of in my previous comment here. Tested > on Win8.1. > > I'm also worried that the click event coordinates you're passing are wrong, > or, conversely (perhaps only if you correct them), that the click event > might trigger a command event (leading to still more duplication). This was > why I was concerned about the open state of the menus and so on in my > previous comments - generally, IME the handling of command events and > whether or not clicks/keypresses trigger command events is highly magical > (cf. bug 977217). I can't see a case where a click event will trigger a command event but yes the coordinates are off. We're probably getting into the real tail end of compatibility problems if we have add-ons checking the click coordinates on a menu item but since we're recreating the event it might not be too difficult to fix that if we find we need to later. > We sadly don't have tests about the items in these menus working correctly, > or the various builtin/add-on cases for how menuitems can > receive/forward/deal with events. > > > I think an easier, more upliftable solution here is to only forward the > events if there is no onclick, oncommand or command attribute to copy. I've adjusted this a little. The presence of a command attribute doesn't affect things as mentioned above. Instead I forward a click event if there is no onclick attribute and a command event if there is no oncommand attribute. I do think it would be safe to just not copy onclick/oncommand and always forward those but the only case this breaks is if the menuitem has both an onclick attribute and a click listener (or ditto for command) which seems like to be rare. There is some sneaky stuff around broadcasters which means that we can get onclick without explicit copying too so there would have to be some extra work to handle that case. Probably only worth it if we see issues. I have this all working with a test with some 21 testcases passing, only 4 of those have sadfaces because they are missing events because of the above. Only problem is that there is some focus issue that so far I've worked around with a timeout but that isn't stable. I'm going to try to figure that out in the next few hours.

Ok here is the updated patch with tests. I've only tested on OSX but it's spinning through try now at https://tbpl.mozilla.org/?tree=Try&rev=56f3163a9ee0. I think this is good to go, adds more test coverage than we had before and safe enough for uplift. In the cases that worked before nothing has changed really so this is just adding compatibility for other cases. I'm PTO from tomorrow onwards though so I won't have time to either land or address review comments here so someone else will need to step up if this needs more work.

Comment on attachment 8426552 [details] [diff] [review] patch Review of attachment 8426552 [details] [diff] [review]: ----------------------------------------------------------------- Generally, looks good to me, try run was nice and green too. Thanks for jumping on this. I'll adjust my own comments and push this later today, I hope. ::: browser/components/customizableui/src/CustomizableWidgets.jsm @@ +105,5 @@ > + if (!item.hasAttribute("onclick")) { > + subviewItem.addEventListener("click", event => { > + let newEvent = new doc.defaultView.MouseEvent(event.type, event); > + item.dispatchEvent(newEvent); > + }, false); Nit: no need to pass false. Same in the test. @@ +110,5 @@ > + } > + > + if (!item.hasAttribute("oncommand")) { > + subviewItem.addEventListener("command", event => { > + let newEvent = new doc.defaultView.CustomEvent(event.type, event); Now, I realize I sent you down this path so this is totally my fault, but unfortunately command events have a source event that indicates whether the command was caused by a click or a keypress or something else (and any modifier keys that were pressed and so on and so forth). Custom events don't, so I suspect this constructor won't work well in terms of passing that information, and the initEvent syntax is a better bet (AFAICT there's no webidl-defined constructor for XULCommandEvent - I'm not sure why, so I'm going to ask Olli about that). Which is sucky, but there we are. :-( ::: browser/components/customizableui/test/browser_988072_sidebar_events.js @@ +36,5 @@ > + return sidebars; > +} > + > +function compareElements(original, displayed) { > + // TODO check the class is right There's a separate test for this, fortunately. :-) @@ +108,5 @@ > + "Check that a sidebar that uses a command event listener works", > +function*() { > + let newSidebar = document.createElement("menuitem"); > + newSidebar.setAttribute("id", "testsidebar"); > + newSidebar.setAttribute("label", "Test Sidebar"); Nit: we're only adding 1 item at a time, and it's always the same, so might as well assign this to gTestSidebar or something and make it part of setup() (and removing it part of teardown() ) @@ +198,5 @@ > + document.getElementById("testsidebar").remove(); > +}); > + > +add_sidebar_task( > + "Check that a sidebar that uses an command attribute and a click listener works", nit: an oncommand attribute

(In reply to :Gijs Kruitbosch from comment #34) > @@ +110,5 @@ > > + } > > + > > + if (!item.hasAttribute("oncommand")) { > > + subviewItem.addEventListener("command", event => { > > + let newEvent = new doc.defaultView.CustomEvent(event.type, event); > > Now, I realize I sent you down this path so this is totally my fault, but > unfortunately command events have a source event that indicates whether the > command was caused by a click or a keypress or something else (and any > modifier keys that were pressed and so on and so forth). Custom events > don't, so I suspect this constructor won't work well in terms of passing > that information, and the initEvent syntax is a better bet (AFAICT there's > no webidl-defined constructor for XULCommandEvent - I'm not sure why, so I'm > going to ask Olli about that). Which is sucky, but there we are. :-( Olli, am I correct thinking it's impossible to create an event like this using new XULCommandEvent from JS ? :-(

You need to use somedoc.createEvent("XULCommandEvent"); for it - for now at least. http://mxr.mozilla.org/mozilla-central/source/dom/webidl/XULCommandEvent.webidl doesn't have constructor.

Of course, when I'm ready to land, there's infra issues and the tree is closed...

Try run in comment #33.

Tomcat kindly landed this earlier today: https://hg.mozilla.org/integration/fx-team/rev/98cb21c770b9

Comment on attachment 8427334 [details] [diff] [review] Patch for checkin I'd like to uplift this to 31 but /without/ the test, as it's oranging rather a bit and 31 is an ESR branch, and the pain for the sheriffs outweighs the benefits of having the test there, IMO. I expect that's an issue with the test, but I don't know that I'll have time to look at this in a hurry - but we should fix this on 31 because it's an ESR branch. I think it's too late to take this on beta. [Approval Request Comment] Bug caused by (feature/regressing bug #): sidebar button / australis / add-on sdk sidebar User impact if declined: non-functional SDK-and-possibly-other sidebar items in the "Sidebar" subview of the sidebar button in the customizable menu. Testing completed (on m-c, etc.): on m-c for a bit, has automated test Risk to taking this patch (and alternatives if risky): low, although the random orange gives me some pause, I expect it just has to do with timing issues. I'll be taking a look at it shortly. String or IDL/UUID changes made by this patch: none

Comment on attachment 8427334 [details] [diff] [review] Patch for checkin Clearly too late for beta but we have time for aurora.