Closed Bug 387466 Opened 13 years ago Closed 12 years ago

Bug 309020 has regressed

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: longsonr)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

The testcase in bug 309020 doesn't work (after being saved and loaded locally).  Neither does the testcase at http://dev.23inch.de/svg/marker/example.xml, for similar reasons.

The basic problem is that the URI passed to nsContentUtils::GetReferencedElement is the URI of the bound document, with the ref tossed on.  We discover that there's XBL involved and set the document URL to the URI of the binding document.  Then the URIs don't match, so we bail out.

I'm assuming this code did work at some point, but I'm not sure what change to make it not work -- the builds in my nightly archive that go back that far are built with --disable-svg.
Flags: blocking1.9?
Note that both the testcases in bug 309020 and the ones in the m.d.t.svg thread with the subject "markers & XBL" would make excellent reftests.
(In reply to comment #0)

To restate your comments in concrete terms

In http://dev.23inch.de/svg/marker/example.xml

The URI is http://dev.23inch.de/svg/marker/example.xml#Triangle

The document URL is calculated to be:
http://dev.23inch.de/svg/marker/example.xml

But for XBL we set it to the URI of the binding document which we determine is:
http://dev.23inch.de/svg/marker/example.xbl

So...

Is the URI incorrect? It comes like that out of the style system c.f. nsSVGPathGeometryFrame::UpdateMarkerProperty

Note that if we don't change the binding document i.e. remove the documentURL = do_QueryInterface(binding->PrototypeBinding()->DocURI()); and just set the isXBL flag then the marker is displayed.

> Is the URI incorrect?

I have no idea.  Depends on how this stuff is designed to work.  That's why I cced the folks who wrote/reviewed it.

> It comes like that out of the style system

Which just uses whatever base URI the caller passes to it when resolving URIs during parsing.  With XBL1, this will generally be the URI of the bound document unless someone jumps through hoops.
Attached patch possible patch (obsolete) — Splinter Review
This patch also fixes bug 399117. I don't know why this change is necessary though.
Attachment #284485 - Flags: review?(roc)
I'm foncused. So in this test:

  if (!EqualExceptRef(url, documentURL)) {

What are the values of url and documentURL, currently?
Currently:

* url is the URI of the ownerDocument of the node that the binding is attached to
* documentURL is the URI of the binding document
So in this case, both values should be http://dev.23inch.de/svg/marker/example.xbl, I think. I don't understand why they're not equal.
When GetReferencedElement is called, I see:

(gdb) p aURI->mSpec.mData
$5 = 0x8912ae8 "http://dev.23inch.de/svg/marker/example.xml#Triangle"
(gdb) p isXBL
$6 = 1
(gdb) p documentURL.mRawPtr->mSpec.mData
$7 = 0x8a8b4c0 "http://dev.23inch.de/svg/marker/example.xbl"

aURI is  GetStyleSVG()->mMarkerEnd.  It looks like the style comes from a "marker-end" atribute.

So presumably the question is when that attribute is actually turned into a content style rule .  And I further presume that this happens after the cloning that we use to install XBL anon content, or that said cloning doesn't preserve the style rule or something.  So when we parse it, we end up using the bound document as the source of the base URI.
nsSVGElemen::UpdateContentStyleRule is what does the parsing.  It's been lazy all along as far as I can tell...  At least since 2004-03-09.  So I'm still not sure how this used to work.
Perhaps for XBL anon content UpdateContentStyleRule should use the binding document for the base URI?  That will fail with xml:base, but that seems like a rarer case...

Alternately, we could demand that binding content set xml:base to the binding document if they want to refer to it.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
OS: Linux → All
Hardware: PC → All
Attached patch possible comment 10 patch (obsolete) — Splinter Review
An attempt to implement bz's first suggestion in comment 10.
Attachment #289938 - Flags: review?(roc)
Attached patch possible comment 10 patch (obsolete) — Splinter Review
This time with all the changed files.
Attachment #289938 - Attachment is obsolete: true
Attachment #289939 - Flags: review?(roc)
Attachment #289938 - Flags: review?(roc)
Assignee: nobody → longsonr
Attachment #284485 - Attachment is obsolete: true
Attachment #284485 - Flags: review?(roc)
Attachment #289939 - Flags: approval1.9?
Comment on attachment 289939 [details] [diff] [review]
possible comment 10 patch

This bug is already blocking1.9+. No need for approval.
Attachment #289939 - Flags: approval1.9?
> This bug is already blocking1.9+. No need for approval.

So it is. Oops.

Checked in.

Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 399117
Can we please check in some tests so this won't regress again?
Flags: in-testsuite?
Attached patch reftestSplinter Review
Will this do you? I couldn't get the xul to match a single SVG file.
Attachment #290232 - Flags: review?(bzbarsky)
All I did was create two versions of the xul testcase, one with a marker and one without and check that they are different. If they are then presumably that is because the marker is affecting the rendering.
It should be pretty easy to write one testcase with XBL and one without, no?  Just take any node that has a binding attached, replace that with an element that has the namespace/localname that's in extends= there, then replace its kids with the <content> of the binding, etc.  That is, do by hand the flattened tree that XBL ends up constructing in the testcase.

I think that's what the test for this should probably look like.  That's what http://dev.23inch.de/svg/marker/example.xml and http://dev.23inch.de/svg/marker/example2.xml are, no?  I thought I cited that testcase when filing the bug...

Better yet would be to avoid using extends="" altogether, since we plan to remove support for it.  Better to just have some SVG, some of which has XBL bound to it and markers inserted in the anonymous content.  Then have the same SVG with non-anonymous markers and the two should look the same.
Is there such a thing as "static" reflow? *g*

Just a thought. One of the reasons I created a separate 'svg' directory for SVG tests was so we could have a collection of tests to share with other SVG implementers (to improve interoperability). With that in mind it's probably useful to create a sub-subdirectory (or directories) for SVG that contains things other implementers may not be interested in (e.g. XUL or XBL). They'll be more likely to look at our tests if they don't have to wade through failing tests that only fail because they contain some XUL or whatever.

Perhaps we should create a subdirectory in 'svg' called 'moz-only' for this sort of thing? We can tidy its content into better sections later (or even just create it later - don't feel you have to do it now) but it would be good to start separating "other" tests.
> Is there such a thing as "static" reflow? *g*

Not sure what you're asking here...
Sorry, that was a little joke about the file name "dynamic-reflow-01.svg". Ignore me on that one.
For what it's worth, there is initial reflow and incremental reflow...  ;)
Comment on attachment 290232 [details] [diff] [review]
reftest

Per comments
Attachment #290232 - Flags: review?(bzbarsky) → review-
(In reply to comment #18)
> It should be pretty easy to write one testcase with XBL and one without, no? 
> Just take any node that has a binding attached, replace that with an element
> that has the namespace/localname that's in extends= there, then replace its
> kids with the <content> of the binding, etc.  That is, do by hand the flattened
> tree that XBL ends up constructing in the testcase.
> 
> I think that's what the test for this should probably look like.  That's what
> http://dev.23inch.de/svg/marker/example.xml and
> http://dev.23inch.de/svg/marker/example2.xml are, no?  I thought I cited that
> testcase when filing the bug...
> 

No matter what I tried, I just couldn't make plain SVG and XBL generated SVG reftests match each other. They look the same to me, but they won't match.

Anyway jwatt added some reftests which cover binding. These fail without the patch in this bug and pass with it so I'm hoping that's enough to mark this bug as in-testsuite+

reftests/svg/moz-only/xbl-basic-01.svg
reftests/svg/moz-only/xbl-basic-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-binding-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-bound-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-bound-02.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-01.svg
reftests/svg/moz-only/xbl-grad-ref--grad-in-resources-02.svg
Blocks: 389670
Yeah, those cover this bug.  Thanks!
Flags: in-testsuite? → in-testsuite+
Has this bug been fixed for firefox 3.0 beta 2? I'm getting negative results on it with the test case in Bug 309020.
Although the final patch fixed http://dev.23inch.de/svg/marker/example.xml it did not fix the testcase in bug 309020.

The testcase in bug 309020 still results in a documentURL of file:///xxx/test.xml and a URL of file:///xxx/test.xhtml#linearGradient2228 

Although we replace the baseURI in UpdateContentStyleRule with file:///xxx/test.xml that has no effect as test.xml has no properties unlike example.xbl in which marker-end is a property of a marker element.

Changing test.xml to have fill="url(#linearGradient2228)" rather than style="fill:url(#linearGradient2228)" makes things partially work as fill is a property but linearGradient2222 is still broken and I don't see a workaround for that.

FWIW The original possible patch would have fixed both examples.
Perhaps we need to reopen this bug then, depending on what our desired behavior is...
Fwiw, I'd assumed that the patch fixed both testcases the bug was filed for...  I guess I shouldn't assume things like that.  :(
It did to begin with ;-)
Well, it sounds like inline style parsing needs a similar change, perhaps.  That's not even SVG-specific, so much; just a general problem with inline style with XBL.

Should I reopen this bug or file a new one on the still-unfixed problem?
I guess reopening is warranted since the patch checked in so far is only a 50% solution to the issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: longsonr → nobody
Status: REOPENED → NEW
Flags: in-testsuite+ → in-testsuite?
Attached patch fix for both examples (obsolete) — Splinter Review
This fixes both testcases. 

overriding GetBaseURI makes things work for non-attribute styles and also xlink:href parsing which is in nsSVGGradientFrame and other places.

Also all the styles which depend on baseURI need updating when the document is bound.
Assignee: nobody → longsonr
Status: NEW → ASSIGNED
Attachment #294224 - Flags: superreview?(roc)
Attachment #294224 - Flags: review?(roc)
The question I have is should I have made the BindToTree change in nsSVGElement rather than nsStyledElement. I guess there could be performance implications for non SVG in what I've done in this patch.
That code change will affect insertion of HTML nodes with inline style... ideally the inline style would know which base URI was used to parse it and we'd only reparse if that's changed.  Until we do that, better to limit the effect to SVG, I think.
Attached patch SVG only change (obsolete) — Splinter Review
Attachment #294224 - Attachment is obsolete: true
Attachment #294453 - Flags: superreview?(roc)
Attachment #294453 - Flags: review?(roc)
Attachment #294224 - Flags: superreview?(roc)
Attachment #294224 - Flags: review?(roc)
Attachment #294453 - Attachment is obsolete: true
Attachment #294478 - Flags: superreview?(roc)
Attachment #294478 - Flags: review?(roc)
Attachment #294453 - Flags: superreview?(roc)
Attachment #294453 - Flags: review?(roc)
Comment on attachment 294478 [details] [diff] [review]
check oldVal is not null since it is dereferenced

Boris is really the better reviewer for this
Attachment #294478 - Flags: superreview?(roc)
Attachment #294478 - Flags: superreview?(bzbarsky)
Attachment #294478 - Flags: review?(roc)
Attachment #294478 - Flags: review?(bzbarsky)
Comment on attachment 294478 [details] [diff] [review]
check oldVal is not null since it is dereferenced

>Index: nsSVGElement.cpp
>+  nsIDocument* doc = GetOwnerDoc();
>+  if (doc) {
>+    nsIContent* bindingParent = GetBindingParent();
>+    if (bindingParent) {
>+      nsXBLBinding* binding = doc->BindingManager()->GetBinding(bindingParent);

You really want to use the ownerDocument of the binding parent here, I would think.  With XBL2, that will be different from the ownerDocument of the node.  I know that's not what the existing code did, but it's the right thing to do.

>+  nsIURI *base = baseURI;
>+  NS_IF_ADDREF(base);
>+  return base;

How about:

  return baseURI.forget();

to not do the extra refcount?

Witht those changes, r+sr=bzbarsky
Attachment #294478 - Flags: superreview?(bzbarsky)
Attachment #294478 - Flags: superreview+
Attachment #294478 - Flags: review?(bzbarsky)
Attachment #294478 - Flags: review+
Fairly low risk. The bindToTree code is taken from the base case with slightly changed logic. The rest of the code is simply moved so it affects non style attributes.
Attachment #289939 - Attachment is obsolete: true
Attachment #294478 - Attachment is obsolete: true
Attachment #294974 - Flags: approval1.9?
Comment on attachment 294974 [details] [diff] [review]
address review comments

This is a blocker. No approval needed. :)
Attachment #294974 - Flags: approval1.9?
Comment on attachment 294974 [details] [diff] [review]
address review comments

checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.