1121035 - XUL event handlers missing from Firefox 36

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Dietrich Ayala (:dietrich)]

I have an add-on that overrides some commands via event handlers in a XUL overlay.

The XUL is here:

https://github.com/autonome/vimkeybindings/blob/master/content/vimkeybindings.xml

It's been working since about 2007, but just stopped working in 36.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is caused by http://hg.mozilla.org/mozilla-central/rev/8c589a6d637e / bug 1075658. Verified by testing with the Nightly builds:

Last good: 2014-10-30
First bad: 2014-10-31

Comment 2

[:Gijs (he/him)]

Dave, any idea how this change broke the keyboard shortcut functionality? :s

(I don't see anything relevant, unless the remote bindings are radically different as far as keybindings are concerned?)

Comment 3

[Dave Townsend [:mossop]]

That patch should have no effect on keyboard handling in general

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=80e18ff7c7b2&tochange=6bd2071b373f

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dave Townsend [:mossop] from comment #3)
> That patch should have no effect on keyboard handling in general

From IRC, Archaeopteryx suggests this change may be relevant:

https://hg.mozilla.org/mozilla-central/rev/8c589a6d637e#l3.1

Comment 6

[Dave Townsend [:mossop]]

Ok, bug 1075658 changed the XBL in use for browser elements in the tabbrowser even for non-e10s, even without that this add-on would be broken by e10s. The problems are two-fold. Firstly the CSS rule it uses to apply its custom binding to browser elements isn't as specific as the one we use to set the new binding (.browserStack > browser) so the binding is never applied. Secondly the custom binding extends the default browser binding rather than the custom new one we are using so it may break certain features in non-e10s, would definitely break the browser entirely in e10s.

I don't think there is anything to be done in Firefox to solve this kind of add-on change, sorry Dietrich you're going to have to make a few changes to the add-on. Let me know if you need tips.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dave Townsend [:mossop] from comment #6)
> Secondly the custom binding extends the default browser binding
> rather than the custom new one we are using

Do we really need to use a custom new binding? I obviously don't know, but it seems possible that the add-on compatibility impact to that change may be larger than we expect, or are ultimately willing to accept.

Comment 8

[Dave Townsend [:mossop]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #7)
> (In reply to Dave Townsend [:mossop] from comment #6)
> > Secondly the custom binding extends the default browser binding
> > rather than the custom new one we are using
> 
> Do we really need to use a custom new binding? I obviously don't know, but
> it seems possible that the add-on compatibility impact to that change may be
> larger than we expect, or are ultimately willing to accept.

It would be possible to do bug 1075658 without a new binding by including some assumptions about the browser code in the toolkit binding which might break seamonkey if we weren't careful. But even without that e10s introduces the new remote-browser binding which breaks this add-on in the same way and avoiding that would be much harder and make the code needlessly complex.

I don't recommend replacing the browser's XBL binding. If you installed two add-ons that did it they would break each other's functionality so I would expect that few add-ons do it (I can see exactly 5 hosted on AMO including dietrich's).

Comment 9

[Dietrich Ayala (:dietrich)]

For the historical record:

* Example code from Mossop for accomplishing similar thing: http://pastebin.mozilla.org/8202725

* Current approach for the add-on: http://pastebin.mozilla.org/8204188

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Unfortunately pastebin isn't a great way to note things for historical record (those pastebins have already been cleared) :(

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #10)
> Unfortunately pastebin isn't a great way to note things for historical
> record (those pastebins have already been cleared) :(

This is still working:
(In reply to Dietrich Ayala (:dietrich) from comment #9)
> * Current approach for the add-on: http://pastebin.mozilla.org/8204188

Content:
var utils = require('sdk/window/utils');

var actions = [
  {key: 'h', command: 'cmd_scrollLeft'},
  {key: 'j', command: 'cmd_scrollLineDown'},
  {key: 'k', command: 'cmd_scrollLineUp'},
  {key: 'l', command: 'cmd_scrollRight'},
  {key: 'g', command: 'cmd_scrollTop'},
  {key: 'G', command: 'cmd_scrollBottom'}
];

function currentAndFutureWindows(func) {
  var windows = require('sdk/windows').browserWindows,
      { viewFor } = require("sdk/view/core");
  for (let window of windows) {
    func(viewFor(window));
  }
  windows.on('open', function(window) {
    func(viewFor(window));
  });
}

function onWindow(window) {
  window.gBrowser.addEventListener("keypress", function(event) {
    var action = actions.find(function(value) {
      return value.key == event.key;
    });

    if (action) {
      runCommand(action.command);
    }
  }, false);
}

function runCommand(cmd) {
  var window = utils.getMostRecentBrowserWindow();
  var controller = window.document.commandDispatcher.getControllerForCommand(cmd);
  controller.doCommand(cmd);
}

currentAndFutureWindows(onWindow);