XBL should apply to nodes not in documents

RESOLVED WONTFIX

Status

()

Core
XBL
RESOLVED WONTFIX
14 years ago
5 years ago

People

(Reporter: bz, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

This can be done by using GetOwnerDoc() instead of GetDocument() appropriately,
I would think.  I have a patch, untested thus far.
Blocks: 53901
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha6
Created attachment 162655 [details] [diff] [review]
Patch, tested on that testcase
Attachment #162585 - Attachment is obsolete: true
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

Thoughts?  This is a big step towards being able to resolve bug 53901...
Attachment #162655 - Flags: superreview?(jst)
Attachment #162655 - Flags: review?(bryner)
Summary: XBL should apply to nodes not in documents → [FIX]XBL should apply to nodes not in documents
Target Milestone: mozilla1.8alpha6 → mozilla1.8alpha5
Actually, it looks like this breaks tabbed browsing.

The problem is that the addTab() method in tabbrowser creates a new <browser>
node.  With my patch, the browser binding is attached to this node at creation
time (by classinfo).  Then the binding constructor tries to set up session
history stuff and fails because there is no docshell (because XUL iframes create
the docshell on initial reflow, which has clearly not happened yet).  So
thereafter we have no session history and tabbrowser just sorta breaks in that
case (eg doesn't show the right label on the tab, etc).

I can try to hack the browser binding to hold off init if the constructor runs
when it's not in the document (and then to init when it's inserted in the
document, if I can figure out a decent way to detect this from XBL).

Alternately, I could make nsIFrameLoaderOwner/nsIFrameLoader scriptable so that
the binding itself can implement nsIFrameLoaderOwner.  Then the docshell would
exist just fine and all that (and we can remove the hack whereby we go through
the box object to get at the docshell; if we have an nsIFrameLoader pointer we
can just get the docshell off of that).  We could probably also remove the hacks
we have in XBL where we force reflow on binding attachment just so XUL iframes
will work.

Finally, I suppose that I could make browser/iframe be a separate class
inheriting from nsXULElement.  This is probably hard.

I don't really see other options... are there any?

Comment 7

14 years ago
We have a number of bindings whose constructors expect or appear to assume to be
in a document: browser, editor, listbox, menulist, progressmeter-undetermined,
radiogroup, radio, scrollbar, toolbar, tabbox, tabs, control, treebody, treecol.
Then all those bindings are _already_ broken for XUL elements that are being
cloned (since those elements get bindings attached immediately).  They need to
be fixed.

Note that I just checked the radio binding, and it deals with not being in a
document just fine. The constructor just does nothing.  I suppose that could
screw up if radiogroup doesn't handle kids being appended properly.

In case it wasn't clear why this bug got filed, right now:

   foo = bar.cloneNode();

will attach the binding immediately, while:

   foo = document.createElement(bar);

will not attach the binding.  This patch fixes this inconsistency.
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

Note to self:  This also breaks the little dropdown widgets in DOM inspector.

Reviewers, I would still like to land this without the classinfo patch, so this
capability will not be enabled quite yet.  Then I'll fix the various bindings
to deal and land the classinfo change.

Comment 10

14 years ago
The way the radiogroup handles it is that it assumes that the radio's binding
will fire when it is added to the document, thus clearing the radiogroup's
cache. So I assume that is already broken for cloned radios.
(In reply to comment #6)
> Alternately, I could make nsIFrameLoaderOwner/nsIFrameLoader scriptable so that
> the binding itself can implement nsIFrameLoaderOwner.  Then the docshell would
> exist just fine and all that (and we can remove the hack whereby we go through
> the box object to get at the docshell; if we have an nsIFrameLoader pointer we
> can just get the docshell off of that).  We could probably also remove the hacks
> we have in XBL where we force reflow on binding attachment just so XUL iframes
> will work.

This sounds reasonable to me.
Ugh.  We can't create a frameloader if we're not in the document (because the
window can't be hooked up).  So the frameloader thing (while desirable in its
own right) won't help much here.

I've filed bug 266590 on fixing up the various XBL bindings.  Once that's fixed,
we can throw the classinfo switch in this bug.

Updated

13 years ago
Blocks: 260838
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

Since you noted that this breaks things, minusing to get off of my list.
Attachment #162655 - Flags: review?(bryner) → review-
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

Brian, please see comment 9.  The XBL changes on their own don't break anything
and are correct, and are blocking other work on GetDocument removal and
removing SetDocument/etc in favor of BindToTree().  I'd really like to get
those changes in.  I can post a patch without the one classinfo chunk if you
prefer, but that'll be the only difference...
Attachment #162655 - Flags: review- → review?(bryner)
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

Ah, ok.  Looks fine then.
Attachment #162655 - Flags: review?(bryner) → review+
Comment on attachment 162655 [details] [diff] [review]
Patch, tested on that testcase

sr=jst
Attachment #162655 - Flags: superreview?(jst) → superreview+
Checked in the XBL part of the patch.  The bug's not fixed, but the rest will
have to wait for now, till bug 266590 is resolved.
Priority: P1 → P2
Target Milestone: mozilla1.8alpha5 → mozilla1.9alpha
If someone wants to help me on bug 266590, I'd appreciate reviews on the patch
that's there.  It's a partial fix to the problem of bogus constructors that's
blocking this bug.

Comment 19

13 years ago
I guess thease bugs can be related with one: bug 250123 and bug 229703.
Given the latest patch in bug 53901, I think we should WONTFIX this?
For XBL1, probably yes...
Assignee: bzbarsky → nobody
Priority: P2 → --
QA Contact: ian → xbl
Summary: [FIX]XBL should apply to nodes not in documents → XBL should apply to nodes not in documents
Target Milestone: mozilla1.9alpha1 → ---

Updated

9 years ago
Blocks: 508339
Duplicate of this bug: 508339
No longer blocks: 508339
And per comment 20 and comment 21...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.