587585 - SVG effects on anonymous XBL elements referring to an element in a different document always tries to load that document as an external resource

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Markus Stange [:mstange]]

Attachment: proposed patch

In bug 547787 I'm trying to use a mask on an anonymous XBL element. The binding that defines this element is in tabbrowser.xml and the element that the binding applies to is in browser.xul. The current code in nsReferencedElement::Reset allows two places for the mask to live in: Either it needs to be in tabbrowser.xml as anonymous content inside the binding, or it needs to be in an external resource document.
Both options are bad: Having it inside the binding would mean that lots of identical SVG frames are created for every tab. Having it in an external resource document would mean that the mask will be applied very late and will cause an additional repaint, thus regressing performance (see bug 546262).

Ideally we'd be able to host the mask inside browser.xul. But that doesn't work with the current code in nsReferencedElement::Reset: "documentURL" will point to tabbrowser.xml and "url" to browser.xul, so we'll try to load browser.xul as an external resource document, and that doesn't work.

Is this by design? It seems like a bug to me.

Incidentally this also also means that -moz-element on anonymous XBL elements doesn't work at all due to the way we choose the URL in ImageRenderer::PrepareImage().

I've attached a patch that implements how I think it should work.
Roc/sicking: Could one of you review the patch or should I wait for bz to return?

I'll write tests once we agree on the correct behavior.

Comment 1

[Robert Longson [:longsonr]]

See bug 406564 and bug 406566. Is one of those a duplicate of this?

Comment 2

[Markus Stange [:mstange]]

Both are different from this bug as far as I can tell. Bug 406564 asks whether the loop over anonymousChildren should exclude elements from the bound document which are inserted by <children/>, and bug 406566 wants that loop to additionally include elements inside the binding's <resources> element (and to have SVG frames created for them, I guess). So they're both about referenced elements *inside* the binding, while this bug is about elements *outside* the binding.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My (limited) understanding of XBL2 is that the bindings document is considered "the document" that the bindings belong to, and ID-references in the bindings would not normally be resolved to elements in the bound document.

It seems to me that we could make URIs where the non-fragment part of the URI is the URI of the bound document refer to the bound document (instead of an external resource document with the same URI as the bound document). That would work for you, right?

Comment 4

[Markus Stange [:mstange]]

It would, and I think that's what the patch does.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, I guess so!

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #0)
> Ideally we'd be able to host the mask inside browser.xul.

Not quite. It's theme-specific code, so ideally it would be a resource in the theme without notably degrading perf.

Comment 7

[Markus Stange [:mstange]]

I agree, that would be even better.

We'll still need this patch to make -moz-element on anonymous elements work.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Doesn't this break the "as anonymous content inside the binding" use case?  And don't we have tests covering that?  And don't they fail with this patch?

Specifically, I don't see when EqualExceptRef(url, bindingDocumentURL) would ever test true.

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #8)
> Specifically, I don't see when EqualExceptRef(url, bindingDocumentURL) would
> ever test true.

In the same cases as before, I'd think. With the old code the check was "EqualExceptRef(url, documentURL)", after documentURL had been set to binding->PrototypeBinding()->DocURI(). Now binding->PrototypeBinding()->DocURI() is stored in bindingDocumentURL.

Anyway, I'll create a testcase and double-check if it still works. I haven't run tests yet because I wanted your feedback on the approach first.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> after documentURL had been set to binding->PrototypeBinding()->DocURI().

Oh, hmm.  I clearly need to reread that logic again.  I don't understand why that code was doing what it's doing, though, unless the relevant URIs for elements coming from XBL are converted to absolute form _before_ adoption into the bound document....  I guess that must be happening.  And the urls you're using are not resolved relative to the XBL document, I take it?

The idea of having XBL anon content lift resources from the bound document is really odd, to me.  Would it make more sense to do sync-loading of external resources that are local or something (ugh)?

Comment 11

[Markus Stange [:mstange]]

