User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_7; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.205 Safari/534.16 Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0 If an SVG path is hidden via JavaScript, and then the URL is changed by calling window.history.pushState, when the path is unhidden, it will be rendered all black – the fill pattern will have been lost. I suspect this has something to do with the baseURI changing and throwing off the fill's reference to the <pattern> element, but this issue doesn't occur in any other browser. Reproducible: Always

This issue can also be triggered simply by including a <base> element in the page pointing to a different URL than the page itself. This bug against Raphael may be useful: https://github.com/DmitryBaranovskiy/raphael/issues/298

Confirming based on attached testcase. Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110426 Firefox/6.0a1 (NOTE: The bug still reproduces if I perform the testcase's step 2 before step 1 -- i.e. if I do 'pushState, hide, show')

Hmm. The base URI does change here. Justin, is that what should be happening? If I read the HTML5 spec, seems like the base URI should not change due to pushstate...

OK, so there are several pieces of the puzzle here. First of all, the markup the page generates looks like this: <circle fill="url(#x)" style="fill: url(#x)"/> Now the SVG spec says that the "style" attribute takes precedence over the "fill" attribute. The CSS attr draft at http://dev.w3.org/csswg/css-style-attr/ says: Relative URLs in the style data must be resolved relative to the style attribute's element (or to the document if per-element resolution is not defined) when the attribute's value is parsed. Since the style is set before we pushState, that means we resolve style relative to the base URI before the pushState. In the case linked in comment 2, we resolve relative to the base URI specified in <base>. Now the question is what to do with the resulting fill URI. Right now, we compare it to the document's _current_ address (in HTML5 terms). If they match, we use getElementById on the ref. If not, we do a resource document load of the fill URI. In the attached testcase, after the pushState the document's current address has changed, so the URIs don't match and we do the resource load thing. But we only allow XML resources, and the document returned by the resolved fill URI is not XML... and furthermore has no SVG elements in it anyway, so it couldn't possibly work. If pushState doesn't change the document's base URI, this specific case could work if the referenced element code compared to the document's address (instead of the current address). Of course <base> would probably not do the right thing still. However if pushState _is_ supposed to change the base URI, then there's no way to make this work across pushState calls as far as I can tell, unless either dereferencing the pushed URI returns an appropriate document containing the right SVG (which is arguably what it should do if pushState is not being abused) or style attributes are forced to be reparsed after the pushState or something. Note that some other UAs just violate specs all over the place last I checked when dealing with SVG URI reference stuff. I don't think we want to go there. I believe Justin is raising an html5 spec issue to get the base URI across pushState sorted out; once we have that sorted we can revisit this bug.

Here's the post I made to WhatWG. http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-April/031344.html

Thanks for looking at this, guys! I'm curious, though: if the <circle> element is created _before_ pushState is called, wouldn't I expect the resource load to happen immediately, while the reference is still good? The most unexpected behavior here is that simply showing and hiding an element which has already been instantiated and rendered seems to trigger a resource load.

If it's created immediately, then at that point we look for the resource in the current document and find it. Then pushState changes the document's URI. Nothing else changes at that point. Then if you hide and reshow the circle, we try to look up the resource... and at _that_ point we discover that our URI no longer matches the document URI and try to load the resource. What's expected here ... depends on the exact thing show and hide do. In general, SVG resources are looked up at paint time in Gecko.

I have this same problem, and work around it as follows using a continuation via setTimeout as follows: { ... if (isFF) setTimeout(function() { stateChange(this.newURL); }.bind({newURL: newURL}), 10); else stateChange(newURL); } function stateChange(newURL) { history.pushState({}, "Text", newURL); }

> I believe Justin is raising an html5 spec issue to get the base URI across pushState sorted out; > once we have that sorted we can revisit this bug. Although bz said this almost a year ago, the issue is *still* not resolved. I talked to hixie as recently as a month ago, but we've agreed on basically nothing. :( FWIW, Hixie supports changing our behavior so this bug goes away, but in a way which would break more visible features, such as <a href='#foo'>. A simple workaround might be to change document.base back after you call pushState. I dunno if that's racy. :-/

Still have some occasional problems with this; had to increase the timeout. I don't understand why this bug is an issue, in my case, the same fill gradient id is available in both the old document url an the new one as of history.pushState(). According to the post above, if the URL has changed (which it only has past the #), the resource is looked up again - as I said, my gradient fill node is present in both the old and the new document, so I'm fine with the paint using either one! By the way, this also happens with history.replaceState(), of course. Also suspect similar problems with markers (e.g. marker-end) as they are referenced by URL, too. It's too bad that there isn't some other mechanism (other than url reference) to get to specify the gradient in the current DOM rather than as an external web resource.

I understand that there is an issue of correctness here, but as a "boots on the ground" developer I'm a bit frustrated that the end result is that Firefox is broken when using SVG refs and pushState together. I really don't want to tell my users that Chrome is the only browser that works. Erik's workaround isn't viable for me because I'm not directly calling pushState. I'm using Backbone.js, which calls it for me. I can't override pushState because I run afoul of "Illegal operation on WrappedNative prototype object", and I'm not willing to support a patched version of Backbone for the rest of my life. Perhaps it's time to find a pragmatic solution to this problem rather than waiting for the standard to change?

Yeah, I agree this is quite bad. I'm sorry I haven't had a chance to think about it.

I found a way to partially implement Erik's setTimeout workaround, but the best I can do is to make the problem happen about half the time instead of all the time. If you can come up with a better workaround it would be greatly appreciated.

Hi Folks, What I was doing before was some processing that would populate/add to the SVG DOM, (then setTimeout as a workaround, ) then call pushState. Now, I've reversed the order: I'm doing pushState, then populate/add to the SVG DOM (and without any setTimeout). Seems to be working correctly all the time now with that technique, no more black boxes at all, knock wood.. I don't know if that'll work for everyone, though. One other thought might be that if you can't reverse the order of pushState & SVG processing, to delay adding the SVG nodes that have gradient fills until after the pushState; and another thought might be to delete the gradient nodes and re-add them after the pushState. (Of course, I haven't tried either of these alternatives.) Best of luck! Erik

Also, I agree that using xlink to specify local resources is pretty wacky, instead of just using the local element id like most other things do. For what it's worth, I tried to put the gradient fills in separate file, but never did get that to work at all, even in a simple static test case. One try was as an external html file, and another as an external svg file; in all the cases I tried, all I got was coal (black boxes) ;) Has anyone been able to reference a resource in another file as the fill? If not, one might ask what's the point of using the xlink format, if it always has to refer to an in-file resource anyway? ;)

> Has anyone been able to reference a resource in another file as the fill? Works just fine. At least as long as the other file has an SVG MIME type, of course.

> I can't override pushState because I run afoul of "Illegal operation on WrappedNative > prototype object" Hm, so this may be a bug. I tried using Object.defineProperty, as: var oldPushState = history.pushState; Object.defineProperty(history, 'pushState', { get: function() { return function(obj, title, url) { var oldBase = document.base; oldPushState(obj, title, url); document.base = oldBase; }; } }); <button onclick='history.pushState("", "", "abc")'>Click me</button> The first few times I click the button, I get "NS_ERROR_XPC_BAD_OP_ON_WN_PROTO: Illegal operation on WrappedNative prototype object", as you described. But if I keep clicking the button, eventually it works, and continues to work.

> Hm, so this may be a bug. I filed bug 752803.

I don't know why we get this time-dependent failure with the testcase in comment 19, but the bug is that I'm not binding history.pushState to anything. That is, the following works: var oldPushState = history.pushState.bind(history); history.pushState = function(obj, title, url) { oldPushState(obj, title, url); }; However, I'm not sure you need to resort to hackery here. AIUI, the problem you're experiencing is that history.pushState modifies your base URI. But it only does that if you don't have an explicit <base> tag. What if you do <head> <base href="http://path/to/my/page.html"> </head> ?

I'd never have thought of that issue as the reason for my page not working properly in Firefox (see duplicate bug 855028). How broken must a specification be to depend on a static document URL but allow to change it at the same time? Or how broken must an implementation be not to be able to compare same things reliably? I mean, if the reference is document-local, why should that information be degraded by restricting it to a specific URL. Wouldn't a more general reference like "this document - don't look anywhere else - no matter the URL" be the best solution here? (I'm not familiar with Firefox internals, but I'd always prefer a this pointer over copying my current name for future lookup.) My workaround is now to backup the SVG element code in a JavaScript variable after it has been added to the page and restore it whenever it is displayed again. <div id="loading-div"> <svg>...</svg> </div> <script> var loadingSvg = $("#loading-div").innerHTML; // Always reference bug 652991 here to know why you did this! </script> // To show it later: function showLoading() { $("#loading-div").style.display = ""; $("#loading-div").innerHTML = loadingSvg; }

