211128 - Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion in bug 210808

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This testcase won't really work right until bug 209573 is fixed.  To test, just
open test.xml and click on the images -- they should not change.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: proposed patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

taking

Comment 4

[Hixie (not reading bugmail)]

This won't cause explicit children to inherit the base URI from their anonymous
parent, right?

(Kudos on fixing this, I did not think that it would even appear on your radar
for a long time!)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126769 [details] [diff] [review]
proposed patch

Hmm.. it may.  I was trying to test that with that testcase, but didn't quite
succeed.  I'll need to modify it a bit.

In any case, jst prefers another method of doing this, by making GetBaseURL
aware of XBL instead of sticking xml:base attributes all over.	Will be working
on that once the patch in bug 209573 is in.

Comment 6

[Hixie (not reading bugmail)]

Yeah, that sounds like a better fix. :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Better testcase...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Better yet testcase

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: work-in-progress patch

This basically works; I'm still testing and I need to decide whether those
calls to SetBindingParent in Editor and the frame constructor are bogus...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127148 [details] [diff] [review]
work-in-progress patch

OK, there seem to be no problems here.	Talked to hyatt, and he says the calls
to SetBindingParent in Editor and CSSFrameConstructor are semi-bogus, but were
hacked in by waterson and would take some work to take out....

This patch makes me pass the testcase attached to this bug.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 127148 [details] [diff] [review]
work-in-progress patch

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

>+      // Set the binding parent first, since setting the parent may cause
>+      // relative URIs to change (due to xml:base) and |child| may need to be
>+      // able to tell that it's anonymous content as it recomputes its base
>+      // URI.
> #ifdef MOZ_XUL
>       // Only cut XUL scrollbars off if they're not in a XUL document.  This allows
>       // scrollbars to be styled from XUL (although not from XML or HTML).
>       nsCOMPtr<nsIAtom> tag;
>       content->GetTag(getter_AddRefs(tag));
>       if (tag.get() == nsXULAtoms::scrollbar) {
>         nsCOMPtr<nsIDOMXULDocument> xulDoc(do_QueryInterface(aDocument));
>         if (xulDoc)
>           content->SetBindingParent(aParent);
>         else content->SetBindingParent(content);
>       }
>       else
> #endif
>         content->SetBindingParent(content);
>+      content->SetParent(aParent);
>     


You're not to blame, but how about factoring this out?	Something like the
following would make things look so much cleaner, IMO:


  nsIContent *bindingParent = content;
#ifdef ...
   if (...) {
     ...
     if (xulDoc) {
       bindingParent = aParent;
     }
   }
#endif
   content->SetBindingParent(bindingParent);
   content->SetParent(aParent);


The patch looks great otherwise.  r=caillon.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 127148 [details] [diff] [review]
work-in-progress patch

- In nsGenericElement::GetBaseURL() const:

+  nsCOMPtr<nsIURI> parentBase;
...
+  PRBool gotParentBase = PR_FALSE;
+  if (bindingParent && bindingParent != parentsBindingParent) {
...
+	 NS_NewURI(getter_AddRefs(parentBase), bindingURI);
+	 gotParentBase = PR_TRUE;
...
+  } else if (mParent) {
     mParent->GetBaseURL(getter_AddRefs(parentBase));
+    gotParentBase = PR_TRUE;
+  }
+
+  if (!gotParentBase && doc) {
     doc->GetBaseURL(getter_AddRefs(parentBase));
   }

Do you really need gotParentBase here? Can't you just check if (!parentBase &&
doc) here in stead, or are there meaningful cases where the above URI getters
return null?

sr=jst

Comment 13

[Boris Zbarsky [:bzbarsky]]

Realistically, the only such case is if you have a document object without a
documentURI....

I suppose that's not really a "meaningful case", and would not be hurt by making
that extra GetBaseURL call.  Will make that change before checking in.

Comment 14

[Boris Zbarsky [:bzbarsky]]

checked in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I backed out the nsGenericElement, nsCSSFrameConstructor, nsXBLBinding, and
editor changes due to the regressions they caused.

Specifically, the regressions were caused by reversing the order of SetParent
and SetBindingParent in nsXBLBinding.  The problem is that SetParent gets the
bindingParent of the new parent and sets it on the node to handle dynamic
insertion of nodes into documents.

So if we have a node <foo> which is an anonymous XBL node, and we attach a
binding to it, nodes in that binding first got the bindingParent set to <foo>,
then when SetParent was called the bindingParent was clobbered to be <foo>'s
bindingParent.  This naturally messed up things, including scoped stylesheets
(which was the real reason for most of the regressions).

So there are a few options here:

1)  I could call SetBindingParent _again_ after calling SetParent.  Seems
    redundant, but the simplest and most effective way of dealing.
2)  We could add a flag to SetParent that tells it not to reset the
    bindingParent.
3)  We could formalize the order in which nodes should be inserted into a
    document.  For example, we could demand that the SetDocument() call happen
    _last_ after all the parent hookups and such.  At the moment, it happens
    _first_ in XBL and both first and last in the content sink (yes, we call
    SetDocument twice in the content sink).

I sort of like #3 myself -- it specifies something that everyone should be
implementing and we can check the various node impls to make sure they will work
with it.  This would also allow removing a bunch of code from various places in
content, in my opinion...

jst?  Thoughts?

Pushing this out for now, since this is starting to seem like an alpha-time
change, but I'd like to agree on a solution in the near future so I can start
working on it (if it's #3; that's the one that really needs work).

Comment 16

[Hixie (not reading bugmail)]

Seems to me that #3 is the only reasonable long-term solution. :-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note to self -- event attachment via HTML attributes should start using the
owner document, not mDocument...  Also need to check other attribute setting
stuff and make sure it functions correctly when mDocument is null.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase to not break

Comment 19

[Boris Zbarsky [:bzbarsky]]

This will take some thought to do right.. no time to do this for 1.6a

Comment 20

[Boris Zbarsky [:bzbarsky]]

Note to self -- BindToTree, when recursing into a form control, should call
SetForm as needed no matter whether we're in a document...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Had to add the xuldoc require because nsXBLPrototypeBinding.h includes headers
in that dir.... :(

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 179955 [details] [diff] [review]
Patch

r+sr=jst

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179955 [details] [diff] [review]
Patch

Requesting 1.8b2 approval.  This makes relative URIs in XBL bindings relative
to the binding, not the bound document, which should make bindings a little
more usable.

Comment 24

[chris hofmann]

Comment on attachment 179955 [details] [diff] [review]
Patch

a=chofmann

Comment 25

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 26

[Boris Zbarsky [:bzbarsky]]

This patch was backed out (see bug 293639).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179955 [details] [diff] [review]
Patch

On its own, this is no good.

Comment 28

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.