335375 - changing accesskey on a <broadcaster> via an overlay doesn't propagate to its <toolbarbutton>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Robert Kaiser]

Not sure if that's a bug or intentional, but it makes overlaying problematical.

I'm writing an overlay for adding a menubar to toolkit JS console in toolkit-based SeaMonkey. During that, we saw that the accesskeys on the console mode toolbar clash with menubar accesskeys, so I need to overlay those as well.

Those accesskeys are defined via <broadcaster> elements that have IDs and DOMI shows that I can successfully change them with the overlay. The <toolbarbutton> connected with that broadcaster still shows the "old" accesskey though, as the change never gets propagated from the broadcaster to the toolbarbutton.

Comment 1

[Robert Kaiser]

bug 335383 has a patch now that shows this behavior. DOMI tells the accesskeys on the broadcasters get overlaid correctly, but the toolbarbuttons still have the pre-overlay accesskeys set.

Comment 2

[Ian Neal]

I guess we should probably look at doing something similar to http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/src/nsXULPopupManager.cpp#1615 once overlays have finished loading and looking for toolbarbutton/toolbaritem children of a toolbar that have a command attribute.
Some sort of hook into nsXULDocument.cpp?

Comment 3

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change v1.0

This just copies the code from AttributeChanged to broadcast the change for when a command/observes element gets overlays.

Comment 4

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change with helper v1.0

Does the same as the other patch but uses a helper.

Comment 5

[[e-mail]]

Comment on attachment 526441 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.0

>         rv = aTargetNode->SetAttr(nameSpaceID, attr, prefix, value, aNotify);
>         if (NS_FAILED(rv)) return rv;
>+
>+        mDocument->BroadcastAttributeChangeFromOverlay(aTargetNode, nameSpaceID,
>+                                                       attr, value);
You don't actually need to do this if aNotify is true, because SetAttr will call AttributeChanged in that case. (This happens when loading a dynamic overlay, e.g. in the preferences window.)

>+    if (!aNode->NodeInfo()->Equals(nsGkAtoms::observes, kNameSpaceID_XUL) &&
>+        !aNode->NodeInfo()->Equals(nsGkAtoms::command, kNameSpaceID_XUL))
>+        return;
Not sure what this is for.

>+        mDelayedAttrChangeBroadcasts.AppendElement(delayedUpdate);
By using this method to schedule a broadcast you'll actually trigger a broadcast event which might not be expected. (I don't know whether it's expected for a dynamic overlay but it has to go through the AttributeChanged path which will broadcast anyway.) You might be able to get away with calling SetAttr(nameSpaceID, attr, prefix, value, PR_FALSE) on the listener element (only if you're filtering this call to come from a real overlay of course).

Comment 6

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change with helper v1.1

Changes since v1.0:
* Only call helper if aNotify is not true.
* Removed unneeded command/observes test from helper.
* Use SetAttr instead of delayedUpdate in helper.

Comment 7

[[e-mail]]

Comment on attachment 526501 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.1