Ahh but the reference isn't document local though. Here's an example using use but stroke and fill should work just the same: http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/struct-use-05-b.html "So this document - don't look anywhere else" is incorrect.

This is the SVG that failed for me: <svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg" width="200" height="200"> <defs> <linearGradient id="grad1"> <stop offset="0%" stop-color="rgb(53, 158, 227)" stop-opacity="0.7"/> <stop offset="100%" stop-color="rgb(53, 158, 227)" stop-opacity="0"/> </linearGradient> </defs> <path d="M100 20 A80 80 0 1 0 180 100" stroke="url(#grad1)" stroke-width="20" fill="transparent"> <animateTransform attributeName="transform" attributeType="XML" type="rotate" from="0 100 100" to="360 100 100" dur="2s" repeatCount="indefinite"/> </path> </svg> What in this document does not reference to the same document? Where is any reference that requires remembering any URL to find it? If any, where is that URL specified right now?

> What in this document does not reference to the same document? url(#grad1) depends on the base URI; in general url() converts to an absolute URI before it does anything else.

I guess there is no way to make an SVG document (like the above) self-containing and reliably integral if it relies on its transient URL, right? Okay, that should be fixed in SVG itself then. Using url() for local references feels like an ugly hack now. Something like stroke="id(grad1)" would be needed.

There are some proposals for that sort of thing, yes. See http://dev.w3.org/csswg/css4-images/#element-notation

Is there any plan to address this issue? Based on the last few comments it seems like the consensus is that this is a flaw in the SVG spec. That's fair enough, but from the perspective of a developer it's frustrating to have to work around this in Firefox when Chrome and Safari handle it without any problem.

Another workaround that has no negative impact to Opera, Safari, and Chrome: After the call to "window.history.pushState(...)", drop and re-add the "fill" attribute. For ex: var fooRect = document.getElementById('fooRect'); fooRect.removeAttribute("fill"); fooRect.setAttribute("fill", "url(#fooGradient)"); --- <svg id="fooSvg" viewBox="0 0 1600 200" preserveAspectRatio="none" xmlns="http://www.w3.org/2000/svg" version="1.1" height="0px" width="100%"> <defs> <linearGradient id="fooGradient" x2="0%" y2="100%"> <stop id="fooGradientstop1" offset="0%" stop-color="rgb(135,206,250)" /> <stop id="fooGradientstop2" offset="100%" stop-color="rgb(70,130,180)" /> </linearGradient> </defs> <rect id="fooRect" width="1600" height="200" fill="url(#fooGradient)"/> </svg>

I think that what we need to do here is to re-resolve any uris in stylesheets when the baseURI changes. Both in inline <style>-sheets and in style="..." attributes. This should happen both when pushState is used, or when a <base> element is inserted/mutated. Or when a node is moved between documents with different baseURIs. Has the CSS spec authors been resisting such a change? What's the argument against? The fact that attributes are parsed and the parsed representation remembered is generally not how the DOM works. I.e. we generally try to make it such that the DOMs current state is what matters, not which other states you transitioned to on your way there. Anything else makes for a very confusing and hard-to-use API as is demonstrated by the comments in this bug.

> re-resolve any uris in stylesheets when the baseURI changes. Is that web-compatible for the background-image case? Not that this affects not only attributes but also <style> elements.

baseURI changes are pretty rare. And I would expect that in most cases the baseURI changes such that references to external resources end up resolving to the same URI. And I would expect that authors expect references to in-document resources (i.e. urls like "#foo") to continue to refer to in-document resources. So yes, I think this is going to be mainly web compatible. Probably not 100% so, as things rarely are. But I would expect more things to get fixed than to break.

> baseURI changes are pretty rare. Except with pushState, right? > And I would expect that in most cases the baseURI changes such that references to > external resources end up resolving to the same URI. I would expect not, actually... We could try to get data on this.

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #34) > > baseURI changes are pretty rare. > > Except with pushState, right? pushState is pretty rare still. > > And I would expect that in most cases the baseURI changes such that references to > > external resources end up resolving to the same URI. > > I would expect not, actually... We could try to get data on this. Sure. Though the longer we wait with this change, the bigger the risk that the current behavior will come to depend on the current behavior. Given that other browsers seems to update references in at least some cases (do we know which?) that gives support to the theory that it won't break the web.

WebKit special-cases "#" last I checked or something. But only in some cases but not others. So it's not that it updates anything; it's just that it doesn't store the absolute URI, so it has to recompute it all the time. But again, only in some cases.

This comment: "pushState is pretty rare still." could look somehow acceptable in February 2014, but in 2016?.. With coming lot of SPAs? That is really annoying bug for every SPA with analytics (charts, diagrams etc.)

Guys, please fix it ugly bug: http://screencast.com/t/xcmQXkjfyJ1 This is really showstopper issue for our customers to use your beautiful browser. Or you convince us and our customers of your browser is non beautiful? We should not use Mozilla FireFox? That's your point?

The point is that we're following the spec here.... What is your exact proposal for "fixing" this? That is, what specific spec violation are you proposing?

All browsers work fine but FireFox. So, these browsers violate the specification?

> That is, what specific spec violation are you proposing? I'd vote for the violation named "consistency". You don't want to be the guy that breaks everything else. Microsoft has done that ten years ago and the web came to prove Microsoft wrong. Does the web have to come to prove Mozilla wrong this time? I hope that doesn't happen.

> I'd vote for the violation named "consistency". The problem is that the other browsers are neither internally consistent nor consistent with each other, in general. If there were actually a clearly documented set of behavior we could try being consistent with it, but there is not: the same url string is handled in wildly different ways in different places in Chrome, for example, depending on some undocumented implementation details... We could try to reverse engineer it, but that's a pretty tall order.. > Microsoft has done that ten years ago The difference is that Microsoft was doing their own non-spec thing. What we have here is a standard everyone agreed on and that other browsers are simply not following after agreeing to it... but also refusing to explain what they _are_ doing and what they would like the standard to say. I mean the outcome _could_ end up the same, because no one apparently cares about what the specs do or don't say, but then this would be the opposite of the Microsoft problem, where Microsoft was what had the majority behavior all by itself and the web chose standards over that.

I continue to think that the solution in comment 33 is the way to go here. Boris, can you give an example of markup that you think work be broken by that behavior?

You mean the solution in comment 31? The basic "markup" that would be broken is if you use a relative background image with a relative URL that doesn't start with '/' and then pushState a string that has a different path from your original path (differing in directory, not just filename). Try this in any browser right now; I expect you will see that the image continues to show up.

Yeah, i mean comment 31. Ok, that scenario is definitely a concern I agree. Could we do the following in the CSS code: Rather than keep just a nsCOMPtr<nsIURI> keep a Variant<nsCOMPtr<nsIURI>, nsString> where the string indicates that it's a self-referencing URI where the string is the fragment. When we first parse the URI we do the comparison with the document URI right away. Then at painting time (or whenever we actually need the resource), rather than compare if the kept URI matches the URI of the document, check if we're keeping a URI or an nsString. Longer term we should definitely figure out which URIs that we should update, and which we should not. The current state where links in <img> and <a> is updated, but links in CSS are not updated is inconsistent and not good (I'm not sure if we currently update URLs *to* external CSS). One thing to keep in mind that if the user press reload, or bookmark the URL, we will load the page from the new URL and resolve all URLs against that new URL. Adding Anne to hopefully get help with figuring this out. Ideally we'd have consistency within browsers, but at the very least we across browsers.

> The current state where links in <img> and <a> is updated <img> is not updated on base URI changes, as far as I know. At least not until you move it in the DOM. > When we first parse the URI we do the comparison with the document URI right away. This sounds like a performance nightmare, honestly.... I really doubt that's what other browsers do.

what part would be a performance problem? Calling parsedURI.Equals(documentURI) for each parsed URI? If that's so slow, we should be able to make it faster for the relevant cases I would imagine. Yup, you seem to be right that we don't update images. I know we iterate through the whole document when the baseURI is changed, but I guess we just update <a> links.

Note that I'm not proposing that we (for now) iterate through all the URIs in the parsed CSS when the base changes. I'm proposing that while we're parsing some CSS, that we at that time compare the parsed URI with the document URI. (And I guess it's EqualsExceptRef that needs to be called, but that shouldn't need to be slower)

> Calling parsedURI.Equals(documentURI) for each parsed URI? Yes, including all the ones that have nothing whatsoever to do with the document in question... Maybe I'm overestimating how many calls we'd end up with in practice...

I'd guess that it's more common for there to be hundreds rather than thousands of CSS URIs in a given page. We should be able to get each comparison down to a vtable call, a QI and a string comparison.

Um... Have you looked at nsStandardURL::Equals?

What I'm saying is that we can improve it for the common case of comparing two http URLs to each other.

I think we should escalate this to the CSS WG, this feature just seems poorly designed. Isn't this also the feature that we poorly implement too, by changing the Fetch mode (from "no-cors" to "cors") if the URL contains a "#"? If this is the feature where we branch on "#" for Fetch, and nobody is really pushing to change that, perhaps this could also be a feature where url(#...) means something different from url(base#...), by canonicalizing to local-ref(id) or some such. Tab and fantasai might have ideas too on other CSS solutions to this problem. (As far as I know the only features we actively notify of base URL changes are links (<a> and <area>, presumably), so :link/visited styling remains up-to-date. We've yet to define that in detail, but we're fairly close now to having that sorted through.)

Would be happy to escalate up to the CSSWG anything that affects their specs, but, I'm not really clear on what exactly I'm supposed to escalate up to the CSSWG. :) Is there a proposal here for changes that are needed, or at least a clear description of what the problem with the specs is? As far as I can guess it's that you want the "computed value" of a URL to not absolutize it, but I'm not actually sure I understood this discussion correctly...

Consider a document, with url https://website.com/a.html, which contains <img src="img.jpg"> <svg> <circle style="fill: url(#x)"/> <circle fill="url(#y)"/> <circle style="fill: url(other.svg#x)"/> <circle fill="url(other.svg#y)"/> </svg> <div style="background: url(img.jpg)">x</div> <style> .myclass { background: url(img.jpg) } </style> <div class=myclass></div> The question is, what should happen in each of the following cases: * pushState("", "", "b.html") is called * pushState("", "", "folder/b.html") is called * a <base href="b.html"> element is inserted into the body of the document * a <base href="folder/b.html"> element is inserted into the body of the document Does the image change? Do any of the circles change their fill? Does any of the <div> background change? (obviously assuming different resources are located at the "new" URLs) The way that most browsers behave, very little is updated when any of the bullets above happen. What's worse in Gecko is that when we repaint, we compare the current URL of the document to the resolved URL of the fill. When pushState runs, that updates the current URL of the document, but doesn't update the resolved URL in the parsed <circle> style attributes. That means that the "#x" URL no longer is considered a same-document reference which is really undesired. There's lots of performance constraints here. And probably plenty of web-compat constraints. So the obvious solution of "update everything whenever the base or document URI changes" probably isn't a viable solution. As a side note, there are clearly CSS interactions here, but there are also SVG, SVG+CSS and plain-HTML problems here.

I think it's clear that what we *want* is for url(#fragment) to always be a "local" ref. I don't believe there is any reasonable case where want that particular syntax to resolve to some non-local URL. (For one, cross-document refs simply don't *work* in several cases/browsers for SVG, and for two, it's stupid.) Fiddling around with the base url used to resolve the fragment into an absolute url is certainly a possible solution, but I think it's clearly hacking around the problem. There's an easy-to-understand semantic being communicated here (local refs), and we're dealing with it in a roundabout way (absolutize the url, then verify that it's the same url as the current page), and that's having unfortunate side-effects. I think we should go in the direction Anne is suggesting - have CSS treat url(#frag) as specifically doing a local ref. Achieving this by computing the url() into some new function that only handles local refs is fine, if we think it's backwards compatible; I'm also okay with just marking the url() as a "local ref" internally and branching serialization that way.

Handling "url(#foo)" as local refs sounds good and should be quite performant. And would resolve this bug as filed. That leaves the background and <img> and other.svg#foo questions in comment 58.

Ah yeah, for refs that actually use some path data, I suspect you should just re-canonicalize the urls. The semantic there is that of a normal link. The author might *presume* that it's a link to the same folder or something, but we can't really tell that. I'll raise the url(#frag) to the CSSWG.

I certainly think that the most logical thing to do is to re-resolve all URLs against the new page URL (or new base URL). But, as mentioned in comment 58, there's likely both performance reasons and webcompat reasons why that's not feasible. Any proposed solution definitely needs to be checked with multiple browser vendors.

Putting that aside for the moment, I've started a thread for the fragment-url handling in CSSWG now. Feel free to comment in support if you'd like.

Here's the thread: https://lists.w3.org/Archives/Public/www-style/2016Mar/0258.html

The thread is here (thanks for raising): https://lists.w3.org/Archives/Public/www-style/2016Mar/thread.html#msg298 Jonas, you're making this problem bigger than it is. <img> is already defined not to update on base URL changes and browsers are interoperable on that. I don't think we should try to fiddle with it. I also don't think we should care about foo#bar type situations to keep things simple and relatively fast. Just encourage folks to use url(#...) and and special case that, and maybe add document(...) or some such if as dedicated syntax down the line (to which the former could canonicalize if deemed compatible enough).

Ok, sounds good. So here's a proposed implementation strategy. It's a slight modification of comment 48 In the parsed style rules, rather than make the parsed objects keep just a nsCOMPtr<nsIURI> keep a Variant<nsCOMPtr<nsIURI>, nsString>. When we're parsing a CSS rule for CSS properties that take advantage of in-document references (such as pattern and fill) we use the following logic: * If the URL starts with '#', parse as normal, but then grab the fragment (nsIURI.GetRef) and store just the resulting string in the Variant described above. * Otherwise parse as a normal URI. When painting, for an nsIURI compare with the document-uri. If they match, grab the fragment (nsIURI.GetRef) and overwrite with an nsString. For a string, assume that the string means internal-reference. Alternatively, when painting, remove the code that compares the parsed URI with the document URI. Instead assume that an nsString means internal-reference and nsIURI means external-reference. This is simpler, but somewhat more likely to break existing content.

Jet, could we find someone to work on this? Seems like the type of quality issue that would be nice to fix. The result of this is that when using SVG-fill and SVG-patterns together with pushState, the fill and pattern disappears in Firefox but not in other browsers. There are workarounds, but they have significant performance impact. Either you can always refer to external SVG files when using SVG-fill and SVG-patterns (which causes additional network loads), or you can remove and re-insert the elements containing the fill/pattern CSS rules, which require non-trivial restyles. The fix isn't trivial, but it shouldn't be too complex either. See comment 66.

Get CJ into the loop and start analyzing the bug before going further.

Sorry, I don't understand the previous comment. Are you saying that you'll get CJ into the loop, or are you asking me to talk to CJ?

(In reply to Jonas Sicking (:sicking) from comment #69) > Sorry, I don't understand the previous comment. Are you saying that you'll > get CJ into the loop, or are you asking me to talk to CJ? I already talked to CJ and he will work on this bug as soon as he completed web compat bug 1261578.

prototyping

By looking into nsStyleStruct.h, except SVG fill, I think we should also considerate: SVG marker nsStyleSVG::mMarkerEnd; nsStyleSVG::mMarkerMid; nsStyleSVG::mMarkerStart; SVG clip path nsStyleClipPath::mURL CSS filter, which may refer to a SVG filter element. nsStyleFilter::mURL CSS background mask, which may refer to a SVG mask element. nsStyleImageLayers::Layers::mSourceURI I think we should apply url re-evaluation before painting on all(or none) of these attributes.

Sounds good. We should do it for any property where we currently do a same-uri-ignore-fragment comparison with the document uri, and grab the element with the given id when the comparison are the same.

Comment on attachment 8746996 [details] MozReview Request: Bug 652991 - Part 1. Bring local-reference flag into nsIURI; It seems a bit weird to do this, not least because it won't round-trip.... So now you get into a situation where newURI(uri.spec) is not equivalent to uri. And even cloning doesn't work, as the patch is written. Is there a reason we're trying to force this into the URI instead of doing the URI-or-local-ref thing that was suggested above? There are also issues with the initial local-ref detection. Pretty sure it would break on url(" #foo") for example. Oh, and we do still need spec clarifications/changes here. Is someone actually working on that?

Comment on attachment 8746997 [details] MozReview Request: Bug 652991 - Part 2. Re-evaluate url before painting; I would have expected changes to nsReferencedElement::Reset somewhere in here... Or are those downstream from this code? In any case, the fact that nsReferencedElement::Reset uses the document URI but this code uses the base URI seems fishy to me.

So one question is... what behavior are we actually after here? Has anyone explicitly written it down? Have we tried creating tests for what we thing is our desired behavior, across various edge cases involving pushState and <base> and interesting syntactic constructs like in comment 81 and then checked what other browsers do on those tests?

(In reply to Boris Zbarsky [:bz] from comment #81) > Comment on attachment 8746996 [details] > MozReview Request: Bug 652991 - Part 1. Bring local-reference flag into > nsIURI; > > It seems a bit weird to do this, not least because it won't round-trip.... > So now you get into a situation where newURI(uri.spec) is not equivalent to > uri. And even cloning doesn't work, as the patch is written. > > Is there a reason we're trying to force this into the URI instead of doing > the URI-or-local-ref thing that was suggested above? I think you are saying using things like Variant<nsCOMPtr<nsIURI>, nsString>. (Joans's proposal in comment 66). The benefit of putting local-reference into nsIURI is less code change, nothing else. > There are also issues with the initial local-ref detection. Pretty sure it > would break on url(" #foo") for example. Thanks. I will trim-left in the next patch. > Oh, and we do still need spec clarifications/changes here. Is someone > actually working on that?

(In reply to Boris Zbarsky [:bz] from comment #82) > Comment on attachment 8746997 [details] > MozReview Request: Bug 652991 - Part 2. Re-evaluate url before painting; > > I would have expected changes to nsReferencedElement::Reset somewhere in > here... Or are those downstream from this code? you are right, put the changes in nsReferencedElement::Reset make more sense. Thanks.

Boris, do you have a proposal for how to actually test this? Part of the problem is that very often we want a "don't change anything" behavior. But how do we tell, especially in other browsers, "nothing is changing because the UA doesn't think anything should be changed", vs. "nothing is changing because we haven't repainted yet"? Sounds like the testcase in comment 0 triggers a display:none/display:block cycle of a parent of the SVG?

Comment on attachment 8746996 [details] MozReview Request: Bug 652991 - Part 1. Bring local-reference flag into nsIURI; I don't think we should change nsIURI. Instead we should change the stylesystem datastructures that hold on to the nsIURI.

> But how do we tell, especially in other browsers, "nothing is changing because the UA doesn't think anything > should be changed", vs. "nothing is changing because we haven't repainted yet"? Seems like in a reftest-like scenario you would just do things that should really involve a repaint (e.g. moving nodes around in the DOM)...

Comment on attachment 8746997 [details] MozReview Request: Bug 652991 - Part 2. Re-evaluate url before painting; I don't know this part of the code well enough to look at this.

Ok, so what I think we should do then is write tests to cover something like the following 1) Load page A.html 2) Use DOM functions to create an SVG pattern and give it id 'x' 3) Optionally set base-uri using <base> to B.html 4) Optionally use pushState to change the page URL to C.html 5) Create a SVG <circle style="fill: url(#x)"> and insert into page 6) Create a SVG <circle style="fill: url(A.html#x)"> and insert into page 7) Wait for page to paint (probably best done using requestAnimationFrame) 8) Optionally set base-uri using <base> to D.html 9) Optionally use pushState to change the page URL to E.html 10) Optionally set "display:none" and then "display:block" on a <div> element which is parent the two circles. 11) Optionally remove the <circle> elements from the document and insert them into the page again 12) Check what is rendered

