Closed Bug 366992 Opened 13 years ago Closed 13 years ago

globalOverlay.js uses bogus controller algorithm

Categories

(Toolkit :: XUL Widgets, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha3

People

(Reporter: neil, Assigned: mano)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
Whoops, I accidentally clicked on that privacy checkbox :-(
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.
Also, as fancy as it was I think we should remove the CommandUpdater object and restore the old functions.
Attached patch patchSplinter Review
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #251476 - Flags: first-review?(gavin.sharp)
Priority: -- → P2
Whiteboard: [19a2]
Target Milestone: --- → mozilla1.9alpha1
Comment on attachment 251476 [details] [diff] [review]
patch

Note: I didn't look for the possible impacts this could have on the places controllers.
Attachment #251476 - Flags: first-review?(gavin.sharp) → first-review+
Whiteboard: [19a2]
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha3
Attached file unit test (obsolete) —
Attachment #255153 - Flags: first-review?(gavin.sharp)
mozilla/toolkit/content/globalOverlay.js 1.29
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Attachment #255153 - Flags: first-review?(gavin.sharp) → first-review+
Attachment #255983 - Attachment is patch: true
Attachment #255983 - Attachment mime type: application/unknown → text/plain
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
Flags: in-testsuite? → in-testsuite+
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?
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}
Attachment #257265 - Flags: first-review?(gavin.sharp)
Attachment #257265 - Flags: first-review?(gavin.sharp) → first-review+
Thanks Mark.

mozilla/toolkit/content/globalOverlay.js 1.30
You need to log in before you can comment on or make changes to this bug.