525373 - Make customizable toolbars play nicely with children with disabled attributes

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Ian Neal]

Attachment: Remove disabled/observes attributes from deck children patch v0.1

At the moment if the toolbarpalette has a toolbaritem in that contains a deck then the customize toolbar code does correctly remove attributes of elements within that deck - see http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1683 or http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1699 for examples.
This means in the customize toolbar window they can show up as disabled.

Comment 1

[Dão Gottwald [:dao]]

What does "firstChild is a deck" have to do with "remove some attributes from its children"?

Comment 2

[Ian Neal]

(In reply to comment #1)
> What does "firstChild is a deck" have to do with "remove some attributes from
> its children"?
Well if the firstChild is a deck, then its children will be elements that have attributes like "disabled" and "observes" which can mess up the palette display.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> if the firstChild is a deck, then its children will be elements that have
> attributes like "disabled" and "observes"

Why exactly? Decks can contain anything.

Comment 4

[Ian Neal]

(In reply to comment #3)
> (In reply to comment #2)
> > if the firstChild is a deck, then its children will be elements that have
> > attributes like "disabled" and "observes"
> 
> Why exactly? Decks can contain anything.
Really? In toolbar palettes? Please give an example

Comment 5

[Dão Gottwald [:dao]]

It could be labels, textboxes or just containers for complex widgets. I guess blindly removing the attributes doesn't hurt, as long as these aren't the primary copies of the elements (are they?). Either way it worries me that we make more bogus assumptions about app-specific widgets.

Comment 6

[Phil Ringnalda (:philor)]

They aren't the primary copy.

The primary copy either lives in a toolbar, or in a toolbarpalette, in the window's document. Anything going through cleanUpItemForPalette is a cloned element that the dialog's document created with importNode, either when the dialog was first opened because the element wasn't on a toolbar, or when it's dragged off a toolbar and dropped on the palette. Its only existence is then in the dialog - when it's dragged onto a toolbar, the ondrop handler just extracts the id and tells the toolbar to insertItem with that id, which moves the child element in the window's document into the toolbar.

I'd be happy to see a solution that doesn't make bogus assumptions about app-specific widgets like the Firefox throbber, addressbar, and search bar (oh, and our deck items), but the only two solutions I see are to try to remove every possibly risky attribute from every child of the toolbaritem, which'll be slower for deep things like the Firefox addressbar with its eleven children, and still bogus since it'll only remove attributes that we know are a problem for app-specific items, or to make the opener provide a cleanUpItemForPalette function which knows about its toolbaritems, which still wouldn't work for odd things added by extensions. Probably the mythical toolbar rewrite ought to make toolbaritems provide their own cleanUpMyselfForPalette function, but I don't expect to see that rewrite any time in the predictable future.

Comment 7

[Dão Gottwald [:dao]]

Of course, preexisting bad practice isn't a good reason to continue this. But I'm also not sure what location and search bar specific code you're referring to. Maybe the removal of the width attribute? The throbber part is dead as far as Firefox is concerned, and the right way to address this is in themes' browser.css.

Comment 8

[Ian Neal]

Attachment: Remove attributes that can cause display issues from all children patch v0.2

Okay, instead of singling out just certain children this patch removes the disabled/observes attributes from all children.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 409958 [details] [diff] [review]
Remove attributes that can cause display issues from all children patch v0.2

I think aItem.querySelectorAll("[disabled],[observes]") is a faster way to do what you want. That said, I'm not sure why "observers" is in that list while "command" isn't.

Comment 10

[Philip Chee]

> I'm not sure why "observers" is in that list while
> "command" isn't.
Come to think of it, I don't understand why "observes" is in that list either.

Comment 11

[Ian Neal]

(In reply to comment #9)
> (From update of attachment 409958 [details] [diff] [review])
> I think aItem.querySelectorAll("[disabled],[observes]") is a faster way to do
> what you want. That said, I'm not sure why "observers" is in that list while
> "command" isn't.
Good question, I seem to remember when I discussed this with Neil he suggested I should do all 5, thanks for the reminder. New patch coming soon.

Comment 12

[Ian Neal]

(In reply to comment #10)
> > I'm not sure why "observers" is in that list while
> > "command" isn't.
> Come to think of it, I don't understand why "observes" is in that list either.
As you don't want elements in the customizeToolbar dialog being affected by elements outside of the dialog as it might cause display issues.

Comment 13

[Dão Gottwald [:dao]]

Why would they be affected by elements in a different document?

Comment 14

[Ian Neal]

(In reply to comment #13)
> Why would they be affected by elements in a different document?
Outside of the dialog not outside of ones that elements in the dialog can still observe and cause an issue.

Comment 15

[Philip Chee]

> Outside of the dialog not outside of ones that elements in the dialog can still
> observe and cause an issue.

I don't understand this comment.

Comment 16

[Ian Neal]

(In reply to comment #15)
> > Outside of the dialog not outside of ones that elements in the dialog can still
> > observe and cause an issue.
> 
> I don't understand this comment.
Elements do not have to be in the dialog box's document to be observable (otherwise there would not be problems with elements like Junk and Delete).

Comment 17

[Ian Neal]

Attachment: Using querySelectorAll patch v0.3

Changes since patch v0.2:
* Now uses a querySelectorAll on the aWrapper element using all 5 "problem" attributes.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #16)
> Elements do not have to be in the dialog box's document to be observable

Are you saying that an observer and the observed element don't need to be in the same document?

> (otherwise there would not be problems with elements like Junk and Delete).

Sure, since the disabled attribute is carried over.

Comment 19

[Ian Neal]

(In reply to comment #18)
> (In reply to comment #16)
> > Elements do not have to be in the dialog box's document to be observable
> 
> Are you saying that an observer and the observed element don't need to be in
> the same document?
> 
> > (otherwise there would not be problems with elements like Junk and Delete).
> 
> Sure, since the disabled attribute is carried over.

Examples being various elements from mailnews (Junk, Delete, Mail Views). Maybe it's because they are being cloned?

Comment 20

[Dão Gottwald [:dao]]

Cloning includes the disabled attribute, so yes.

Comment 21

[Philip Chee]

> Cloning includes the disabled attribute, so yes.
So removing the disabled attribute should be sufficient was what I was thinking but IanN confused me.

Comment 22

[Ian Neal]

Unfortunately the patch that gave us customizeToolbar gives no clue as to why they thought "observes" attribute was considered to be a "bad thing". I am happy not to have it in, but I'll leave it to Dão confirm.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 410994 [details] [diff] [review]
Using querySelectorAll patch v0.3

I don't think we should remove command and observes deeply without knowing why. Maybe we shouldn't even do it for the item itself.

I also don't see why type and width are in that list.

Comment 24

[Philip Chee]

These are just cloned nodes in the customize palette window. Here they are just placeholders, so we just want a visual representation of those items without any behavioural quirks. In this situation removing command and observes elements removes the possibility of unwanted side effects interfering with the customizeToolbar code.

Comment 25

[Dão Gottwald [:dao]]

It doesn't remove that possibility, as there's still <xul:observes/>. I think this can be addressed easily by making sure the ids in customizeToolbar.xul aren't ambiguous.

Comment 26

[Ian Neal]

(In reply to comment #23)
> (From update of attachment 410994 [details] [diff] [review])
> I don't think we should remove command and observes deeply without knowing why.
> Maybe we shouldn't even do it for the item itself.
> 
> I also don't see why type and width are in that list.
I can see why width is in the list as that is most likely to cause issues when cloning elements into the dialog. I agree that type should not be in the list.

(In reply to comment #25)
> It doesn't remove that possibility, as there's still <xul:observes/>. I think
> this can be addressed easily by making sure the ids in customizeToolbar.xul
> aren't ambiguous.
I think changing all ids as you clone the elements across will mean lots of changes to css and would cause issues with css for some extensions/add-ons which used to "just work" but would now need additional css to show up properly in the customizeToolbar dialog.

Comment 27

[Dão Gottwald [:dao]]

I didn't say nor mean "changing all ids as you clone the elements", there's no point in doing that. I was referring to the elements that are part of the customizeToolbar.xul UI and could theoretically be observed.

Comment 28

[Philip Chee]

> It doesn't remove that possibility, as there's still <xul:observes/>. I think

This is descending into bikeshedding territory. KISS and just remove the most obvious attributes. The edge cases if any can be followed up if anyone files a bug on an actual incident.

> this can be addressed easily by making sure the ids in customizeToolbar.xul
> aren't ambiguous.
See Ians reply.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> > It doesn't remove that possibility, as there's still <xul:observes/>. I think
> 
> This is descending into bikeshedding territory. KISS and just remove the most
> obvious attributes. The edge cases if any can be followed up if anyone files a
> bug on an actual incident.

We don't have a single actual incident right now.

> > this can be addressed easily by making sure the ids in customizeToolbar.xul
> > aren't ambiguous.
> See Ians reply.

I just replied to that.

Comment 30

[Ian Neal]

(In reply to comment #27)
> I didn't say nor mean "changing all ids as you clone the elements", there's no
> point in doing that. I was referring to the elements that are part of the
> customizeToolbar.xul UI and could theoretically be observed.
Ah, you meant the dialog before any cloned elements get put into it.
CustomizeToolbarKeyset, cmd-close1, cmd-close2, cmd-close3, main-box, palette-box, modelist, smallicons and donebutton all seem to be unique.
(In reply to comment #29)
> (In reply to comment #28)
> > > It doesn't remove that possibility, as there's still <xul:observes/>. I think
> > 
> > This is descending into bikeshedding territory. KISS and just remove the most
> > obvious attributes. The edge cases if any can be followed up if anyone files a
> > bug on an actual incident.
> 
> We don't have a single actual incident right now.
Well there wouldn't be, no changes have been made yet, Ratty was referring to future incidents once this change has gone in.

Comment 31

[Dão Gottwald [:dao]]

No, he was referring to incidents with <xul:observes/>. I'm saying that we don't have such incidents with the command and observes attributes either. So the KISS way would be to leave all these things alone.

Comment 32

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

(In reply to comment #30)
> cmd-close1, cmd-close2, cmd-close3, main-box, donebutton

It's not only about whether these are unique, but also whether they're common/generic enough that they could potentially be listed in <observes> of items being added to the palette. We could give them a "customize-" prefix to mitigate this, but I'm not sure that's really necessary.

(In reply to comment #23)
> (From update of attachment 410994 [details] [diff] [review])
> I don't think we should remove command and observes deeply without knowing why.

I understand that there may be little point to doing it, but I also don't really see any big downsides. I think I'd be fine with that patch, but without "type" and "width".

I'd also be fine with leaving all the other code as is, but replacing |aItem.removeAttribute("disabled");| with the querySelectorAll deep-removal of just "disabled", which I think would satisfy everyone?

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> I'd also be fine with leaving all the other code as is, but replacing
> |aItem.removeAttribute("disabled");| with the querySelectorAll deep-removal of
> just "disabled", which I think would satisfy everyone?

Indeed, I think that's all we need for this bug.

Comment 34

[Ian Neal]

Attachment: a KISS patch v0.3a

This patch:
* Simply replaces the aItem.removeAttribute("disabled") with the querySelectorAll deep-removal of just "disabled".

I did also try getting rid of all the removeAttribute lines and tested against Firefox, Thunderbird and SeaMonkey and that worked (if we did want some code clean up).

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 411195 [details] [diff] [review]
a KISS patch v0.3a

>   if (aItem.localName == "toolbaritem" && aItem.firstChild) {
>     aItem.firstChild.removeAttribute("observes");
>   }

Can you get rid of this? It seems completely half-assed to do this only for the first child and the observes attribute.

>+  var items = aWrapper.querySelectorAll("[disabled]");
>+  for (var i = 0; i < items.length; ++i)
>+    items[i].removeAttribute("disabled");

"items" seems misleading in this context, as it reminds me of "toolbaritem".
Also, this might be a good candidate for forEach:

> Array.forEach(aWrapper.querySelectorAll("[disabled]"), function (node) {
>   node.removeAttribute("disabled");
> });

Comment 36

[Ian Neal]

Attachment: forEach patch v0.3b [Checkin: Comment 37]

Changes since v0.3a:
* Removed special case for observes.
* Switched to Array.forEach.

Carrying forward r=

Comment 37

[Ian Neal]

Comment on attachment 411292 [details] [diff] [review]
forEach patch v0.3b [Checkin: Comment 37]

http://hg.mozilla.org/mozilla-central/rev/274e471f0c25

Comment 38

[Ian Neal]

Attachment: forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

Equivalent patch for 1.9.1.x tree, fixes issues with the customizeToolbar dialog for TB and SM. Low risk. Requesting a= for 1.9.1.x tree.

Comment 39

[[e-mail]]

Any particular reason to prefer querySelectorAll to getElementsByAttribute?

Comment 40

[Daniel Veditz [:dveditz]]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

too late for 1.9.1.6 but we'll check back when we're working on 1.9.1.7. We'll want to see that this is fixed on 1.9.2 first, though.

Comment 41

[Philip Chee]

> Any particular reason to prefer querySelectorAll to getElementsByAttribute?
Earlier iterations of this patch dealt with more than one attribute.

Comment 42

[Dão Gottwald [:dao]]

You'll need to request approval for 1.9.2 as well.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

Still waiting for a 1.9.2 fix first (don't want to fix in 3.5.x and then regress in 3.6.x). 1.9.2 approval is unlikely at this point, pretty much only blockers are getting in. Will take some IRC lobbying I think.

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

a=beltzner for the 1.9.2 branch

Do we really need it for 1.9,1?

Comment 45

[Ian Neal]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aab51a78e6e8

As far as 1.9.1.x is concerned, it fixes a minor display issue in the customize toolbar dialog for products that use decks within their toolbars (so TB and SM at least), with those icons showing as disabled in the dialog. So not a security issue or a regression, just a fixable blemish that makes the products look more polished.

Comment 46

[Steffen Wilberg]

toolkit/content/customizeToolbar.js
     1.7 - * is stripped of all properties that may adversely affect its
     1.8 + * is stripped of any aiitributes that may adversely affect its
http://hg.mozilla.org/releases/mozilla-1.9.2/diff/aab51a78e6e8/toolkit/content/customizeToolbar.js

typo: "aiitributes"

Comment 47

[Ian Neal]

(In reply to comment #46)
> toolkit/content/customizeToolbar.js
>      1.7 - * is stripped of all properties that may adversely affect its
>      1.8 + * is stripped of any aiitributes that may adversely affect its
> http://hg.mozilla.org/releases/mozilla-1.9.2/diff/aab51a78e6e8/toolkit/content/customizeToolbar.js
> 
> typo: "aiitributes"

Thanks for the catch, fixed - http://hg.mozilla.org/releases/mozilla-1.9.2/rev/26b0e84316fb

Comment 48

[Daniel Veditz [:dveditz]]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 49

[Mike Shaver (:shaver emeritus)]

I think this set of changes caused 1.9.2 builds to become Very Ugly when using Strata -- might affect other themes as well, Johnathan is looking into it, will file etc.

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

We should back it out immediately if that's the case.

Comment 51

[Mike Shaver (:shaver emeritus)]

False alarm, sorry -- I got somehow switched to another theme (probably testing personas with the hover-annoy bug) and THAT makes the strata-buddy addon stuff ugly.

Apologies!

Comment 52

[Ian Neal]

Comment on attachment 411297 [details] [diff] [review]
forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/31f97a231f23