But definitely don't write 64 separate tests. If we want to test all combinations we should do so in an automated fashion. Alternatively we can pick just the most meaningful things to test and write manual tests for those.

Any updates here?

I am busy on another topic, will come back to this bug one week later.

I just changed the spec to specify how fragment-only URLs are specially-handled in CSS, per the resolution we obtained at the f2f meeting a few weeks ago: <https://drafts.csswg.org/css-values/#local-urls>

Tab, do you know how closely that matches existing browsers? Presumably it does not match current Firefox behavior, which is a good thing.

It doesn't match anyone exactly. Chrome/WebKit have bizarre behavior, but in at least some cases their current behavior has the same effect as what's specified; in particular, the test case linked early in the thread works "as expected" in Chrome (where the pushState has no effect on the rendering).

Hi dholbert, To fix this bug, a new section in spec created[1]. Let's call it fragment URL. I worked on fixing this old bug, everything works fine until I met use-02-extref.svg reftest failure. Here is a simple version of this reftest A.svg <svg> <!-- draw in blue --> <radialGradient id="radialGrad1" ...> <stop offset="000%" stop-color="blue" /> <stop offset="100%" stop-color="blue" /> </radialGradient> <use xlink:href="B.svg#rect4"> </svg> B.svg <svg> <!-- draw in red --> <radialGradient id="radialGrad1" ...> <stop offset="000%" stop-color="red" /> <stop offset="100%" stop-color="red" /> </radialGradient> <rect id="rect4" x="200" y="125" width="200" height="125" stroke="none" fill="url(#radialGrad1)" /> </svg> In short, A.svg use a rect in B.svg. That rect has a local reference URL attribute. According to SVG spec[2], the <use> in "A.svg" will clone <rect> in document B.svg into document A.svg. So we have an anonymous <rect> element in document A.svg(which is cloned from B.svg), and this cloned <rect> has a fill attribute, which value is "#radicalGrad1". "#radicalGrad1" is local reference URL[1], we resolve it by using the current document, which is A.svg. So the final url is http://..../A.svg#radialGrad1.(Before we introduce local-ref url, the final url is http://..../B.svg#radialGrad1) In the end, a blue rectangle is drawn on the screen and test case failed (Reftest wants a red rectangle). According to [1], I think I probably should modify use-02-extref.svg to fit [1], but I would like to hear your opinion first. PS: [3] is the link of try. Except use-02-extref.svg, I got two more test failure. I think the reason is the same, or similiar. [1] https://drafts.csswg.org/css-values/#local-urls [2] https://svgwg.org/svg2-draft/struct.html#UseElement [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6627ad5398b3&selectedJob=22842036

