366992 - globalOverlay.js uses bogus controller algorithm

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[[e-mail]]

The internal controller algorithm used by Gecko searches from the focused element through its window ancestry to find a controller that supports the specified command. At that point the search should stop, and the enabled state should be queried solely from that controller. However bug 317633 introduced a wrapper function that manually searches additional controllers looking for an enabled command, which defeats the purpose of the supportsCommand method.

For an example, SuiteRunner wants to use a command to enable the Copy menuitem depending on whether the evaluator box has focus or not. The Ctrl+C keystroke already works correctly because that it processed internally, but the menuitem will copy the error message if the evaluator box has focus but no selection.

I don't know if any browser code actually depends on this bogus behaviour.

In addition to removing the wrapper, the doCommand method needs to account for the fact that top.document.commandDispatcher.getControllerForCommand may not return an enabled controller.

Comment 1

[[e-mail]]

Whoops, I accidentally clicked on that privacy checkbox :-(

Comment 2

[Asaf Romano (gone)]

I'm OK with taking this change back now that bug 359462 is fixed.

If doing this does break the places controllers somehow, we can change the places-views to use insertControllerAt rather than appendController.

Comment 3

[Asaf Romano (gone)]

Also, as fancy as it was I think we should remove the CommandUpdater object and restore the old functions.

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

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

Comment on attachment 251476 [details] [diff] [review]
patch

Note: I didn't look for the possible impacts this could have on the places controllers.

Comment 6

[Asaf Romano (gone)]

Attachment: unit test

Comment 7

[Asaf Romano (gone)]

mozilla/toolkit/content/globalOverlay.js 1.29

Comment 8

[Asaf Romano (gone)]

Attachment: unit test for checkin

Comment 9

[Asaf Romano (gone)]

mozilla/toolkit/content/tests/chrome/Makefile.in 1.3
mozilla/toolkit/content/tests/chrome/bug366992_window.xul initial revision: 1.1
mozilla/toolkit/content/tests/chrome/test_bug366992.xul initial revision: 1.1

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 251476 [details] [diff] [review]
patch

+function goOnEvent(node, aEvent)
+{
+  var numControllers = aNode.controllers.getControllerCount();
+  var controller;
+
+  for (var controllerIndex = 0; controllerIndex < numControllers; controllerIndex++) {
+    controller = aNode.controllers.getControllerAt(controllerIndex);
+    if (controller)
+      controller.onEvent(aEvent);
+  }
+}

I'm getting JavaScript Error: "aNode is not defined" {file: "chrome://global/content/globalOverlay.js" line: 139} on recent suiterunner builds - shouldn't the parameter be aNode and not node?

Comment 11

[Mark Banner (:standard8)]

Attachment: Fixes regression (aNode not node)

Fixes regression that I mentioned in comment 10, which was causing JavaScript Error: "aNode is not defined" {file: "chrome://global/content/globalOverlay.js" line: 139}

Comment 12

[Asaf Romano (gone)]

Thanks Mark.

mozilla/toolkit/content/globalOverlay.js 1.30