Closed Bug 211128 Opened 17 years ago Closed 20 days ago

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

Categories

(Core :: XBL, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

See discussion in bug 210808
Attached file Testcase (obsolete) —
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.
Attached patch proposed patch (obsolete) — Splinter Review
Attachment #126769 - Flags: superreview?(jst)
Attachment #126769 - Flags: review?(jst)
taking
Assignee: hyatt → bzbarsky
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Summary: Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → [FIX]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Target Milestone: --- → mozilla1.5alpha
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 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.
Attachment #126769 - Attachment is obsolete: true
Attachment #126769 - Flags: superreview?(jst)
Attachment #126769 - Flags: review?(jst)
Yeah, that sounds like a better fix. :-)
Attached file Better testcase... (obsolete) —
Attachment #126768 - Attachment is obsolete: true
Attached file Better yet testcase
Attachment #127139 - Attachment is obsolete: true
Attached patch work-in-progress patch (obsolete) — Splinter Review
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 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.
Attachment #127148 - Flags: superreview?(jst)
Attachment #127148 - Flags: review?(caillon)
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.
Attachment #127148 - Flags: review?(caillon) → review+
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
Attachment #127148 - Flags: superreview?(jst) → superreview+
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.
Summary: [FIX]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → [FIXr]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Target Milestone: mozilla1.5alpha → mozilla1.5beta
checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 212579
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.5beta → mozilla1.6alpha
Seems to me that #3 is the only reasonable long-term solution. :-)
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.
Attached file testcase to not break
Attachment #127148 - Attachment is obsolete: true
This will take some thought to do right.. no time to do this for 1.6a
Target Milestone: mozilla1.6alpha → mozilla1.7alpha
Summary: [FIXr]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Blocks: 60724
Depends on: 53901
Blocks: 203202
Note to self -- BindToTree, when recursing into a form control, should call
SetForm as needed no matter whether we're in a document...
Blocks: 275150
Depends on: 286000
Attached patch PatchSplinter Review
Had to add the xuldoc require because nsXBLPrototypeBinding.h includes headers
in that dir.... :(
Attachment #179955 - Flags: superreview?(jst)
Attachment #179955 - Flags: review?(jst)
Summary: Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → [FIX]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Target Milestone: mozilla1.7alpha → mozilla1.8beta3
Comment on attachment 179955 [details] [diff] [review]
Patch

r+sr=jst
Attachment #179955 - Flags: superreview?(jst)
Attachment #179955 - Flags: superreview+
Attachment #179955 - Flags: review?(jst)
Attachment #179955 - Flags: review+
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.
Attachment #179955 - Flags: approval1.8b2?
Summary: [FIX]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → [FIXr]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Comment on attachment 179955 [details] [diff] [review]
Patch

a=chofmann
Attachment #179955 - Flags: approval1.8b2? → approval1.8b2+
Fixed.
Status: REOPENED → RESOLVED
Closed: 17 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta2
Depends on: 293639
This patch was backed out (see bug 293639).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bzbarsky → general
Status: REOPENED → NEW
Priority: P2 → --
Target Milestone: mozilla1.8beta2 → ---
Summary: [FIXr]Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise) → Anonymous nodes in XBL should have the XBL doc as the base URI (unless specified otherwise)
Assignee: xbl → nobody
QA Contact: ian → xbl
Comment on attachment 179955 [details] [diff] [review]
Patch

On its own, this is no good.
Attachment #179955 - Flags: review+ → review-

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.

Status: NEW → RESOLVED
Closed: 15 years ago20 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.