PS: chrome and edge both draw red rectangle(because they do not support local-reference URL?).

Per IRC discussion just now, I think we need to avoid breaking the scenario that's being exercised in use-02-extref.svg (A.svg cloning a chunk of B.svg which has a local reference that points to something else in B.svg). If the spec doesn't cover this scenario, it probably should.

Without SVG use element and moz-binding attribute, it's relatively easy to implement local-ref in gecko. But of cause I need to handle them T.T. FragementOrURL in Part 2. is base on Jonas's proposal. The function to fetch correct document URI is GetOriginalDocURI of Part 4.

Comment on attachment 8763284 [details] Bug 652991 - Part 2. Create FragmentOrURL to hold both local-ref/non-local-ref URL. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59530/diff/3-4/

Comment on attachment 8763283 [details] Bug 652991 - Part 1. Carry local-url-flag in URLValueData. https://reviewboard.mozilla.org/r/59528/#review60512 ::: layout/style/nsCSSParser.cpp:8222 (Diff revision 3) > + nsString trimmedURL = aURL; > + trimmedURL.Trim(" \t\n\r\f"); // trim HTML5 whitespace > + bool isLocalRef = trimmedURL.Length() ? (trimmedURL.First() == '#') : false; The URL Standard says that all leading C0 control characters are also trimmed from the start: https://url.spec.whatwg.org/#concept-basic-url-parser This seems to match what nsStandardURL does when resolving relative URLs, which uses net_FilterURIString (though that function isn't available from nsCSSParser): http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/netwerk/base/nsURLHelper.h#107-116 But it would be good to avoid manipulating the string and instead just search for the first non-C0 non-SP character. Maybe just loop over the characters? (And maybe factor that out into a static inline helper function.) ::: layout/style/nsCSSValue.h:103 (Diff revision 3) > // aString and aBaseURI. > URLValueData(nsStringBuffer* aString, > already_AddRefed<PtrHolder<nsIURI>> aBaseURI, > already_AddRefed<PtrHolder<nsIURI>> aReferrer, > - already_AddRefed<PtrHolder<nsIPrincipal>> aOriginPricinpal); > + already_AddRefed<PtrHolder<nsIPrincipal>> aOriginPricinpal, > + bool aLocalURLFlag = false); Since we don't have that many callers of the URLValueData and URLValue constructors, it probably isn't worth giving this a default value. Better to be explicit at the call sites. ::: layout/style/nsCSSValue.h:136 (Diff revision 3) > + // mLocalURLFlag is set when url()’s value starts with a U+0023 number sign > + // (#) character. Not sure what happened with the formatting there, but probably good to avoid the non-ASCII apostrophe (and the double space before the "url"). ::: layout/style/nsCSSValue.h:138 (Diff revision 3) > PtrHandle<nsIPrincipal> mOriginPrincipal; > private: > mutable bool mURIResolved; > + // mLocalURLFlag is set when url()’s value starts with a U+0023 number sign > + // (#) character. > + bool mLocalURLFlag = false; If the constructors set mLocalURLFlag then we don't need the default value here.