>+        nsAutoString currentValue;
>+        PRBool hasAttr = l->GetAttr(kNameSpaceID_None, aAttribute,
>+                                    currentValue);
>+        // We need to update listener only if we're
>+        // (1) removing an existing attribute,
>+        // (2) adding a new attribute or
>+        // (3) changing the value of an attribute.
>+        if (!hasAttr || !aValue.Equals(currentValue)) {
Since we're not notifying listeners, I don't think we need this, we can just call SetAttr directly. Apart from this the patch looks OK to me, except you'll need a test, probably a chrome mochitest, since that's the easiest way to get overlays working.

Comment 8

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change with helper v1.2

Changes since v1.1:
* Remove unneeded check for existing attribute and value.

Comment 9

[Ian Neal]

Attachment: Test for bug 335375 patch v1.0

This patch:
* Tests for accesskey attribute existence and value on both command and toolbarbutton elements

Without fix, passes 2 and fails 2
With fix, passes all 4

Comment 10

[[e-mail]]

Comment on attachment 526612 [details] [diff] [review]
Test for bug 335375 patch v1.0

>+<?xml-stylesheet href="/tests/SimpleTest/test.css" type="text/css"?>
Nit: chrome://mochikit/content/tests/SimpleTest/test.css

>+        ok(cmd.hasAttribute("accesskey"), "command missing accesskey");
>+        is(cmd.getAttribute("accesskey"), "C", "command has wrong accesskey");
>+        ok(button.hasAttribute("accesskey"), "button missing accesskey");
>+        is(button.getAttribute("accesskey"), cmd.getAttribute("accesskey"),
>+           "button has different accesskey to command");
I'm not 100% convinced it's worth checking both hasAttribute and getAttribute. Also I don't like the wording, it looks odd to see TEST-PASS command missing accesskey; for instance for the last test you could write something like "checking command and button have same accesskey". r=me with that fixed.

It might be worth changing the attribute in script to show that this works.

Comment 11

[[e-mail]]

Comment on attachment 526611 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2

>+        nsCOMPtr<nsIDOMElement> listenerEl = do_QueryReferent(bl->mListener);
>+        nsCOMPtr<nsIContent> l = do_QueryInterface(listenerEl);
I've just noticed that we don't use listenerEl so we can go straight to nsCOMPtr<nsIContent> l = do_QueryRefernt(bl->mListener);

Comment 12

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change with helper v1.2a

Changes since v1.2:
* Removed unused listenerEl.

Carrying forward r+

Comment 13

[Ian Neal]

Comment on attachment 526624 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2a

Requesting additional review

Comment 14

[Ian Neal]

Attachment: Test for bug 335375 patch v1.1 [Checked in: Comment 22]

Changes since test v1.0:
* Removed tests for hasAttribute.
* Changed wording for TEST-PASS comments.
* Added setting an accesskey and testing that it is inherited.

Carrying forward r+

Comment 15

[Ian Neal]

Comment on attachment 526625 [details] [diff] [review]
Test for bug 335375 patch v1.1 [Checked in: Comment 22]

Requesting an additional review

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 526624 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2a


> nsresult
>+nsXULDocument::BroadcastAttributeChangeFromOverlay(nsIContent* aNode,
>+                                                   PRInt32 aNameSpaceID,
>+                                                   nsIAtom* aAttribute,
>+                                                   nsIAtom* aPrefix,
>+                                                   const nsAString& aValue)
>+{
>+    nsresult rv = NS_OK;
>+
>+    nsCOMPtr<nsIDOMElement> domele = do_QueryInterface(aNode);
>+    if (!domele || !mBroadcasterMap || !CanBroadcast(aNameSpaceID, aAttribute))
>+        return rv;
Since QI on cycle collectable elements is slowish, better to do it only if
needed.
So please check mBroadcasterMap and CanBroadcast(aNameSpaceID, aAttribute)
before using domele.


>+
>+    BroadcasterMapEntry* entry = static_cast<BroadcasterMapEntry*>
>+        (PL_DHashTableOperate(mBroadcasterMap, domele.get(), PL_DHASH_LOOKUP));
>+    if (!PL_DHASH_ENTRY_IS_BUSY(entry))
>+        return rv;
>+
>+    // We've got listeners: push the value.
>+    PRInt32 i;
>+    for (i = entry->mListeners.Count() - 1; i >= 0; --i) {
>+        BroadcastListener* bl = static_cast<BroadcastListener*>
>+            (entry->mListeners[i]);
>+
>+        if ((bl->mAttribute != aAttribute) &&
>+            (bl->mAttribute != nsGkAtoms::_asterix))
>+            continue;
>+
>+        nsCOMPtr<nsIContent> l = do_QueryReferent(bl->mListener);
>+        if (l) {
>+            rv = l->SetAttr(aNameSpaceID, aAttribute,
>+                            aPrefix, aValue, PR_FALSE);
So when do we notify about the attr change?

Could you rather call BroadcastAttributeChangeFromOverlay asynchronously or using a script runner and then
notify when calling SetAttr. (In that case check CanBroadcast and mBroadcasterMap as early as possible)
And to keep everything safe, collect first all the listeners to a
nsCOMArray and then in a separate loop call SetAttr to objects in that array.

Comment 17

[[e-mail]]

(In reply to comment #16)
>(From update of attachment 526624 [details] [diff] [review])
>>+            rv = l->SetAttr(aNameSpaceID, aAttribute,
>>+                            aPrefix, aValue, PR_FALSE);
>So when do we notify about the attr change?
We don't, but then we didn't notify about the attr change that we want to propagate, because we haven't started layout yet. See the very first line of added code.

Comment 18

[[e-mail]]

(In reply to comment #17)
>(In reply to comment #16)
>>(From update of attachment 526624 [details] [diff] [review])
>>>+            rv = l->SetAttr(aNameSpaceID, aAttribute,
>>>+                            aPrefix, aValue, PR_FALSE);
>>So when do we notify about the attr change?
>We don't, but then we didn't notify about the attr change that we want to
>propagate, because we haven't started layout yet. See the very first line of
>added code.
Also not directly relevant, but the original workaround is to overlay the equivalent attributes on the listener elements directly, which of course also happens without notifying.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 526624 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2a

Ok, Neil explained this well
I'd still like to see QI to nsIDOMElement happen after
!mBroadcasterMap and !CanBroadcast() checks.

Comment 20

[Ian Neal]

Attachment: Broadcast changes to command/overlay elements on change with helper v1.2b [Checked in: Comment 21]

Changes since v1.2a:
* Check for mBroadcasterMap and canBroadcast before QI to nsIDOMElement.

Carrying forward r+ from Neil and smaug

Comment 21

[Ian Neal]

Comment on attachment 527007 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2b [Checked in: Comment 21]

http://hg.mozilla.org/mozilla-central/rev/a015d7120b5a

Comment 22

[Ian Neal]

Comment on attachment 526625 [details] [diff] [review]
Test for bug 335375 patch v1.1 [Checked in: Comment 22]

http://hg.mozilla.org/mozilla-central/rev/c618f3084efb

Comment 23

[Ian Neal]

Comment on attachment 527007 [details] [diff] [review]
Broadcast changes to command/overlay elements on change with helper v1.2b [Checked in: Comment 21]

Fixes an issue to do attributes on command elements in overlays working correctly

Comment 24

[Ian Neal]

Comment on attachment 526625 [details] [diff] [review]
Test for bug 335375 patch v1.1 [Checked in: Comment 22]

Adds a test to check that that attributes on command elements in overlays work correctly

Comment 25

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

Why does this need to land on aurora? Patches should only land there if they're  critical fixes for some reason, and this bug's patch doesn't appear to be so.

Comment 26

[Justin Wood (:Callek)]

(In reply to comment #25)
> Why does this need to land on aurora? Patches should only land there if they're
>  critical fixes for some reason, and this bug's patch doesn't appear to be so.

Gavin, as a SeaMonkey driver and knowing the aurora schedule/plan, I feel safe letting this slip past aurora and would actually agree with you in that this should not land.

That said, this is an issue that will (does) affect SeaMonkey a11y support, so would be *nice* to have there.

Comment 27

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

Lots of things are "nice to have", can't backport them all. There's another train coming, and it's coming soon! :)