Rob--W / browser-action-jplib

Jetpack module to add a Browser action badge to the toolbar, using the chrome.browserAction syntax from Chromium
21 stars 9 forks source link

popup doesn't show in Firefox 36.0b5 #18

Closed chatziko closed 9 years ago

chatziko commented 9 years ago

Testing with Firefox 36.0b5, the button shows but the popup does not display on click. There's no error message so I'm not sure what's going on. I'll test more when I find the time.

This might be useful: Add-on Compatibility for Firefox 36

Rob--W commented 9 years ago

This bug is most likely caused by https://github.com/mozilla/addon-sdk/commit/ea2f56bdd45b54e4ec59bfc97275b9d4added77f in sdk/panel.js

-on(shows, "data", ({target}) => emit(panelFor(target), "show"));
+on(shows, "data", ({target}) => {
+  let panel = panelFor(target);
+  if (modelFor(panel).ready)
+    emit(panel, "show");
+});

That modelFor(panel).ready appears to be false when the code path is triggered...

In its turn, it is caused by https://github.com/mozilla/addon-sdk/blob/0f2ba1873ba007f943c3ec5cb0f1cbec7eaf8bf5/lib/sdk/panel.js#L291-L296:

let ready = filter(panelEvents, ({type, target}) =>
  getAttachEventType(modelFor(panelFor(target))) === type);

This method is called first with type = "DOMContentLoaded" and then "load". They are equivalent to "ready" and "end", respectively (https://github.com/mozilla/addon-sdk/blob/0f2ba1873ba007f943c3ec5cb0f1cbec7eaf8bf5/lib/sdk/content/utils.js#L34-L42):

function getAttachEventType(model) {
  if (!model) return null;
  let when = model.contentScriptWhen;
  return requiresAddonGlobal(model) ? 'document-element-inserted' :
         when === 'start' ? 'document-element-inserted' :
         when === 'ready' ? 'DOMContentLoaded' :
         when === 'end' ? 'load' :
         null;
}

Since my library declares contentScriptWhen: 'start', the object in let ready will only forward events of type document-element-inserted. This event is never triggered though, so viewFor(panel).state is never set to true, and therefore the show/hide events are never triggered in panel.

To work around the issue, you could edit the library and set contentScriptWhen: 'start' with contentScriptWhen: 'ready: https://github.com/Rob--W/browser-action-jplib/blob/5d1e6df04c4fcb1c0ba1c68b92aaf3928cda09b7/lib/browserAction.js#L95. Doing that is not okay, because then the browserAction APIs within the popup (messaging and dimension calculation) is initiated too late. The best way of fixing this bug is to file a bug and/or submit a patch to the addon-sdk. I will do this within a few days.

For the record, I got the previous results as follows:

  1. Start Firefox with an addon that used browser-action-jplib.
  2. Enable addon debugging as described at https://developer.mozilla.org/en-US/Add-ons/Add-on_Debugger#Enabling_the_Add-on_Debugger.
  3. Open the debugger.
  4. Put a breakpoint at sdk/panel.js:298 (that is the line after let ready = ...)
  5. Disable the add-on.
  6. Enable the add-on.
  7. Now the breakpoint is triggerd, and I read the value of type, and step into the getAttachEventType method and conclude that start is never triggered,

ffdebug

Rob--W commented 9 years ago

Filed a bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1128478 and submitted a patch to https://github.com/mozilla/addon-sdk/pull/1854.

chatziko commented 9 years ago

Thanks for the quick response!

The patch was merged in addon-sdk, but I guess it also needs to be merged in firefox.

I just tried 36.0b8, the issue is still there.

Rob--W commented 9 years ago

I pinged the reviewer on the bugzilla ticket to ask how to proceed further. Follow that bug report if you want to stay updated.

Rob--W commented 9 years ago

Upstream bug has been fixed, this bug will not show up on Firefox stable.

chatziko commented 9 years ago

Tested it in Firefox 36 stable and it works fine. Thanks a lot for the help!