Comment on attachment 8763284 [details] Bug 652991 - Part 2. Create FragmentOrURL to hold both local-ref/non-local-ref URL. https://reviewboard.mozilla.org/r/59530/#review60532 The overall approach of wrapping these values in a FragmentOrURL is fine, but some issues: ::: layout/style/nsStyleStruct.h:3198 (Diff revision 4) > protected: > nscoord mColumnRuleWidth; // [reset] coord > nscoord mTwipsPerPixel; > }; > > +struct FragmentOrURL { Nit: "{" on new line. ::: layout/style/nsStyleStruct.h:3199 (Diff revision 4) > nscoord mColumnRuleWidth; // [reset] coord > nscoord mTwipsPerPixel; > }; > > +struct FragmentOrURL { > + NS_INLINE_DECL_REFCOUNTING(FragmentOrURL); I think we don't need to heap allocate FragmentOrURL objects; instead just use them directly in the style struct members. The mURL and mRef members in the union are refcounted already, and I don't think there's much advantage to sharing FragmentOrURL objects between style structs (which would happen in the style struct copy constructors). Then we don't need NS_INLINE_DECL_REFCOUNTING here. You'll probably need to add a copy constructor, though. ::: layout/style/nsStyleStruct.h:3209 (Diff revision 4) > + already_AddRefed<nsIURI> ResolveByURI(nsIURI* aURI) const; > + already_AddRefed<nsIURI> ResolveByContent(nsIContent* aContent) const; Suggestion: I think it would be fine to name these the same, say ResolveURL. ::: layout/style/nsStyleStruct.h:3212 (Diff revision 4) > + } > + > + already_AddRefed<nsIURI> ResolveByURI(nsIURI* aURI) const; > + already_AddRefed<nsIURI> ResolveByContent(nsIContent* aContent) const; > + > + void GetLocalRef(nsString &aRef) const; Nit: "&" next to type. ::: layout/style/nsStyleStruct.h:3218 (Diff revision 4) > + void SetURL(nsIURI* aURL, bool aLocalRef); > + void SetURL(const nsCSSValue* aValue); Minor point: I think the name of the class "FragmentOrURL" sounds like you have a discriminated union (which it is), but the method named SetURL sounds like it will always set a URL rather than a Fragment, but really it depends on whether aLocalRef is true or false. Maybe call it SetValue? It looks like we only ever call the first SetURL overload from the second one. Maybe just have a single SetURL(const nsCSSValue*) method? ::: layout/style/nsStyleStruct.h:3222 (Diff revision 4) > + union { > + nsCOMPtr<nsIURI> mURL; > + RefPtr<nsStringBuffer> mRef; > + }; Is this union valid? Reading http://en.cppreference.com/w/cpp/language/union and https://en.wikipedia.org/wiki/C%2B%2B11#Unrestricted_unions it sounds like you would need a constructor and destructor on the union. It might be simpler just to use raw pointers and to call AddRef and Release on them explicitly. ::: layout/style/nsStyleStruct.h:3226 (Diff revision 4) > + > + union { > + nsCOMPtr<nsIURI> mURL; > + RefPtr<nsStringBuffer> mRef; > + }; > + bool mIsLocalRef = false; This will be set during the constructor, so no need for a default value here. ::: layout/style/nsStyleStruct.cpp:67 (Diff revision 4) > { > return aURI1 == aURI2 || // handle null==null, and optimize > (aURI1 && aURI2 && aURI1->URIEquals(*aURI2)); > } > > +static bool EqualURIs(FragmentOrURL *aURI1, FragmentOrURL *aURI2) Nit: new line before "EqualURIs", and "*"s next to types. ::: layout/style/nsStyleStruct.cpp:1027 (Diff revision 4) > + return false; > + } > + > + if (mIsLocalRef) { > + nsAutoCString str1, str2; > + if (mRef->StorageSize() != aOther.mRef->StorageSize()) { An nsStringBuffer's storage size might be bigger than the string it contains. So I don't think it's really possible just to use an nsStringBuffer to store the fragment, because you don't know what the string length is. Well, you can look for the \0 character, since you do get the nsStringBuffer from an nsCString that you create in SetURL. Or you could call Data() on the nsStringBuffer to get a pointer to the character data, and wrap that in an nsDependentCString. Then you don't need to search for the \0. (Alternatively you could use an nsCString in the union, though that will mean you have to work out those issues with constructors/destructor on the union.) BTW you can just use an nsCString here rather than nsAutoCString, since you don't need to use the stack-allocated string storage (it will just share the string buffer in the nsCString). The sharing works with nsAutoCString too, but it's not necessary to have that stack storage. ::: layout/style/nsStyleStruct.cpp:1032 (Diff revision 4) > + if (mRef->StorageSize() != aOther.mRef->StorageSize()) { > + return false; > + } > + > + mRef->ToString(mRef->StorageSize() - 1, str1, false); > + aOther.mRef->ToString(mRef->StorageSize() -1, str2, false); Nit: space after the "-". ::: layout/style/nsStyleStruct.cpp:1043 (Diff revision 4) > +} > + > +void > +FragmentOrURL::SetURL(nsIURI* aURL, bool aLocalRef) > +{ > + NS_ASSERTION(aURL, "expected pointer"); Maybe |MOZ_ASSERT(aURL);| to be consistent with the other SetURL method. ::: layout/style/nsStyleStruct.cpp:1077 (Diff revision 4) > +{ > + nsCOMPtr<nsIURI> result; > + > + if (mIsLocalRef) { > + MOZ_ASSERT(aURI); > + MOZ_ASSERT(mRef->StorageSize() > 1); Again, you can't rely on StorageSize() being the length of the string here. ::: layout/style/nsStyleStruct.cpp:1080 (Diff revision 4) > + nsAutoCString ref; > + mRef->ToString(mRef->StorageSize() - 1, ref, false); Whatever approach you use earlier (nsDependentCString wrapping mRef->Data() maybe), use here too. ::: layout/style/nsStyleStruct.cpp:1100 (Diff revision 4) > + nsCOMPtr<nsIURI> url = aContent->GetBaseURI(); > + return ResolveByURI(url); > +} > + > +void > +FragmentOrURL::GetLocalRef(nsString &aRef) const Nit: "&" next to type.

Comment on attachment 8768108 [details] Bug 652991 - Part 3. Add parameter aContent to ExtractComputedValue https://reviewboard.mozilla.org/r/62422/#review60574 ::: layout/style/nsStyleContext.h:413 (Diff revision 2) > - nscolor GetVisitedDependentColor(nsCSSProperty aProperty); > + nscolor GetVisitedDependentColor(nsCSSProperty aProperty, > + nsIContent *aContent = nullptr); Since GetVisitedDependentColor with fill/stroke properties only works if those properties had a color specified (and not a url() value), do we need to pass through the nsIContent* here and in the other functions in this file? It seems like we don't, as long as we adjust the assertion in StyleAnimationValue::ExtractAnimationValue to only assert if aContent is null and we have a url(#fragment) value. If that works, then I think this patch will become a lot smaller.

