Closed Bug 380990 Opened 17 years ago Closed 17 years ago

for Bookmarks menu, unable to open submenus (AddBinding broken)

Categories

(Core :: XBL, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: moco, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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
Blocks: 370099
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
Or rather, because we call addBinding before the popup element is appended.
Attachment #265120 - Flags: review?(sspitzer)
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.
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?
(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.
The workaround is here so we can enable places-bookmarks today, fyi.
Yeah, see:
-  nsCOMPtr<nsIDocument> document = aContent->GetOwnerDoc();
+  nsCOMPtr<nsIDocument> document;
+  if (aContent->HasFlag(NODE_FORCE_XBL_BINDINGS)) {
+    document = aContent->GetOwnerDoc();
+  }
+  else {
+    document = aContent->GetCurrentDoc();
+  }
 
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.
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: 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: general → bzbarsky
Priority: -- → P1
Attached patch Like soSplinter Review
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+
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: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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.

Attachment

General

Created:
Updated:
Size: