Either expose <xul:command> / [command] attribute broadcasting to chrome HTML documents, or stop using it in browser.xul

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: bgrins, Assigned: bdahl)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
+++ This bug was initially created as a clone of Bug #1479908 +++

As far as we can tell, there is a similar attribute broadcasting setup that we were addressing in Bug 1479908 <xul:command id="foo"> and the [command=foo] pattern we use.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1479908#c15 for more background.
(Reporter)

Comment 1

8 months ago
Here's the code that falls back to the command attribute if [observes] isn't present: https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/dom/xul/XULDocument.cpp#2434-2446. Note that it skips that for <key> and <menuitem>.
(Reporter)

Comment 2

8 months ago
There's some useful discussion about our options in Bug 1479908, mostly centered around either:

(a) Implement the same thing in JS via Custom Elements and Mutation Observers
(b) Stop relying on this behavior. The basic idea here would be to store state inside of a JS object instead of a DOM node, and have the observing nodes get mutated when the state object changes.

My summary of the main questions around (a):
* Possible async bugs with changing these to use Mutation Observers instead of the current impl in XULDocument
* Possible perf implications of creating a bunch of Mutation Observers. One for each <command> to detect arbitrary attribute changes, but also if we wanted to dynamically detect new nodes that code created with [command=foo] I think we'd need a global Mutation Observer listening to every new node. Unsure how frequently that happens - maybe we'd be OK to require a manual wiring up in that case.

My summary of the main questions around (b):
* We'd need to come up with an API and prototype it (there's a strawman in https://bugzilla.mozilla.org/show_bug.cgi?id=1479908#c8)
* What would migration for existing code look like?
(Reporter)

Comment 3

8 months ago
As for migration of existing code to (b): maybe there's a version where we move the actual <command>s to a JS API but then support [command=foo] attribute for nodes defined in markup as a way to ease migration. Then for dynamically added nodes or changes to [command] attribute on an existing node via setAttribute you'd need to interact with the API directly.
(Reporter)

Comment 4

8 months ago
In a new window for browser.xul:

> document.querySelectorAll("command").length
> 102

Checking in the Inspector, these are all grouped inside of 4 main commandsets:

1) mainCommandSet
2) editMenuCommands
3) placesCommands
4) One without an ID, but it contains downloads-related commands like downloadsCmd_doDefault
(Reporter)

Updated

8 months ago
Blocks: 1486144

Comment 5

8 months ago
So, XUL command elements and the related dispatching infrastructure help particularly when:
 1. several places in the user interface (button elements, menu item elements, keyboard shortcuts)
 2. can invoke a defined command (cmd_copy)
 3. whose availability and effect may vary depending on the focused element in the window.

Some nice additional functionality is the awareness of the keyboard shortcut for each command so we can display it beside the menu item label.

Some thoughts, especially if we go with (b) with one object per command per window:
 - When there's only one triggering element and one effect we don't need to use commands
   (for example most of the Downloads Panel commands are in this category)
 - Broadcasting the "oncommand" attribute may be superfluous
    - It's often just goDoCommand(name)
    - For Custom Elements we may prefer addEventListener(() => goDoCommand(name))
    - It doesn't change dynamically
 - We can use Cu.getWeakReference so we don't need to unregister observing nodes
 - We can support registering callback functions instead of just nodes
   (this allows for updating more complex state when the enabled/disabled state changes)
 - We may reuse the same global object map for command dispatching
   (although it may also make sense to use a separate data structure)

An example of the data structure with command dispatching for each command:

{
  cmd_copy: {
    shortcutKey: ...,
    controllers: [
      { hasFocus/handlesCommand, commandEnabled, doCommand },
      { hasFocus/handlesCommand, commandEnabled, doCommand },
    ],
    elementsToDisableWhenCommandDisabled: [
      WeakReference(node),
      WeakReference(node),
    ],
    elementsToHideWhenCommandDisabled: [
      WeakReference(node),
      WeakReference(node),
    ],
    callbacksToInvokeWhenCommandStateChanges: [
      callback,
      callback,
    ],
  },
  cmd_paste: {
    ...
  },
}
Depends on: 1486613

Updated

7 months ago
Depends on: 1492417
(Assignee)

Updated

7 months ago
Depends on: 1493210
(Assignee)

Comment 6

6 months ago
The majority of the XUL broadcaster logic is moved out of XULDocument and
into a separate class (XULBroadcastManager). The hookup points for when
listeners need to be created and listeners need to be notified is now
handled by the XULElement itself and nsDocument. To avoid any overhead,
the XULBroadcastManager is only ever created when a document uses a
listener.

The new approach does have the disadvantage that broadcasting can now only
work with XULElements, but going forward we'd like to discontinue this
feature and rely on MutationObservers to implement similar things.

One test had to be modified to use XUL elements instead of HTML elements
because of the reason noted above.
(Reporter)

Updated

6 months ago
Assignee: nobody → bdahl
Status: NEW → ASSIGNED

Comment 7

6 months ago
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/394a1e5544f6
Support XUL broadcasters in non-XUL documents. r=smaug

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/394a1e5544f6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.