Closed Bug 525373 Opened 15 years ago Closed 15 years ago

Make customizable toolbars play nicely with children with disabled attributes

Categories

(Toolkit :: Toolbars and Toolbar Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- .1-fixed
status1.9.1 --- .8-fixed

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attachment #409238 - Flags: review?(gavin.sharp)
What does "firstChild is a deck" have to do with "remove some attributes from its children"?
(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.
(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.
(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
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.
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.
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.
Attachment #409238 - Flags: review?(gavin.sharp)
Okay, instead of singling out just certain children this patch removes the disabled/observes attributes from all children.
Attachment #409238 - Attachment is obsolete: true
Attachment #409958 - Flags: review?(dao)
Attachment #409958 - Flags: review?(dao) → review-
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.
> 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.
(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.
(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.
Why would they be affected by elements in a different document?
(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.
> 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.
(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).
Changes since patch v0.2:
* Now uses a querySelectorAll on the aWrapper element using all 5 "problem" attributes.
Attachment #409958 - Attachment is obsolete: true
Attachment #410994 - Flags: review?(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.
(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?
Cloning includes the disabled attribute, so yes.
> Cloning includes the disabled attribute, so yes.
So removing the disabled attribute should be sufficient was what I was thinking but IanN confused me.
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 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.
Attachment #410994 - Flags: review?(dao) → review-
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.
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.
(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.
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.
> 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.
(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.
(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.
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.
(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?
(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.
Attached patch a KISS patch v0.3a (obsolete) — Splinter Review
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).
Attachment #410994 - Attachment is obsolete: true
Attachment #411195 - Flags: review?(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");
> });
Attachment #411195 - Flags: review?(dao) → review+
Changes since v0.3a:
* Removed special case for observes.
* Switched to Array.forEach.

Carrying forward r=
Attachment #411292 - Flags: review+
Attachment #411195 - Attachment is obsolete: true
Summary: Make customizable toolbars play nicely with toolbaritems that contain a deck → Make customizable toolbars play nicely with children with disabled attributes
Comment on attachment 411292 [details] [diff] [review]
forEach patch v0.3b [Checkin: Comment 37]

http://hg.mozilla.org/mozilla-central/rev/274e471f0c25
Attachment #411292 - Attachment description: forEach patch v0.3b → forEach patch v0.3b [Checkin: Comment 37]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3
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.
Attachment #411297 - Flags: review+
Attachment #411297 - Flags: approval1.9.1.7?
Attachment #411297 - Flags: approval1.9.1.6?
Any particular reason to prefer querySelectorAll to getElementsByAttribute?
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.
Attachment #411297 - Flags: approval1.9.1.6?
> Any particular reason to prefer querySelectorAll to getElementsByAttribute?
Earlier iterations of this patch dealt with more than one attribute.
You'll need to request approval for 1.9.2 as well.
Target Milestone: mozilla1.9.3 → mozilla1.9.3a1
Attachment #411297 - Flags: approval1.9.2?
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.
Attachment #411297 - Flags: approval1.9.2.1?
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?
Attachment #411297 - Flags: approval1.9.2.1? → approval1.9.2.1+
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.
Attachment #411297 - Attachment description: forEach patch v0.3b for 1.9.1 → forEach patch v0.3b for 1.9.1/2 [1.9.2.x Checkin: Comment 45]
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"
(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 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
Attachment #411297 - Flags: approval1.9.1.8? → approval1.9.1.8+
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.
We should back it out immediately if that's the case.
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 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
Attachment #411297 - Attachment description: forEach patch v0.3b for 1.9.1/2 [1.9.2.x Checkin: Comment 45] → forEach patch v0.3b for 1.9.1/2 [1.9.2.1 Checkin: Comment 45, 1.9.1.9 Checkin: Commment 52]
Attachment #411297 - Flags: approval1.9.2.2+ → approval1.9.2.1+
Attachment #411297 - Flags: approval1.9.2?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: