Closed
Bug 380990
Opened 18 years ago
Closed 18 years ago
for Bookmarks menu, unable to open submenus (AddBinding broken)
Categories
(Core :: XBL, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: moco, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
for Bookmarks menu, unable to open submenus. For example, the Bookmarks Toolbar Folder submenu won't show.
but, you can see in the bookmark manager window that it has items.
dietrich spotted this in his recent mac build, I and am able to reproduce it, too.
note, for sub folders of the personal toolbar, and the menupopups we create, I can see subfolders.
Reporter | ||
Comment 1•18 years ago
|
||
note, this appears to be mac only (places bookmarks)
I see it on: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a5pre) Gecko/20070515 Minefield/3.0a5pre
Target Milestone: --- → Firefox 3 alpha5
Comment 2•18 years ago
|
||
So this seems like a regression from bug 53901, the mac code path for this calls addBinding on the xul popup (as a workaround for an old menu menu bug), then in LoadBindings, NODE_FORCE_XBL_BINDINGS is not set so we call GetCurrentDoc (rather than GetOwnerDoc), which is null, probably because the popup has no frame.
Blocks: 53901
Comment 3•18 years ago
|
||
Or rather, because we call addBinding before the popup element is appended.
Comment 4•18 years ago
|
||
Attachment #265120 -
Flags: review?(sspitzer)
Assignee | ||
Comment 5•18 years ago
|
||
Uh... how did this ever work before? We used to always call GetDocument(), and it returned non-null when it does now, or when we have NODE_FORCE_XBL_BINDINGS set now. Unless _something_ got confused.
The workaround is nice and all, but we need to figure out why this broke. The change for bug 53901 was explicitly designed to NOT break anything like this.
Assignee | ||
Comment 6•18 years ago
|
||
Is the popup in question coming about as a result of a CloneNode call? Or no? If it's not, then how come the binding was ever getting attached?
Comment 7•18 years ago
|
||
(In reply to comment #5)
> Uh... how did this ever work before? We used to always call GetDocument(), and
> it returned non-null when it does now, or when we have NODE_FORCE_XBL_BINDINGS
> set now. Unless _something_ got confused.
Hrm? LoadBindings used to always call GetOwnerDocument if i read that patch right...
(In reply to comment #6)
> Is the popup in question coming about as a result of a CloneNode call? Or no?
> If it's not, then how come the binding was ever getting attached?
no, it's created using createElement.
Comment 8•18 years ago
|
||
The workaround is here so we can enable places-bookmarks today, fyi.
Comment 9•18 years ago
|
||
Yeah, see:
- nsCOMPtr<nsIDocument> document = aContent->GetOwnerDoc();
+ nsCOMPtr<nsIDocument> document;
+ if (aContent->HasFlag(NODE_FORCE_XBL_BINDINGS)) {
+ document = aContent->GetOwnerDoc();
+ }
+ else {
+ document = aContent->GetCurrentDoc();
+ }
Assignee | ||
Comment 10•18 years ago
|
||
Oh, I see. No, it hasn't always done that; see the checkin history. The fact that it did at all is an accident, basically.
We probably do want this to work, but I'd need to review the patch for bug 53901 in its entirety to say how.
Assignee | ||
Comment 11•18 years ago
|
||
OK, so I see what happened here. The code didn't really match the conceptual model of "how XBL ought to work, and always has (until Gecko 1.8)" we had, due to the checkin for bug 265086, and places was relying on the way the code worked, not the way it was "supposed" to work, so when the code was changed ("fixed"?) to fit the conceptual model, things broke.
It looks like LoadBindings() has the following callers:
1) CSSFrameConstructor
2) DOMClassInfo
3) nsBindingManager::AddLayeredBinding
The only caller of nsBindingManager::AddLayeredBinding is nsDocument::AddBinding.
In the frame constructor, the node will be in the document. DOMClassInfo does its own check of the NODE_FORCE_XBL_BINDINGS bit. So the only way to get into LoadBindings with a node that is not in a document and doesn't have NODE_FORCE_XBL_BINDINGS is via nsDocument::AddBinding. I'd argue that we should in fact just attach the binding in that case, since the caller is pretty explicitly asking for it (so the odd interactions I got when I tried to make createElement() attach bindings automatically shouldnot arise, in general).
So we should just back out the change to nsXBLService::LoadBindings, in my opinion.
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → general
Component: Places → XBL
Product: Firefox → Core
QA Contact: places → ian
Summary: for Bookmarks menu, unable to open submenus → for Bookmarks menu, unable to open submenus (AddBinding broken)
Target Milestone: Firefox 3 alpha5 → mozilla1.9alpha5
Assignee | ||
Updated•18 years ago
|
Assignee: general → bzbarsky
Priority: -- → P1
Assignee | ||
Comment 12•18 years ago
|
||
Attachment #265131 -
Flags: superreview?(jonas)
Attachment #265131 -
Flags: review?(jonas)
Comment on attachment 265131 [details] [diff] [review]
Like so
Yeah, I was wondering about that when I made the change. But since it used to be GetDocument() not long ago it seemed like the right thing to do.
But it makes sense to do this.
Attachment #265131 -
Flags: superreview?(jonas)
Attachment #265131 -
Flags: superreview+
Attachment #265131 -
Flags: review?(jonas)
Attachment #265131 -
Flags: review+
Assignee | ||
Comment 14•18 years ago
|
||
Fixed.
I'm _so_ looking forward to XBL2, when the "how things should work" information will live someplace other than a few people's heads... and we'll have a test suite. Speaking of which, someone want to write a regression test for this? My actual xbl-writing fu is weak. :(
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment 15•18 years ago
|
||
Comment on attachment 265120 [details] [diff] [review]
working workaround - call addBinding after appendChild
Thanks Boris!
Attachment #265120 -
Attachment is obsolete: true
Attachment #265120 -
Flags: review?(sspitzer)
You need to log in
before you can comment on or make changes to this bug.
Description
•