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)
Toolkit
Toolbars and Toolbar Customization
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
Details
Attachments
(2 files, 4 obsolete files)
1.75 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
iannbugzilla
:
review+
dveditz
:
approval1.9.2.1+
dveditz
:
approval1.9.1.8+
|
Details | Diff | Splinter Review |
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)
Comment 1•15 years ago
|
||
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.
Comment 3•15 years ago
|
||
(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
Comment 5•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #409958 -
Flags: review?(dao) → review-
Comment 9•15 years ago
|
||
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•15 years ago
|
||
> 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.
Assignee | ||
Comment 11•15 years ago
|
||
(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.
Assignee | ||
Comment 12•15 years ago
|
||
(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.
Assignee | ||
Comment 14•15 years ago
|
||
(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•15 years ago
|
||
> 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.
Assignee | ||
Comment 16•15 years ago
|
||
(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).
Assignee | ||
Comment 17•15 years ago
|
||
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)
Comment 18•15 years ago
|
||
(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.
Assignee | ||
Comment 19•15 years ago
|
||
(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 21•15 years ago
|
||
> Cloning includes the disabled attribute, so yes.
So removing the disabled attribute should be sufficient was what I was thinking but IanN confused me.
Assignee | ||
Comment 22•15 years ago
|
||
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•15 years ago
|
||
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-
Comment 24•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 26•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
> 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•15 years ago
|
||
(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.
Assignee | ||
Comment 30•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 34•15 years ago
|
||
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 35•15 years ago
|
||
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+
Assignee | ||
Comment 36•15 years ago
|
||
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
Assignee | ||
Comment 37•15 years ago
|
||
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
Assignee | ||
Comment 38•15 years ago
|
||
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?
Comment 39•15 years ago
|
||
Any particular reason to prefer querySelectorAll to getElementsByAttribute?
Comment 40•15 years ago
|
||
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?
Comment 41•15 years ago
|
||
> Any particular reason to prefer querySelectorAll to getElementsByAttribute?
Earlier iterations of this patch dealt with more than one attribute.
Comment 42•15 years ago
|
||
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 43•15 years ago
|
||
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 44•15 years ago
|
||
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+
Assignee | ||
Comment 45•15 years ago
|
||
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]
Comment 46•15 years ago
|
||
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"
Assignee | ||
Comment 47•15 years ago
|
||
(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•15 years ago
|
||
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+
Comment 49•15 years ago
|
||
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 51•15 years ago
|
||
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!
Assignee | ||
Comment 52•15 years ago
|
||
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]
status1.9.1:
--- → .8-fixed
status1.9.2:
--- → .1-fixed
Updated•15 years ago
|
Attachment #411297 -
Flags: approval1.9.2.2+ → approval1.9.2.1+
Updated•15 years ago
|
Attachment #411297 -
Flags: approval1.9.2?
You need to log in
before you can comment on or make changes to this bug.
Description
•