Closed
Bug 1121035
Opened 10 years ago
Closed 10 years ago
XUL event handlers missing from Firefox 36
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: dietrich, Unassigned)
References
Details
(Keywords: regression)
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.
Updated•10 years ago
|
Comment 1•10 years ago
|
||
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
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Comment 2•10 years ago
|
||
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?)
Blocks: 1075658
Component: Untriaged → Tabbed Browser
Flags: needinfo?(dtownsend)
Product: Core → Firefox
Version: unspecified → 36 Branch
Comment 3•10 years ago
|
||
That patch should have no effect on keyboard handling in general
Flags: needinfo?(dtownsend)
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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).
Reporter | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
Unfortunately pastebin isn't a great way to note things for historical record (those pastebins have already been cleared) :(
Comment 11•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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);
You need to log in
before you can comment on or make changes to this bug.
Description
•