571099 - Firefox modifies the "color" property for <a> elements in SVG

Status: ASSIGNED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, Mozilla sets the "color" property to be link-colored (blue or purple, depending on visitedness) on <a> elements in SVG.  This overrides the value of 'color' that the <a> element would otherwise inherit from its parent.

Not that this behavior isn't normally visible, since SVG doesn't do anything with the 'color' property most of the time.  But it *is* visible if you set fill="currentColor" on the <a> or something inside of it.

This behavior makes us give incorrect results on the SVG 1.1F2 test animate-elem-78-t.html (linked in URL field).  Look at the bottom-right rect on that test (which jumps right & left) -- when it's in its leftmost position, it's link-colored (blue or purple), whereas everything else in that test is pinkish in that state.  It's expected (by the test) to match everything else -- the inherited value of 'color' is the 'pinkish' color, and the <a> has fill="currentColor" in an apparent attempt to use that inherited color-value, but we end up ignoring the inherited value and use a link-color instead.

(Luckily the PNG reference case only shows the block in its rightmost position, when it's got the correct color. :))

In Opera & Chromium, the rect is pinkish like everything else, so I think we're alone in our behavior here.  I don't think our behavior is particularly useful or sensible, so I'd argue that we should match Opera & Chromium.

Reduced testcase coming up in a minute.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

See this self-describing testcase.

My expected rendering for this testcase would be for all of the lines to be green (except the bottom one, which has no 'fill' value set).

Opera & Chromium have that rendering, while Firefox 3.0.18 up through today's mozilla-central nightly show the first two lines as being purple & blue, respectively. (I haven't tested any earlier Firefox versions.)

Comment 2

[Robert Longson [:longsonr]]

The patch for bug 570354 didn't fix this then?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't think our behavior is particularly useful or sensible

Our behavior is basically the result of the :link and :visited rules in the pref stylesheet.  Are you arguing that those should not apply to SVG links?  Why not?

Comment 4

[Robert Longson [:longsonr]]

No, the patch for bug 570354 has no effect here.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Are you arguing that those should not apply to SVG links? 
> Why not?

Yes.

Firstly, those rules *aren't particularly useful* for SVG links, since:
 (a) Most of the time they aren't going to have any visible effect.  (unlike HTML, where 'color' actually controls the text color).
 (b) No other SVG implementation that I know of applies these rules to SVG, so authors can't rely on them.

Secondly, since we're the only implementation that applies these rules (AFAIK), they sort of create an interop issue, where we're the odd one out.

And thirdly, based on animate-elem-78-t.html, it looks like the SVG spec doesn't think (or doesn't know?) that these rules should be applied.

So, based on the above, yes -- I'd argue that the default :link/:visited rules shouldn't apply to SVG links.

I don't feel strongly on this point, though, and I'm willing to be convinced otherwise. (though in that case, we'd want to ask www-svg about changing animate-elem-78-t.html)

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, I think I buy that.  Do we still want the moz-any-link rules from ua.css applying to svg:a?  What about the :-moz-any-link:active rule from the prefs sheet?

For the rest, PresShell::CreatePreferenceStyleSheet will need to add an @namespace rule, the indices for the other rules it inserts will need to be adjusted, and well'll want to change the *|*:link selector to *|*:not(svg|*):link (and similar for :visited) in PresShell::SetPrefLinkRules, right?

Comment 7

[Robert Longson [:longsonr]]

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-styling-css-06-b.html indicates that to get visited and colouring it seems like the author is expected to use the an explicit :visited rule.

Comment 8

[Robert Longson [:longsonr]]

Attachment: Bug 571099 - Stop modifying the unvisited, visited and active colours of SVG a elements

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Please be careful of performance numbers with a patch like that. (I'm not sure the servo-based style system has the same performance characteristics, but in the old style system those would have been the most perf-sensitive selectors in the UA sheet.)

Also, does this change match the relevant specs? If not, are there issues on file to update them?

Comment 10

[Robert Longson [:longsonr]]

The SVG specification has never said that <a> elements should have this, I guess specs don't normally say what doesn't happen, they say what does.

Also Safari doesn't do this either.

Comment 11

[Robert Longson [:longsonr]]

Are there perf tests I can run on try that would reveal any impact?

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think it's worth checking whether either the HTML or CSS specifications say this should happen.

For tests, at a minimum I'd look at our existing perf-reftests and talos tests, but heycam or emilio might have more specific suggestions.

Comment 13

[Daniel Holbert [:dholbert]]

Also FWIW, it seems Chrome/Chromium's behavior has changed on this in the time since this bug was filed -- though their behavior makes no sense & is probably a bug. I'll go ahead and file one. (They render the top line of my testcase as black, for no clear reason.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (just using rects)

Comment 15

[Daniel Holbert [:dholbert]]

FWIW, I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1103536 on Chrome's somewhat-odd current behavior (rendering visited SVG links with black as the computed color, overriding the color that the element otherwise would've had/inherited).

Comment 16

[Cameron McCormack (:heycam)]

Regarding the performance of these selectors, I think (and Emilio can correct me once he's back from PTO next week if I'm wrong) that we handle :not() efficiently enough now that this shouldn't be a problem.

If it is a problem, then we can work around that pretty easily -- the only elements that can match the link pseudo-classes are HTML a and area, SVG a, and (if you can believe it) any MathML element with an href="" attribute on it. So if you need to you could rewrite these rules as:

html|*:link, math|*:link { color: ... }
...

and probably the change in specificity here wouldn't cause a problem.

For a perf test, you could try something like this, dropped into the perf-reftest-singletons directory (though I wouldn't bother committing this test):

<!DOCTYPE html>
<script src="util.js"></script>
<style id="s"></style>
<script>
window.onload = function() {
  let styles = '.x a {}';
  let root = document.createElement("div");
  for (let i = 0; i < 100000; i++) {
    let link = document.createElement("a");
    link.className = `y${i}`;
    link.href = "x";
    root.append(link);
    styles += `.y${i} {}`;
  }
  s.textContent = styles;
  document.body.appendChild(root);
  flush_style(root);
  root.className = "x";
  perf_start();
  flush_style(root);
  perf_finish();
};
</script>

(Open the document and it'll alert the time spent under the second flush_style call, which should re-do selector matching on all of the a elements.)

It's a little noisy on my machine, but if you record the results of say 10 runs of this with and without your patch, it should tell you whether the time spent evaluating the :not(svg|*) is noticeable.

As for specs, the HTML spec has rules in the suggested UA sheets that match only HTML elements, and there are no rules in the SVG 2 spec that set the color on links, though there are rules that set the cursor.

Comment 17

[Daniel Holbert [:dholbert]]

Worth noting: in Chrome's related bug on this ( https://bugs.chromium.org/p/chromium/issues/detail?id=352912 , which my bug was duped to), there are suggestions from Blink developers that Chrome should align SVG with HTML (i.e. matching Firefox's current behavior).

In particular, this comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=352912#c12
pdr suggests that they align with HTML (which I think would mean visitedness would have an effect). He says this is pretty much resolved at the spec level, but he links to https://github.com/w3c/svgwg/issues/47 which heycam filed & which, by my reading, resolved on saying we should not align with HTML (i.e. visitedness should have no effect).

The spec bug seems most authoritative here & leans in favor of taking this patch. And if we do take this patch, we should weigh in on the Chromium bug to make sure they don't change to match our current (maybe-soon "old") behavior. :)

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:longsonr, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.