(In reply to comment #10)
> And the urls you're
> using are not resolved relative to the XBL document, I take it?

I'm not sure which URLs you mean. The mask I'm using has "mask: url(chrome://browser/content/browser.xul#tab-left-curve-mask)" set on it. The -moz-element URL is constructed here: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3748

> The idea of having XBL anon content lift resources from the bound document is
> really odd, to me.

I'd agree if we were talking about a CSS stylesheet that only applies to a binding. But in this case we're talking about browser.css, which is a stylesheet for bound document (browser.xul) that just happens to also style anonymous content inside the tabs.

> Would it make more sense to do sync-loading of external
> resources that are local or something (ugh)?

"Ugh", right. Or start the loads to them at style resolution and have them block the document load event, just like images. (Or if that's not how they work, do whatever images do.)

But that still wouldn't address the -moz-element issue. The use case there is that I want to this: document.mozSetImageElement("active-personas-image", img) on the browser.xul document and then have anonymous content inside the tabs use it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'm not sure which URLs you mean.

Whatever urls are going into this function.  Sounds like you're using an absolute URI, so the "resolved relative to ..." part doesn't apply at all.

> But in this case we're talking about browser.css

Right.  Fundamentally, the problem is that there's conflation of URIs and id-refs here...  Ah, well.

> Or start the loads to them at style resolution

I wonder what the impact of this would be... Might be worth doing.

> and have them block the document load event

I believe external resources already do this, if their loads start before the load event fires.

> But that still wouldn't address the -moz-element issue.

Yes.  It's not clear to me whether that should work... It seems to be seriously violating the compartmentalization that XBL is supposed to provide.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
> > Would it make more sense to do sync-loading of external
> > resources that are local or something (ugh)?
> 
> "Ugh", right. Or start the loads to them at style resolution and have them
> block the document load event, just like images. (Or if that's not how they
> work, do whatever images do.)

See bug 546262 comment 6.

> But that still wouldn't address the -moz-element issue.

Maybe this bug's summary should be changed. As mentioned and agreed upon earlier, we don't really want theme specific SVG masks in browser.xul.

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #12)
> > Or start the loads to them at style resolution
> 
> I wonder what the impact of this would be... Might be worth doing.

Would you mind writing a patch that does this (in bug 546262)? I'm afraid my time for doing these things in the near future is running out fast.

> > and have them block the document load event
> 
> I believe external resources already do this, if their loads start before the
> load event fires.

I see.

> > But that still wouldn't address the -moz-element issue.
> 
> Yes.  It's not clear to me whether that should work... It seems to be seriously
> violating the compartmentalization that XBL is supposed to provide.

I don't really have an opinion on this. All I need is some way of doing what I want appearance-wise. In this case it means I have to place a tab-shaped cut-out of the active personas image behind the active tab, and the current implementation approach I have in mind requires masks and -moz-element on anonymous content. I'm happy to change approaches as long as it works.

However, I don't understand what we would gain by disallowing the usages I'm proposing here. We don't break anything, we only make cases work that didn't work before, don't we?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> We don't break anything

We sort of do.  We make reusable widgets (XBL) suddenly depend on the place they're included.

The ideal thing would be if we kept track of where a style is coming from and based where we look for the element on that.  So a browser.css style applied to an anon node of a binding attached to an element in browser.xul would look in browser.xul.  But style coming from inside the binding would look inside the binding.  That's way outside the scope of this bug, though...  Might be good to file a followup on being able to do this.  It'd involve storing more data in the style structs (e.g. a CSSValue::URI instead of nsIURI), but should be totally doable.

Comment 16

[Markus Stange [:mstange]]

(In reply to comment #15)
> That's way outside the scope of this bug, though... 

OK, what should we do in this bug, then?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, a good start is addressing the issues Dao had, right?

Comment 18

[Markus Stange [:mstange]]

You mean fixing bug 546262? And how should we fix -moz-element?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I guess we should take the original patch in this bug for now, given the schedule situation, to fix -moz-element, and file a followup to do it right...

Comment 20

[Markus Stange [:mstange]]

Okay, I'll start running and writing tests now.

Comment 21

[Markus Stange [:mstange]]

Attachment: same patch but with tests

Comment 22

[Johnny Stenback (:jst)]

I'm having a hard time coming up with good arguments for why this would block 2.0, marking blocking-, but if you disagree please renominate with reasoning why this has to be fixed for 2.0.

Comment 23

[Markus Stange [:mstange]]

Bug 547787 is blocking, and the patch over there doesn't work without the patch in this bug.

Boris, when do you think you'll get to the review?

Comment 24

[Johnny Stenback (:jst)]

beta8+ per comment 23.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 474794 [details] [diff] [review]
same patch but with tests

r=me

Comment 26

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/889a05b6dc87

Comment 27

[Steffen Wilberg]

"The most common grafxbot failure with 4.0b8/4.0b9pre is that in the test
== mask-html-xbl-bound-01.html mask-html-01-ref.svg
the test file is rendered as a blank page. This happens on all OS's."

See bug 622717.