https://reviewboard.mozilla.org/r/59532/#review60580 ::: layout/style/nsComputedDOMStyle.cpp:5568 (Diff revision 4) > - if (svg->mMarkerEnd) > - val->SetURI(svg->mMarkerEnd); > + if (svg->mMarkerEnd) { > + nsCOMPtr<nsIURI> markerURI =svg->mMarkerEnd->ResolveByContent(mContent); > + val->SetURI(markerURI); > + } The new section in the CSS Values and Units spec says that these url(#fragment) values should be serialized just like that, rather than as the resolved, absolute URL. So we don't want to resolve them here. Instead I think we'll need to add support to nsROCSSPrimitiveValue to store url(#fragment) values, so they can be serialized differently from how the CSS_URI type is serialized: http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/layout/style/nsROCSSPrimitiveValue.cpp#462-467 ::: layout/style/nsRuleNode.cpp:9333 (Diff revision 4) > NS_STYLE_FILL_RULE_NONZERO); > > // marker-end: url, none, inherit > const nsCSSValue* markerEndValue = aRuleData->ValueForMarkerEnd(); > if (eCSSUnit_URL == markerEndValue->GetUnit()) { > - svg->mMarkerEnd = markerEndValue->GetURLValue(); > + svg->mMarkerEnd = new FragmentOrURL(markerEndValue); This will need to change if we make nsStyleStruct::mMarkerEnd etc. be |FragmentOrURL| rather than |RefPtr<FragmentOrURL>|. ::: layout/svg/nsSVGEffects.h:589 (Diff revision 4) > static nsSVGPaintingProperty * > GetPaintingPropertyForURI(nsIURI *aURI, nsIFrame *aFrame, > URIObserverHashtablePropertyDescriptor aProp); > + > + /** > + * A helper function to resolve masker's URL. "marker" ::: layout/svg/nsSVGEffects.h:592 (Diff revision 4) > + > + /** > + * A helper function to resolve masker's URL. > + */ > + static already_AddRefed<nsIURI> > + GetMakerURI(nsIFrame *aFrame, RefPtr<FragmentOrURL> nsStyleSVG::* aMarker); "GetMarkerURI" ::: layout/svg/nsSVGEffects.cpp:682 (Diff revision 4) > GetOrCreateFilterProperty(aFrame); > > if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame && > static_cast<nsSVGPathGeometryElement*>(aFrame->GetContent())->IsMarkable()) { > // Set marker properties here to avoid reference loops > - const nsStyleSVG *style = aFrame->StyleSVG(); > + nsCOMPtr<nsIURI> makerURL = "markerURL" ::: layout/svg/nsSVGEffects.cpp:686 (Diff revision 4) > // Set marker properties here to avoid reference loops > - const nsStyleSVG *style = aFrame->StyleSVG(); > - GetMarkerProperty(style->mMarkerStart, aFrame, MarkerBeginProperty()); > - GetMarkerProperty(style->mMarkerMid, aFrame, MarkerMiddleProperty()); > - GetMarkerProperty(style->mMarkerEnd, aFrame, MarkerEndProperty()); > + nsCOMPtr<nsIURI> makerURL = > + nsSVGEffects::GetMakerURI(aFrame, &nsStyleSVG::mMarkerStart); > + GetMarkerProperty(makerURL, aFrame, MarkerBeginProperty()); > + makerURL = nsSVGEffects::GetMakerURI(aFrame, &nsStyleSVG::mMarkerMid); > + GetMarkerProperty(makerURL, aFrame, MarkerBeginProperty()); MarkerMiddleProperty() ::: layout/svg/nsSVGEffects.cpp:688 (Diff revision 4) > - GetMarkerProperty(style->mMarkerStart, aFrame, MarkerBeginProperty()); > - GetMarkerProperty(style->mMarkerMid, aFrame, MarkerMiddleProperty()); > - GetMarkerProperty(style->mMarkerEnd, aFrame, MarkerEndProperty()); > + nsSVGEffects::GetMakerURI(aFrame, &nsStyleSVG::mMarkerStart); > + GetMarkerProperty(makerURL, aFrame, MarkerBeginProperty()); > + makerURL = nsSVGEffects::GetMakerURI(aFrame, &nsStyleSVG::mMarkerMid); > + GetMarkerProperty(makerURL, aFrame, MarkerBeginProperty()); > + makerURL = nsSVGEffects::GetMakerURI(aFrame, &nsStyleSVG::mMarkerEnd); > + GetMarkerProperty(makerURL, aFrame, MarkerBeginProperty()); MarkerEndProperty() ::: layout/svg/nsSVGEffects.cpp:863 (Diff revision 4) > } > } > + > +template <class Compare> > +static already_AddRefed<nsIURI> > +GetOriginalDocURI(nsIFrame *aFrame, Compare comp) Nit: "*" next to type. ::: layout/svg/nsSVGEffects.cpp:865 (Diff revision 4) > + for (nsIContent* content = aFrame->GetContent(); content; > + content = content->GetParent()) { > + if (content->HasFlag(NODE_IS_ANONYMOUS_ROOT)) { For elements not in a <use> shadow tree, this will still loop all the way up to the document root. It would be good to avoid this, and I think we can, by using the same approach as nsINode::IsAnonymousContentInSVGUseSubtree. ::: layout/svg/nsSVGEffects.cpp:874 (Diff revision 4) > + // comp return true only if this local url is inherited from the parent > + // SVG use element. If that's the case, we should resolve this local > + // url by the base URI of this SVG use element. I see now why you use heap allocated FragmentOrURL objects now. ::: layout/svg/nsSVGEffects.cpp:877 (Diff revision 4) > + result = comp(parent->GetPrimaryFrame()) > + ? parent->GetBaseURI() > + : aFrame->GetContent()->GetBaseURI(); Unfortunately there is a case where this won't work: when the style was provided outside the <use> but not by inheriting it through the <use> itself. (This kind of styling that peeks inside the shadow tree may or may not be what the spec says should happen, but it works at least in Firefox, Safari and Edge.) <!-- test.svg --> <style> rect { fill: url(#x); } </style> <linearGradient id="r" .../> <use xlink:href="other.svg#r"> <!-- other.svg --> <linearGradient id="r" .../> <rect id="r" width="100" height="100"/> If we do decide support this, then we might have to do something like: record on a StyleRule whether it comes from an SVG resource document, so that in nsRuleNode::ComputeSVGData, we can copy that flag into a new boolean on FragmentOrURL. Then we could use that boolean to determine which document base URI to use. I worry this is confusing behaviour to describe to the author, and that it's not what the specs say to do with <use>, but I understand this is the "current" behaviour of Firefox, since we resolve to absolute URIs currently, without your patches.

Review commit: https://reviewboard.mozilla.org/r/66688/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66688/

Comment on attachment 8763283 [details] Bug 652991 - Part 1. Carry local-url-flag in URLValueData. https://reviewboard.mozilla.org/r/59528/#review63506 r=me with these changes. ::: layout/style/nsCSSValue.h:134 (Diff revision 4) > RefPtr<nsStringBuffer> mString; > PtrHandle<nsIURI> mReferrer; > PtrHandle<nsIPrincipal> mOriginPrincipal; > private: > mutable bool mURIResolved; > + // mLocalURLFlag is set when url starts by a U+0023 number sign(#) character. s/by/with/ ::: layout/style/nsCSSValue.h:140 (Diff revision 4) > + bool mLocalURLFlag; > > URLValueData(const URLValueData& aOther) = delete; > URLValueData& operator=(const URLValueData& aOther) = delete; > + > + bool IsLocalURL(nsStringBuffer* aString); Please either make this static, or (preferably) move it into nsCSSValue.cpp as a static function. ::: layout/style/nsCSSValue.h:640 (Diff revision 4) > + bool GetLocalURLFlag() const > + { > + MOZ_ASSERT(mUnit == eCSSUnit_URL || mUnit == eCSSUnit_Image, > + "not a URL value"); > + return mUnit == eCSSUnit_URL ? > + mValue.mURL->GetLocalURLFlag() : mValue.mImage->GetLocalURLFlag(); > + } > + I think, like the other metadata associated with URLValueData, we should just leave callers to go through GetURLStructValue() cand call GetLocalURLFlag() on that. ::: layout/style/nsCSSValue.cpp:2523 (Diff revision 4) > , mReferrer(Move(aReferrer)) > , mOriginPrincipal(Move(aOriginPrincipal)) > , mURIResolved(true) > { > MOZ_ASSERT(mOriginPrincipal, "Must have an origin principal"); > + mLocalURLFlag = IsLocalURL(aString); This can go in the list of initializers above. ::: layout/style/nsCSSValue.cpp:2538 (Diff revision 4) > , mReferrer(Move(aReferrer)) > , mOriginPrincipal(Move(aOriginPrincipal)) > , mURIResolved(false) > { > MOZ_ASSERT(mOriginPrincipal, "Must have an origin principal"); > + mLocalURLFlag = IsLocalURL(aString); This one too. ::: layout/style/nsCSSValue.cpp:2610 (Diff revision 4) > } > > +bool > +css::URLValueData::IsLocalURL(nsStringBuffer* aString) > +{ > + // Find the first none-"C0 controls + space" character. s/none/non/ ::: layout/style/nsCSSValue.cpp:2611 (Diff revision 4) > + nsString::char_type* current = > + static_cast<nsString::char_type*>(aString->Data()); Feel free to just use char16_t rather than nsString::char_type, like nsCSSValue::GetBufferValue does. ::: layout/style/nsCSSValue.cpp:2614 (Diff revision 4) > +{ > + // Find the first none-"C0 controls + space" character. > + nsString::char_type* current = > + static_cast<nsString::char_type*>(aString->Data()); > + for (; *current != '\0'; current++) { > + if (static_cast<uint8_t>(*current) > 0x20) { This will incorrectly skip over characters like U+0120; just drop the static_cast.

