Closed Bug 1138065 Opened 7 years ago Closed 7 years ago

view elements as fragment identifiers should have normal target matching


(Core :: SVG, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: longsonr, Assigned: longsonr)



(1 file, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
See and in particular which should display with a coloured outline.
Attachment #8570924 - Flags: review?(dholbert)
Assignee: nobody → longsonr
Comment on attachment 8570924 [details] [diff] [review]

>diff --git a/dom/svg/SVGFragmentIdentifier.cpp b/dom/svg/SVGFragmentIdentifier.cpp
>--- a/dom/svg/SVGFragmentIdentifier.cpp
>+++ b/dom/svg/SVGFragmentIdentifier.cpp
>@@ -254,17 +254,18 @@ SVGFragmentIdentifier::ProcessFragmentId
>   if (viewElement) {
>     if (!rootElement->mCurrentViewID) {
>       rootElement->mCurrentViewID = new nsString();
>     }
>     *rootElement->mCurrentViewID = aAnchorName;
>     rootElement->mUseCurrentView = true;
>     rootElement->InvalidateTransformNotifyFrame();
>-    return true;
>+    // just an ordinary anchor rather than an SVG fragment identifier
>+    return false;
>   }

Two things:

(1) I don't fully understand what this comment is saying or why it merits "return false". Could you clarify?  It looks like this is for the #myView case, and the SVG spec says this *is* a fragment identifier:
  # An SVG fragment identifier can come in two forms:
  #  Shorthand bare name form of addressing (e.g., MyDrawing.svg#MyView

(2) The ProcessFragmentIdentifier() documentation needs a tweak. It currently says the following, in SVGFragmentIdentifier.h :
   @return true if we found something we recognised

I don't think that's true anymore (or at least, it's not specific enough -- needs some more nuance now that we can recognize something & return false). There's only one remaining "return true" case, and I think it might be for the scenario where we find "#svgView(...), though I'm not 100% sure.

(This is probably r+ with that fixed; but I'm marking it as r- for now, because I'm not sure I understand it without those comment-clarifications. I'd like to see & grok the new comments before officially r+'ing.)
Attachment #8570924 - Flags: review?(dholbert) → review-
Version: unspecified → Trunk
Attachment #8570924 - Attachment is obsolete: true
Attachment #8572875 - Flags: review?(dholbert)
Comment on attachment 8572875 [details] [diff] [review]
expanded comments

Nice, this is much clearer -- thanks.

>diff --git a/dom/svg/SVGFragmentIdentifier.cpp b/dom/svg/SVGFragmentIdentifier.cpp
>-    return true;
>+    // not an svgView style fragment identifier, return false so the caller
>+    // continues processing to match any :target pseudo elements

Clarifying nit -- please add parens and a hyphen, as follows:
  s/svgView style/svgView()-style/

In particular:
 (1) The parens make it clearer that you're talking about the svgView() syntax, and not e.g. a "SVG <view>"
 (2) Without a hyphen, one could be forgiven for getting the word-grouping wrong, and reading this as "svgView [style-fragment-identifier]". Of course this is alternate reading is meaningless, but that's not obvious; it's conceivable that there could be such a thing as a 'style fragment' & a 'style fragment identifier'.

>diff --git a/dom/svg/SVGFragmentIdentifier.h b/dom/svg/SVGFragmentIdentifier.h
>+   * @return true if we found a valid svgView, in which case further
>+   * processing by the caller can stop. We return false otherwise
>+   * as we may have an ordinary anchor which needs to be :target matched.

Similar clarification to above: please reword the first line as:
 @return true if we found a valid svgView()-style fragment identifier [...]
Attachment #8572875 - Flags: review?(dholbert) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.