Comment on attachment 8763284 [details] Bug 652991 - Part 2. Create FragmentOrURL to hold both local-ref/non-local-ref URL. https://reviewboard.mozilla.org/r/59530/#review63514 r=me with these things. ::: layout/style/nsStyleStruct.h:3233 (Diff revision 5) > + : mURL(nullptr) , mIsLocalRef(false) > + { *this = aSource; } > + ~FragmentOrURL() { ReleaseRef(); } Nit: an extra space before ":", and drop the space before ",". Though if you make mURL an nsCOMPtr as I suggest below, you can drop the initializations and just rely on the assignment. (And you'll be able to drop the destructor and ReleaseRef method.) ::: layout/style/nsStyleStruct.h:3247 (Diff revision 5) > + already_AddRefed<nsIURI> Resolve(nsIURI* aURI) const; > + already_AddRefed<nsIURI> Resolve(nsIContent* aContent) const; Please add a comment above these explaining what they do. ::: layout/style/nsStyleStruct.h:3254 (Diff revision 5) > + > + void GetLocalRef(nsString& aRef) const; > + bool IsLocalRef() const { return mIsLocalRef; } > + > +private: > + nsIURI* mURL; Make this an nsCOMPtr<nsIURI> so you don't need to manually refcount it. ::: layout/style/nsStyleStruct.cpp:1065 (Diff revision 5) > + bool ret = false; > + mURL->EqualsExceptRef(aURI, &ret); > + return ret; > +} > + > + Nit: drop one of these blank lines.

Comment on attachment 8774076 [details] Bug 652991 - Part 4. Reftest for SVG marker. https://reviewboard.mozilla.org/r/66688/#review63516 Can you add another subtest where the marker-* rules are added from an external style sheet, linked to from the main reftest file?

Comment on attachment 8774077 [details] Bug 652991 - Part 6. Reftest for clip-path. https://reviewboard.mozilla.org/r/66690/#review63518 A subtest using external style sheets would be good here too. ::: layout/reftests/svg/use-localRef-clipPath-01.svg:4 (Diff revision 1) > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> > + <title>Testcase for clipPath linked to local-ref URL</title> > + <defs> > + <clipPath id="clip1"> Nit: tab should be spaces.

Comment on attachment 8774079 [details] Bug 652991 - Part 8. Reftest for filter. https://reviewboard.mozilla.org/r/66694/#review63520 Here too.

Comment on attachment 8774080 [details] Bug 652991 - Part 10. Reftest for paint-server. https://reviewboard.mozilla.org/r/66696/#review63522 An external style sheet subtest here too. :) ::: layout/reftests/svg/use-localRef-fill-01.svg:4 (Diff revision 1) > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> > + <title>Testcase for fill linked to local-ref URL</title> > + <defs> > + <linearGradient id="gradient1"> Nit: de-indent these elements inside the <defs> one level (and in the other two files in this patch).

Comment on attachment 8763288 [details] Bug 652991 - Part 11. Reftest for window.history.pushState. https://reviewboard.mozilla.org/r/59538/#review63528 ::: layout/reftests/bugs/652991-1a.html:3 (Diff revision 5) > +<!DOCTYPE html> > +<html class="reftest-wait"> > + <svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="200" height="200"> (In SVG-in-HTML, you don't need the xmlns="". And version="1.1" has no effect anywhere, so feel free to drop that too.) ::: layout/reftests/bugs/652991-2.html:17 (Diff revision 5) > + <script> > + function doTest() { > + window.history.pushState(null, "", "new-page"); > + > + drawPath.style.display = "none"; > + window.setTimeout(() => { Nit: de-indent this line. ::: layout/reftests/bugs/652991-3-ref.html:10 (Diff revision 5) > + <marker id="markerCircle" markerWidth="8" markerHeight="8" refX="5" refY="5"> > + <circle cx="5" cy="5" r="2" style="stroke: none; fill:#000000;"/> > + </marker> > +</defs> > +<path id="drawPath" d="M10,10 L60,10 L110,10" > + style="stroke: #6666ff; stroke-width: 1px; fill: none; marker-start: url(#markerCircle); marker-mid: url(#markerCircle);; marker-end: url(#markerCircle);"/> And here. ::: layout/reftests/bugs/652991-3.html:10 (Diff revision 5) > + <marker id="markerCircle" markerWidth="8" markerHeight="8" refX="5" refY="5"> > + <circle cx="5" cy="5" r="2" style="stroke: none; fill:#000000;"/> > + </marker> > + </defs> > + <path id="drawPath" d="M10,10 L60,10 L110,10" > + style="stroke: #6666ff; stroke-width: 1px; fill: none; marker-start: url(#markerCircle); marker-mid: url(#markerCircle);; marker-end: url(#markerCircle);"/> Nit: double ";;" just before marker-end.

1. Rebase 2. Remove external css reftest because of bug 1289299. 3. Simplify uri resolve logic in ResolveFragmentOrURL

Comment on attachment 8763285 [details] Bug 652991 - Part 3. Using FragmentOrURL to represent SVG maker url. https://reviewboard.mozilla.org/r/59532/#review65602 ::: layout/style/nsComputedDOMStyle.cpp:5587 (Diff revision 7) > > already_AddRefed<CSSValue> > nsComputedDOMStyle::DoGetMarkerEnd() > { > RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue; > + // Bug 1288812 Mention in the comment what this is about (serialization just using the fragment)? (In the followup bug you might like to factor out these three methods, since they're almost identical.) ::: layout/style/nsStyleStruct.cpp:922 (Diff revision 7) > - if (!EqualURIs(mMarkerEnd, aNewData.mMarkerEnd) || > - !EqualURIs(mMarkerMid, aNewData.mMarkerMid) || > - !EqualURIs(mMarkerStart, aNewData.mMarkerStart)) { > + if (!EqualURIs(&mMarkerEnd, &aNewData.mMarkerEnd) || > + !EqualURIs(&mMarkerMid, &aNewData.mMarkerMid) || > + !EqualURIs(&mMarkerStart, &aNewData.mMarkerStart)) { Since mMarkerEnd etc. can't be null, I think we can just use this: if (mMarkerEnd != aNewData.mMarkerEnd || ... and then remove the EqualURIs function added in the previous patch. ::: layout/svg/nsSVGEffects.cpp:698 (Diff revision 7) > GetOrCreateFilterProperty(aFrame); > > if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame && > static_cast<nsSVGPathGeometryElement*>(aFrame->GetContent())->IsMarkable()) { > // Set marker properties here to avoid reference loops > - const nsStyleSVG *style = aFrame->StyleSVG(); > + nsCOMPtr<nsIURI> makerURL = s/makerURL/markerURL/ ::: layout/svg/nsSVGEffects.cpp:894 (Diff revision 7) > + if (!content->IsInAnonymousSubtree()) { > + // content is not in a shadow tree. Resolve url by baseURI of content > + // directly. > + nsCOMPtr<nsIURI> baseURI = content->GetBaseURI(); > + return aFragmentOrURL->Resolve(baseURI); > + } else { You could make this a bit shorter by combining the two branches here. nsCOMPtr<nsIURI> originURI = ...; nsCOMPtr<nsIURI> baseURI = !content->IsInAnonymousSubtree() || aFragmentOrURL->EqualsExceptRef(originURI) ? originURI : content->OwnerDoc()->GetBaseURI(); Also, do you want to use IsAnonymousContentInSVGUseSubtree() instead of IsInAnonymousSubtree()? ::: layout/svg/nsSVGEffects.cpp:901 (Diff revision 7) > + // directly. > + nsCOMPtr<nsIURI> baseURI = content->GetBaseURI(); > + return aFragmentOrURL->Resolve(baseURI); > + } else { > + // content is in a shadow tree. > + // Depend on where this url comes from, choice either the original "Depending", "choose". ::: layout/svg/nsSVGEffects.cpp:907 (Diff revision 7) > + nsCOMPtr<nsIURI> result = aFragmentOrURL->Resolve(baseURI); > + return result.forget(); Just return aFragmentOrURL->Resolve(baseURI).

Comment on attachment 8768109 [details] Bug 652991 - Part 5. Using FragmentOrURL to represent SVG clippath. https://reviewboard.mozilla.org/r/62424/#review65604 ::: layout/style/StyleAnimationValue.cpp:3958 (Diff revision 6) > + nsString pathString; > + clipPath.GetURL()->GetSourceString(pathString); > RefPtr<nsStringBuffer> uriAsStringBuffer = > - GetURIAsUtf16StringBuffer(clipPath.GetURL()); > + nsCSSValue::BufferFromString(pathString); I was going to comment to say that the string that we pass in to the URLValue constructor should just be "#fragment", if the fragment flag is set. But I think it doesn't matter, since I believe the string we store in the URLValue won't be used or exposed to script anywhere, because here we are producing a specified value to feed into the animation style rule, and script can't access those. ::: layout/svg/nsSVGEffects.h:606 (Diff revision 6) > + > + /** > + * A helper function to resolve clip-path URL. > + */ > + static already_AddRefed<nsIURI> > + GetClipPathURI(nsIFrame *aFrame); Nit: "*" next to type.

Comment on attachment 8774078 [details] Bug 652991 - Part 7. Using FragmentOrURL to represent SVG filter url. https://reviewboard.mozilla.org/r/66692/#review65612 ::: layout/svg/nsSVGEffects.cpp:296 (Diff revision 4) > NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_END > > nsSVGFilterChainObserver::nsSVGFilterChainObserver(const nsTArray<nsStyleFilter>& aFilters, > - nsIContent* aFilteredElement) > + nsIContent* aFilteredElement, > + nsIFrame* aFiltedFrame) s/aFiltedFrame/aFilteredFrame/ ::: layout/svg/nsSVGEffects.cpp:302 (Diff revision 4) > { > for (uint32_t i = 0; i < aFilters.Length(); i++) { > if (aFilters[i].GetType() != NS_STYLE_FILTER_URL) > continue; > > + // aFiltedFrame can be null if this filter is belonged to a s/is belonged/belongs/ ::: layout/svg/nsSVGFilterInstance.cpp:130 (Diff revision 4) > // filter element. > if (!mTargetContent) { > return nullptr; > } > > + // aTargetFrame can be null if this filter is belonged to a s/is belonged/belongs/

Comment on attachment 8763287 [details] Bug 652991 - Part 9. Using FragmentOrURL to represent PanitServer url. https://reviewboard.mozilla.org/r/59536/#review65624 ::: layout/style/StyleAnimationValue.cpp:4170 (Diff revision 8) > + nsIDocument* doc = aStyleContext->PresContext()->Document(); > + nsString pathString; > + paint.mPaint.mPaintServer->GetSourceString(pathString); > RefPtr<nsStringBuffer> uriAsStringBuffer = > - GetURIAsUtf16StringBuffer(paint.mPaint.mPaintServer); > + nsCSSValue::BufferFromString(pathString); > + > NS_ENSURE_TRUE(!!uriAsStringBuffer, false); > - nsIDocument* doc = aStyleContext->PresContext()->Document(); > RefPtr<mozilla::css::URLValue> url = > - new mozilla::css::URLValue(paint.mPaint.mPaintServer, > + new mozilla::css::URLValue(paint.mPaint.mPaintServer->GetSourceURL(), > uriAsStringBuffer, > doc->GetDocumentURI(), > doc->NodePrincipal()); Since we have a few copies of this code, it might make sense to factor it out into a static helper functino that takes a FragmentOrURL and nsIDocument and returns a new css::URLValue. ::: layout/style/nsComputedDOMStyle.cpp:5548 (Diff revision 8) > - > - val->SetURI(paint->mPaint.mPaintServer); > + nsCOMPtr<nsIURI> paintServerURI = > + paint->mPaint.mPaintServer->GetSourceURL(); > + val->SetURI(paintServerURI); Mention the serialization issue here too? ::: layout/style/nsStyleStruct.h:3277 (Diff revision 8) > > struct nsStyleSVGPaint > { > union { > nscolor mColor; > - nsIURI *mPaintServer; > + FragmentOrURL *mPaintServer; Nit: "*" next to type. ::: layout/svg/nsSVGEffects.cpp:619 (Diff revision 8) > if (grandparent && grandparent->GetType() == nsGkAtoms::svgTextFrame) { > frame = grandparent; > } > } > + > + nsCOMPtr<nsIURI> paintServerURL =nsSVGEffects::GetPaintURI(frame, aPaint); Nit: space after "=". ::: layout/svg/nsSVGEffects.cpp:964 (Diff revision 8) > > return ResolveFragmentOrURL(aFrame, url); > } > + > +already_AddRefed<nsIURI> > +nsSVGEffects::GetPaintURI(nsIFrame *aFrame, Nit: "*" next to type.

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2a34e4ee11b Part 1. Carry local-url-flag in URLValueData. r=heycam https://hg.mozilla.org/integration/autoland/rev/14d5d16962ed Part 2. Create FragmentOrURL to hold both local-ref/non-local-ref URL. r=heycam https://hg.mozilla.org/integration/autoland/rev/218ce0f2616a Part 3. Using FragmentOrURL to represent SVG maker url. r=heycam https://hg.mozilla.org/integration/autoland/rev/4a566814d354 Part 4. Reftest for SVG marker. r=heycam https://hg.mozilla.org/integration/autoland/rev/861c8457fa3b Part 5. Using FragmentOrURL to represent SVG clippath. r=heycam https://hg.mozilla.org/integration/autoland/rev/1a189a14779d Part 6. Reftest for clip-path. r=heycam https://hg.mozilla.org/integration/autoland/rev/fa667fb962cf Part 7. Using FragmentOrURL to represent SVG filter url. r=heycam https://hg.mozilla.org/integration/autoland/rev/fe4cc99b15a0 Part 8. Reftest for filter. r=heycam https://hg.mozilla.org/integration/autoland/rev/685de09e5481 Part 9. Using FragmentOrURL to represent PanitServer url. r=heycam https://hg.mozilla.org/integration/autoland/rev/b3eeaeca4fb7 Part 10. Reftest for paint-server. r=heycam https://hg.mozilla.org/integration/autoland/rev/ee4bd9f4aabc Part 11. Reftest for window.history.pushState. r=heycam

https://ameliabr.github.io/svgwg/build/publish/struct.html#UseElement If the referenced element is in an external file, then all URL references in attributes and style properties must be made absolute as described in Generating the absolute URL, before copying the value to the element instances. The shadow tree itself uses the same document base URL as the document that includes it.

Thanks CJ for fixing this!

https://hg.mozilla.org/mozilla-central/rev/a2a34e4ee11b https://hg.mozilla.org/mozilla-central/rev/14d5d16962ed https://hg.mozilla.org/mozilla-central/rev/218ce0f2616a https://hg.mozilla.org/mozilla-central/rev/4a566814d354 https://hg.mozilla.org/mozilla-central/rev/861c8457fa3b https://hg.mozilla.org/mozilla-central/rev/1a189a14779d https://hg.mozilla.org/mozilla-central/rev/fa667fb962cf https://hg.mozilla.org/mozilla-central/rev/fe4cc99b15a0 https://hg.mozilla.org/mozilla-central/rev/685de09e5481 https://hg.mozilla.org/mozilla-central/rev/b3eeaeca4fb7 https://hg.mozilla.org/mozilla-central/rev/ee4bd9f4aabc

https://hg.mozilla.org/integration/mozilla-inbound/rev/609608670ddf6e5fcce1e8bf02cf45e1bff835b3 Bug 652991 - Part 12. Correct pointer/refernce parameter convention. r=me

Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/609608670ddf Part 12. Correct pointer/refernce parameter convention. r=me

Any plans when this fix will be released?

(In reply to youCanCallMeDen from comment #235) > Any plans when this fix will be released? The bug status says it will be released in Firefox 51.

brutal workaround for those who don't want wait that long https://github.com/rexxars/react-hexagon/issues/4#issuecomment-254174681

Has this fix landed? I believe I'm seeing similar behavior to this in Firefox 52. At least based on Comment #2 stating that a <base> tag can trigger the issue. Here's a codepen that renders an SVG in chrome but not firefox: http://codepen.io/anon/pen/oWgMov I have a minimal HTML version of this as well: ======================= <!DOCTYPE html> <head> <title>a</title> <base href="/test"> </head> <body> <svg width="32px" viewBox="0 0 16 16"> <defs> <path d="M8 5a2 2 0 1 1 0-4 2 2 0 0 1 0 4zm0 5a2 2 0 1 1 0-4 2 2 0 0 1 0 4zm0 5a2 2 0 1 1 0-4 2 2 0 0 1 0 4z" id="a"/> </defs> <use xlink:href="#a"/> </svg> </body> </html> ======================= 1. This file renders the icon in Chrome and Safari, but not in Firefox. 2. Removing the <base> tag makes it render fine in Firefox. I have also seen this issue on a page that is a valid HTML5-routed application using a proper base tag, but this minimal repro with just some random base tag seemed to do the trick. Can anyone advise me if this is the same issue or a different bug?

Minimal HTML file with base tag failing to render SVG.

(In reply to prodigysim from comment #239) > Created attachment 8858347 [details] > ff-svg-test.html > > Minimal HTML file with base tag failing to render SVG. Thanks for reporting, I will check.

(In reply to prodigysim from comment #239) > Created attachment 8858347 [details] > ff-svg-test.html > > Minimal HTML file with base tag failing to render SVG. It's SVGUseElement::LookupHref() does not handle local URL well. I file another issue(bug 1357432) to fix it. Thanks for reporting.