Closed Bug 265894 Opened 20 years ago Closed 7 years ago

Support SVG CSS selector matching rules for <svg:use>

Categories

(Core :: SVG, defect, P1)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: tor, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(13 files, 13 obsolete files)

SVG + CSS match error
640 bytes, application/xml
Details
(Hack) Prevent phony svg rule match
4.58 KB, patch
Details | Diff | Splinter Review
WIP
155.48 KB, patch
Details | Diff | Splinter Review
Bug 265894 - Part 1. Implement NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 4. Let SVGSymbolElement inherit from SVGViewportElementBase.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 3. Implement nsSVGViewportFrame, and let nsSVGInnerFrame inherit from it.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 5. Implement nsSVGSymbolFrame to layout symbol element.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 6. Create symbol frame in nsCSSFrameConstructor.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 7. Do not convert symbol element into svg element.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 8. Implement -moz-use-shadow-tree-root pseudo class.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 9. Selectors should not cross use-element shadow tree boundary.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 265894 - Part 10. Remove SVGSVGElement::HasPreserveAspectRatio.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Depends on: 237020
So the issue here is that by the time we're resolving the style context we no
longer know that the element is an anonymous child of <use>, and more
importantly don't know what it was cloned off of.

One possible solution is to change <use> to implement CreateFrameFor() and
expose apis on the frame constructor to do that, somehow.

Another possible solution is to make it possible for nsIAnonymousContentCreator
to hand back not just nodes, but style contexts to be used for them.

I sort of like this last solution... David, what do you think?
Probably the real solution is to reverse the style context <-> frame
relationship.  Using anonymous content creation mechanisms really doesn't make
sense here.

What really needs to happen is:
 * To implement svg:use across documents, we need an SVG document manager object
that manages all the SVG documents involved.
 * Each of those documents needs a style set object. (This really requires
splitting nsStyleSet in half, since some parts of it involve rule tree and style
context tree ownership, and we shouldn't have more than one of those per document.)
 * SVG frame construction needs to get the right style context from the right
style set.
Attached image testcase (obsolete) — 
I was about to file a bug on this testcase. I presume fixing this bug will fix
the problem it demonstrates?
the testcase worksforme 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Attached file SVG + CSS match error — 
I just bumped over this problem minutes ago. Here's a TC for the bug. This shows that one can match the <circle /> as if it's a child of the <g>. This is explicitly not allowed by the W3C SVG 1.1 specification:

"CSS2 selectors cannot be applied to the (conceptually) cloned DOM tree because its contents are not part of the formal document structure."

See http://www.w3.org/TR/SVG11/struct.html#UseElement

The TC doesn't fail in Opera 9.
Assignee: general → nobody
QA Contact: ian → general
Shouldn't this first testcase be obsoleted?  It works for me also.  The second test case does exhibit the described problem.
2nd testcase is still valid.
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre
I think I've encountered a similar issue but relating to markers instead. In my example I have 3 files:

#1 arrow.svg - Contains a path
#2 marker.svg - Contains a triangle marker
#3 style.css - Applies the triangle marker in marker.svg to the path in arrow.svg and styles it green.
Example File: http://www.mikehemesath.com/svg_markers/demo2/arrow.svg

Although the external stylesheet rule does successfully place the marker in marker.svg onto the path in arrow.svg, it doesn't apply the green fill style. This test case does work in Opera.
I'm fairly sure this use bug is unrelated to any marker issue.
Comment on attachment 184975 [details]
testcase

First testcase is WFM (and has been since soon after it was posted back in 2005, per comment 4 / comment 6). Obsoleting it.
Attachment #184975 - Attachment is obsolete: true
Note that nsIAnonymousContentCreator can in fact hand back style contexts with the nodes now.
I came across this bug on my own without finding this one first. I just wanted to provide some further examples. I'm not sure what good it will do, being that the issue is 10 years old, but here it is:

http://codepen.io/tinyglowstudio/pen/zvqdjy
If anybody stumbles on this issue in the future, there's a good explanation and workaround by AmeliaBR here: http://stackoverflow.com/a/27872310/681179
Summary: Support SVG CSS cascading rules for <svg:use> → Support SVG CSS selector matching rules for <svg:use>
FYI to anyone working on this (or thinking about starting):

Style matching rules for cloned content in `use` elements have been updated in SVG 2:
https://www.w3.org/TR/SVG2/struct.html#UseStyleInheritance

The new rules are consistent with the shadow DOM style scoping and inheritance used for web components, with the extra requirement that stylesheets from the source document are cloned into the shadow tree's scope.

This actually results in a few edge cases where Firefox already matches the new rules & the other browsers don't (see the example in that section about changes from SVG 1.1 to SVG 2).  But there are still important changes that need to be made to get Firefox to spec:

- Symbol elements should be cloned as symbol, not as svg.  The distinction about whether a symbol should be rendered or not depends on whether it is a direct child of a use element's shadow root.

- Selectors should not cross the shadow DOM boundary.  E.g., use > circle should never match, even if the use element duplicates a circle, because the cloned circle is in the shadow DOM, not the main DOM.  Hopefully that will be easier to implement now that these rules are consistent with the broader web platform concept of Shadow DOM.

- When the cloned content is from an external file, any <style> blocks in that file should be parsed and cloned into the shadow DOM's scope, after adjusting any URLs if required.

PS:  If anything in the spec is unclear or impossible to implement, please raise a bug on the SVG Working Group's GitHub repo: https://github.com/w3c/svgwg/issues
Priority: -- → P3
Assignee: nobody → cku
all duplicate bugs can be put into two categories:
1. we clone a symbol referenced element as an svg element.
2. we can match a rule cross use-element shadow tree's boundary.

I need to fix both of them .
So glad to see someone working on this!

For part 1 (cloned <symbol> becomes <svg>), you might want to take a look at how I recommended handling it for the SVG 2 specs:
https://www.w3.org/TR/SVG2/struct.html#SymbolElement
https://www.w3.org/TR/SVG2/styling.html#UAStyleSheet

A <symbol> in SVG 2 becomes a renderable element that is mostly equivalent to a nested <svg>, except that an !important user agent stylesheet rule hides it unless it is in a direct child of a shadow-DOM root hosted by a <use> element.  That way, you can keep the cloned element's tag name as <symbol> while still describing the different display behaviors in standard CSS.

For part 2 (respecting the shadow-DOM boundary), you'll want to coordinate with the code for web components shadow DOM.  The extra complication for <use> is that if the cloned graphics are from the same document, then the stylesheets from that document get cloned into the shadow tree's scope, too.
https://www.w3.org/TR/SVG2/struct.html#UseStyleInheritance
(In reply to C.J. Ku[:cjku](UTC+8) from comment #37)
> all duplicate bugs can be put into two categories:
> 1. we clone a symbol referenced element as an SVG element.
dbaron also pointed out #1 at:
https://bugzilla.mozilla.org/show_bug.cgi?id=1268431#c7
The current implementation is using SVGSVGElement to replace SVGSymbolElement, so that we can leverage SVGSVGInnerFrame for layout and rendering.

So, unpreventable, we will match rules of SVG tags on those phony svg element.

I think the correct fix is to implement SymbolFrame(base on SVGSVGInnerFrame) and not converting SymbolElement to SVGSVGElement in SVGUseElement::CreateAnonymousContent.
> 2. we can match a rule cross use-element shadow tree's boundary.
> 
> I need to fix both of them.
(In reply to Amelia Bellamy-Royds from comment #38)
> So glad to see someone working on this!
> 
> For part 1 (cloned <symbol> becomes <svg>), you might want to take a look at
> how I recommended handling it for the SVG 2 specs:
> https://www.w3.org/TR/SVG2/struct.html#SymbolElement
> https://www.w3.org/TR/SVG2/styling.html#UAStyleSheet
> 
> A <symbol> in SVG 2 becomes a renderable element that is mostly equivalent
> to a nested <svg>, except that an !important user agent stylesheet rule
> hides it unless it is in a direct child of a shadow-DOM root hosted by a
> <use> element.  That way, you can keep the cloned element's tag name as
> <symbol> while still describing the different display behaviors in standard
> CSS.
> 
> For part 2 (respecting the shadow-DOM boundary), you'll want to coordinate
> with the code for web components shadow DOM.  The extra complication for
> <use> is that if the cloned graphics are from the same document, then the
> stylesheets from that document get cloned into the shadow tree's scope, too.
> https://www.w3.org/TR/SVG2/struct.html#UseStyleInheritance
Thanks for all these valuable information. I will try to match the spec as possible as I can while implementation.
Almost all duplicated bugs can be fixed with this hack. Except 
https://codepen.io/electerious/pen/yyYeBW
Attachment #8874763 - Attachment is obsolete: true
Attachment #8877064 - Flags: review?(cam)
Attachment #8877065 - Flags: review?(cam)
Attachment #8877066 - Flags: review?(cam)
Attachment #8877067 - Flags: review?(cam)
Attachment #8874817 - Flags: review?(cam)
Attachment #8877068 - Flags: review?(cam)
Comment on attachment 8877065 [details]
Bug 265894 - Part 2. A cloned symbol element instance should not match rules of svg tag.

https://reviewboard.mozilla.org/r/148410/#review153242

::: dom/base/Element.h:1349
(Diff revision 2)
> +  virtual nsIAtom* GetTagNameAtom() const {
> +    return mNodeInfo->NameAtom();
> +  }

I'm concerned about this because we call this virtual method on every element that we are performing selector matching on, which is a place where performance is critical.  Apart from that, it feels a bit weird to me to expose a different tag name for selector matching.  If we ever start exposing the shadow tree's contents to script using Shadow DOM APIs (like the spec suggests), then we really should have a <symbol> at the root of the shadow tree.  (And it would be very odd to find an element whose .localName is "svg" but which returns true for .matches("symbol").)

Instead of creating an <svg> element and pretending it is <symbol> for selector matching, can we instead just change nsCSSFrameConstructor::FindSVGData to create an nsSVGInnerSVGFrame for <symbol>s at the root of a <use> shadow tree?  (Maybe it would make sense to rename nsSVGInnerSVGFrame to something else, like SVGViewportFrame.)
Comment on attachment 8877067 [details]
Bug 265894 - Part 4. Selectors should not cross use-element shadow tree boundary.

https://reviewboard.mozilla.org/r/148414/#review153252

::: layout/style/nsCSSRuleProcessor.cpp:2419
(Diff revision 2)
> -      nsIContent *content = prevElement->GetParent();
> +      nsIContent *content =
> +        prevElement->IsInAnonymousSubtree() && prevElement->IsAnonymousContentInSVGUseSubtree()
> +          ? prevElement->GetParentInsideUseElementShadowTree()
> +          : prevElement->GetParent();

There are a few other calls to GetParent() in nsCSSRuleProcessor, but I guess we don't really need to update them.  (We can end up setting slow selector flags on the parent, if we have rules like |div + symbol| or |symbol:first-child|, but that's probably rare.)

It would also be good to avoid the virtual function calls here, I think, and we can limit the work it does a bit by checking for NODE_IS_ANONYMOUS_ROOT which is set here:

  http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/layout/base/nsCSSFrameConstructor.cpp#4273-4276

So what about just something like this:

  nsIContent* content = prevElement->GetParent();
  if (prevElement->IsRootOfAnonymousSubtree() &&
      content->IsSVGElement(nsGkAtoms::use)) {
    content = nullptr;
  }

plus a comment explaining why we're doing this.
Comment on attachment 8877066 [details]
Bug 265894 - Part 3. Setup display prop value of the cloned symbol element.

https://reviewboard.mozilla.org/r/148412/#review153266

::: dom/svg/SVGSVGElement.cpp:1244
(Diff revision 2)
> +  nsAutoString displayOfHost;
> +  if (aShadowHost->GetPrimaryFrame()) {
> +    nsCSSKeyword keyword =
> +      nsCSSProps::ValueToKeywordEnum(aShadowHost->GetPrimaryFrame()->StyleDisplay()->mDisplay,
> +                                     nsCSSProps::kDisplayKTable);
> +
> +    AppendUTF8toUTF16(nsCSSKeywords::GetStringValue(keyword), displayOfHost);
>  
> +    displayOfHost =
> +      NS_LITERAL_STRING("display:") + displayOfHost + NS_LITERAL_STRING(";");
> +  } else {
> +    displayOfHost = NS_LITERAL_STRING("display:block;");
> +  }
> +
> +  // The generated instance of a ‘symbol’ that is the direct referenced
> +  // element of a ‘use’ element must always have a computed value of inline
> +  // for the display property. In other words, it must be rendered whenever
> +  // the host ‘use’ element is rendered.
> +  nsAutoString inlineStyle;
> +  svgNode->GetAttr(kNameSpaceID_None, nsGkAtoms::style, inlineStyle);
> +  inlineStyle += displayOfHost;
> +  svgNode->SetAttr(kNameSpaceID_None, nsGkAtoms::style, inlineStyle, true);

Rather than messing with the style="" attribute like this, can we do it through a UA style sheet rule instead?  Looking at the parent's display property value and determining what to set display to on this element seems unnecessary.  All we need to do is force display to block or inline or some non-none value on the svgNode.  If the parent is display:none, it won't have a frame, and it won't create the anonymous content here.  If the parent is some other display value, we want svgNode to be display:inline or whatever.

So I think we can just unconditionally set |display: inline !important| in a UA style sheet.  What might be tricky is finding the right selector to use.  Adding a new pseudo-class which matches this element seems like the right thing to do, so:

  symbol:-moz-use-shadow-tree-root {
    display: inline !important;
  }

or something like that.  Implementing a pseudo-class like that should be similar to the existing :-moz-native-anonymous one.
Comment on attachment 8877065 [details]
Bug 265894 - Part 2. A cloned symbol element instance should not match rules of svg tag.

https://reviewboard.mozilla.org/r/148410/#review153242

> I'm concerned about this because we call this virtual method on every element that we are performing selector matching on, which is a place where performance is critical.  Apart from that, it feels a bit weird to me to expose a different tag name for selector matching.  If we ever start exposing the shadow tree's contents to script using Shadow DOM APIs (like the spec suggests), then we really should have a <symbol> at the root of the shadow tree.  (And it would be very odd to find an element whose .localName is "svg" but which returns true for .matches("symbol").)
> 
> Instead of creating an <svg> element and pretending it is <symbol> for selector matching, can we instead just change nsCSSFrameConstructor::FindSVGData to create an nsSVGInnerSVGFrame for <symbol>s at the root of a <use> shadow tree?  (Maybe it would make sense to rename nsSVGInnerSVGFrame to something else, like SVGViewportFrame.)

1. Rename nsSVGInnerSVGFrame as SVGViewportFrame.
2. Create a base class(SVGViewportElementBase?) for SVGSVGElement and SVGSymbolElement

OK, I already have an WIP, will update new patches later
Attached patch WIP (obsolete) — — Splinter Review 

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #54)
> can we instead just change
> nsCSSFrameConstructor::FindSVGData to create an nsSVGInnerSVGFrame for
> <symbol>s at the root of a <use> shadow tree?  (Maybe it would make sense to
> rename nsSVGInnerSVGFrame to something else, like SVGViewportFrame.)

The terms "root-<svg>", "outer-<svg>" and "inner-<svg>" are standard terms, so I'm not a fan of changing this frame name. Admittedly I haven't read through this bug and its patches yet though...

    
    

    
  
It's more that we need a term for a frame that's appropriate both for inner <svg> elements and for used <symbol> elements.  The essence of the frame is that it establishes a viewport and does some sizing stuff, so I thought SVGViewportFrame might be appropriate.  Not sure it's really necessary to have that as an abstract class, and then have nsSVGInnerSVGFrame and a new nsSVGSymbolFrame inherit from it.  I guess CJ will find out.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP
        — 
          — Splinter Review
      

    


  

        Attachment #8877503 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
  

        Attachment #8877064 -
        Attachment is obsolete: true

        Attachment #8877064 -
        Flags: review?(cam)

    
  

        Attachment #8877065 -
        Attachment is obsolete: true

        Attachment #8877065 -
        Flags: review?(cam)

    
  

        Attachment #8877066 -
        Attachment is obsolete: true

        Attachment #8877066 -
        Flags: review?(cam)

    
  

        Attachment #8877067 -
        Attachment is obsolete: true

        Attachment #8877067 -
        Flags: review?(cam)

    
  

        Attachment #8877068 -
        Attachment is obsolete: true

        Attachment #8877068 -
        Flags: review?(cam)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8877631 -
        Flags: review?(cam)

        Attachment #8877632 -
        Flags: review?(cam)

        Attachment #8877633 -
        Flags: review?(cam)

        Attachment #8877634 -
        Flags: review?(cam)

        Attachment #8877635 -
        Flags: review?(cam)

        Attachment #8877636 -
        Flags: review?(cam)

        Attachment #8877637 -
        Flags: review?(cam)

        Attachment #8877638 -
        Flags: review?(cam)

        Attachment #8877639 -
        Flags: review?(cam)

        Attachment #8877672 -
        Flags: review?(cam)

    
    

    
  
Comment on attachment 8877631 [details]
Bug 265894 - Part 1. Implement NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER.

https://reviewboard.mozilla.org/r/149078/#review153760

::: dom/base/Element.h:1815
(Diff revision 2)
> +#define NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER(_elementName)            \
> +nsresult                                                                    \
> +_elementName::Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult,   \
> +                    bool aPreallocateChildren) const                        \
> +{                                                                           \

Can you somehow have a single definition that both NS_IMPL_ELEMENT_CLONE_WITH_INIT and NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER uses?

Xidorn showed me a trick yesterday, that you could use:

#define EXPAND(...) __VA_ARGS__
#define NS_IMPL_ELEMENT_CLONE_WITH_INIT_HELPER(_elementName, _args) \
  ...
  _elementName *it = new _elementName(ni EXPAND _args);
  ...
#define NS_IMPL_ELEMENT_CLONE_WITH_INIT(_elementName) \
  NS_IMPL_ELEMENT_CLONE_WITH_INIT_HELPER(_elementName, ())
#define NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER(_elementName) \
  NS_IMPL_ELEMENT_CLONE_WITH_INIT_HELPER(_elementName, (, NOT_FROM_PARSER))
#undef EXPAND

        Attachment #8877631 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877632 [details]
Bug 265894 - Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it.

https://reviewboard.mozilla.org/r/149080/#review153764

::: dom/svg/SVGSVGElement.h:19
(Diff revision 2)
> -    : nsISVGPoint(&aPt->mPt, true), mElement(aPt->mElement) {}
> -
> -  NS_DECL_ISUPPORTS_INHERITED
> -  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMSVGTranslatePoint, nsISVGPoint)
>  
> -  virtual DOMSVGPoint* Copy() override;
> +typedef SVGViewportElementBase SVGSVGElementBase;

This makes me realise that maybe "SVGViewportElement" is a better name than "SVGViewportElementBase", since the "...Base" naming is usually used for these typedefs.

::: dom/svg/SVGSVGElement.h
(Diff revision 2)
> -  float PixelUnitToMillimeterX();
> -  float PixelUnitToMillimeterY();
> -  float ScreenPixelToMillimeterX();
> -  float ScreenPixelToMillimeterY();
> -  bool UseCurrentView();
> -  float CurrentScale();
> -  void SetCurrentScale(float aCurrentScale);
> -  already_AddRefed<nsISVGPoint> CurrentTranslate();
> -  void SetCurrentTranslate(float x, float y);
> -  uint32_t SuspendRedraw(uint32_t max_wait_milliseconds);
> -  void UnsuspendRedraw(uint32_t suspend_handle_id);
> -  void UnsuspendRedrawAll();
> -  void ForceRedraw();
> -  void PauseAnimations();
> -  void UnpauseAnimations();
> -  bool AnimationsPaused();
> -  float GetCurrentTime();
> -  void SetCurrentTime(float seconds);
> -  void DeselectAll();
> -  already_AddRefed<DOMSVGNumber> CreateSVGNumber();
> -  already_AddRefed<DOMSVGLength> CreateSVGLength();
> -  already_AddRefed<SVGAngle> CreateSVGAngle();
> -  already_AddRefed<nsISVGPoint> CreateSVGPoint();
> -  already_AddRefed<SVGMatrix> CreateSVGMatrix();
> -  already_AddRefed<SVGIRect> CreateSVGRect();
> -  already_AddRefed<SVGTransform> CreateSVGTransform();
> -  already_AddRefed<SVGTransform> CreateSVGTransformFromMatrix(SVGMatrix& matrix);
> -  using nsINode::GetElementById; // This does what we want
> -  already_AddRefed<SVGAnimatedRect> ViewBox();
> -  already_AddRefed<DOMSVGAnimatedPreserveAspectRatio> PreserveAspectRatio();
> -  uint16_t ZoomAndPan();
> -  void SetZoomAndPan(uint16_t aZoomAndPan, ErrorResult& rv);
> -  virtual nsSVGViewBox* GetViewBox() override;

A lot of these methods seem like they should remain here on SVGSVGElement, since they things implementing methods and properties on the SVGSVGElement Web IDL interface, which aren't also on SVGSymbolElement.

The viewBox and preserveAspectRatio related ones could move, since both elements support those, but I think the rest should stay here.

Similarly for a bunch of the private methods and fields below.  For example, WillBeOutermostSVG doesn't seem like something that is needed for symbol elements as well.  So please go through each of these and just move those up to SVGViewportElement which you know are needed for the common behaviour that <svg> and use-referenced-<symbol> have.

    
    

    
  
Comment on attachment 8877634 [details]
Bug 265894 - Part 3. Implement nsSVGViewportFrame, and let nsSVGInnerFrame inherit from it.

https://reviewboard.mozilla.org/r/149084/#review153770

I didn't read the code moving parts closely.

::: layout/svg/nsSVGViewportFrame.h:16
(Diff revision 2)
> +#include "nsSVGContainerFrame.h"
> +#include "nsISVGSVGFrame.h"
> +
> +class gfxContext;
> +
> +class nsSVGViewportFrame

Please add a comment here saying that this is a superclass for inner SVG frames and symbol frames.

::: layout/svg/nsSVGViewportFrame.h:21
(Diff revision 2)
> +  explicit nsSVGViewportFrame(nsStyleContext* aContext)
> +    : nsSVGDisplayContainerFrame(aContext, kClassID)
> +  {
> +  }

Do we need this constructor?  If not (and I think we don't, since it's not a concrete class we'll instantiate), please remove it.

::: layout/svg/nsSVGViewportFrame.h:25
(Diff revision 2)
> +protected:
> +  explicit nsSVGViewportFrame(nsStyleContext* aContext)
> +    : nsSVGDisplayContainerFrame(aContext, kClassID)
> +  {
> +  }
> +  explicit nsSVGViewportFrame(nsStyleContext* aContext, nsIFrame::ClassID aID)

No need for explicit here.

        Attachment #8877634 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877635 [details]
Bug 265894 - Part 5. Implement nsSVGSymbolFrame to layout symbol element.

https://reviewboard.mozilla.org/r/149086/#review153772

::: layout/svg/nsSVGSymbolFrame.cpp:28
(Diff revision 2)
> +nsSVGSymbolFrame::Init(nsIContent*       aContent,
> +                       nsContainerFrame* aParent,
> +                         nsIFrame*         aPrevInFlow)

Nit: fix up indenting.

        Attachment #8877635 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877636 [details]
Bug 265894 - Part 6. Create symbol frame in nsCSSFrameConstructor.

https://reviewboard.mozilla.org/r/149088/#review153774

        Attachment #8877636 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877637 [details]
Bug 265894 - Part 7. Do not convert symbol element into svg element.

https://reviewboard.mozilla.org/r/149090/#review153776

::: commit-message-82deb:1
(Diff revision 2)
> +Bug 265894 - Part 7. Do not covert symbol element into svg element.

convert

::: layout/svg/nsSVGContainerFrame.cpp:378
(Diff revision 2)
> -  MOZ_ASSERT(mContent->IsSVGElement(nsGkAtoms::svg) ||
> +  MOZ_ASSERT(mContent->IsAnyOfSVGElements(nsGkAtoms::svg) ||
> +             mContent->IsSVGElement(nsGkAtoms::symbol) ||

mContent->IsAnyOfSVGElements(nsGkAtoms::svg, nsGkAtoms::symbol)

        Attachment #8877637 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877638 [details]
Bug 265894 - Part 8. Implement -moz-use-shadow-tree-root pseudo class.

https://reviewboard.mozilla.org/r/149092/#review153778

::: layout/style/nsCSSPseudoClasses.cpp:142
(Diff revision 2)
> +    case CSSPseudoClassType::mozUseShadowTreeRoot:
> +      return Some(aElement->IsInAnonymousSubtree() && aElement->IsAnonymousContentInSVGUseSubtree());

As I mentioned in a previous comment, I think we can do this check so that it's a bit cheaper:

  aElement->IsRootOfAnonymousSubtree() &&
  aElement->GetParent()->IsSVGElement(nsGkAtoms::use)

This avoids doing the second part of the check when we're in the use element shadow tree somewhere deeper than the root.  And because we only need to check when we know we're the root of the shadow tree, we can use GetParent() rather than GetBindingParent() (a virtual call) which is what IsAnonymousContentInSVGUseSubtree does.

        Attachment #8877638 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877639 [details]
Bug 265894 - Part 9. Selectors should not cross use-element shadow tree boundary.

https://reviewboard.mozilla.org/r/149094/#review153780

::: layout/style/nsCSSRuleProcessor.cpp:2437
(Diff revision 2)
> +      if (prevElement->IsRootOfAnonymousSubtree() &&
> +          content->IsSVGElement(nsGkAtoms::use)) {

Since this is the same as the condition in the previous patch, you could factor that out into a new inline method on nsINode, if you want.

::: layout/style/nsCSSRuleProcessor.cpp:2440
(Diff revision 2)
>      else {
>        nsIContent *content = prevElement->GetParent();
> +      if (prevElement->IsRootOfAnonymousSubtree() &&
> +          content->IsSVGElement(nsGkAtoms::use)) {
> +        // 'content' is the shadow root of an use-element shadow tree.
> +        // Accroding to the spec, we should not match rules cross the shadow

According

        Attachment #8877639 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8874817 [details]
Bug 265894 - Part 14. Reftest for style matching in use-element shadow tree.

https://reviewboard.mozilla.org/r/146204/#review153782

::: layout/reftests/svg/use-element-shadow-tree-rule-matching.html:4
(Diff revision 5)
> +  div {
> +	  width: 100px;
> +	  height:100px;
> +    position: fixed;
> +  }

The tabs in this file make the indenting look strange.  Please replace them with spaces.

::: layout/reftests/svg/use-element-shadow-tree-rule-matching.html:11
(Diff revision 5)
> +	  height:100px;
> +    position: fixed;
> +  }
> +
> +  /* #outer is in main DOM, circle instance in use-element shadow DOM,
> +     dhould not match this rule for cloned circle instance. */

should

::: layout/reftests/svg/use-element-shadow-tree-rule-matching.html:19
(Diff revision 5)
> +	  fill: red;
> +	  stroke-width: 4px;
> +  }
> +
> +  /* div is in main DOM, rect instance in use-element shadow DOM.
> +     dhould not match this rule for cloned rect instance. */

should

::: layout/reftests/svg/use-element-shadow-tree-rule-matching.html:55
(Diff revision 5)
> +    <symbol id="symbol">
> +      <circle cx="25" cy="25" r="25" />
> +    </symbol>
> +  </defs>
> +
> +  <g id="outter">

"outer" (and in the style sheet)

::: layout/reftests/svg/use-element-shadow-tree-rule-matching.html:72
(Diff revision 5)
> +  <svg>
> +    <use xlink:href="#symbol" fill="lime" width="50" height="50" />
> +  </svg>
> +</div>
> +
> +<div  style="left: 220px; top: 10px;">

Nit: one too many spaces after "div".

        Attachment #8874817 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8877672 [details]
Bug 265894 - Part 15. (Stylo) Fix counterpart of Part 8 and Part 9 in stylo.

https://reviewboard.mozilla.org/r/149120/#review153784

::: servo/components/selectors/matching.rs:569
(Diff revision 1)
>                      *relevant_link = RelevantLinkStatus::NotLooking;
>                      (element.prev_sibling_element(),
>                       SelectorMatchingResult::NotMatchedAndRestartFromClosestDescendant)
>                  }
>                  Combinator::Child | Combinator::Descendant => {
> +                    if element.is_root_of_use_element_anonymous_subtree() {

Since Servo's selector matching is in theory in a separate crate (even though we temporarily have it in the Servo tree here), I think we should use a more generic function name to determine whether selector matching should stop traversing up the tree here.

How about naming it something like "blocks_ancestor_combinators"?

::: servo/components/selectors/tree.rs:83
(Diff revision 1)
>      ///
>      /// Note: this can be false even if `.parent_element()` is `None`
>      /// if the parent node is a `DocumentFragment`.
>      fn is_root(&self) -> bool;
> +
> +    fn is_root_of_use_element_anonymous_subtree(&self) -> bool {

Please add a comment here saying what this function is used for (once it's renamed).

::: servo/components/style/gecko/wrapper.rs:1619
(Diff revision 1)
> +        if self.flags() & (NODE_IS_ANONYMOUS_ROOT as u32) == 0 {
> +            return false
> +        }
> +
> +        match self.parent_element() {
> +            Some(e) => e.get_local_name().as_ptr() == atom!("use").as_ptr(),

We should check the namespace too.

        Attachment #8877672 -
        Flags: review?(cam) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8877632 -
        Flags: review?(cam)

        Attachment #8878364 -
        Flags: review?(cam)

    
    

    
  
why do symbol elements need framparser in their constructor now? i.e. why does an SVGViewportlement take aFromParser in its constructor. aFromParser should be <svg> element specific.

    
    

    
  
(In reply to Robert Longson from comment #126)
> why do symbol elements need framparser in their constructor now? i.e. why
> does an SVGViewportlement take aFromParser in its constructor. aFromParser
> should be <svg> element specific.
no need. I will remove it. Thank you for catching it up

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8877632 [details]
Bug 265894 - Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it.

https://reviewboard.mozilla.org/r/149080/#review154854

This is looking better, but there are a few more things that I think should stay on SVGSVGElement.

::: dom/svg/SVGViewportElement.h:49
(Diff revision 6)
> +// Stores svgView arguments of SVG fragment identifiers.
> +class SVGView {
> +public:
> +  SVGView();
> +
> +  nsSVGEnum                             mZoomAndPan;
> +  nsSVGViewBox                          mViewBox;
> +  SVGAnimatedPreserveAspectRatio        mPreserveAspectRatio;
> +  nsAutoPtr<nsSVGAnimatedTransformList> mTransforms;
> +};

Views are something specific to <svg> elements.  Can we keep that in SVGSVGElement?

::: dom/svg/SVGViewportElement.h:143
(Diff revision 6)
> +  void ChildrenOnlyTransformChanged(uint32_t aFlags = 0);
> +
> +  gfx::Matrix GetViewBoxTransform() const;
> +
> +  // WebIDL
> +  using nsINode::GetElementById; // This does what we want

This should stay on SVGSVGElement, since its IDL interface defines a getElementById function, but SVGSymbolElement doesn't.

::: dom/svg/SVGViewportElement.h:152
(Diff revision 6)
> +  // Methods for <image> elements to override my "PreserveAspectRatio" value.
> +  // These are private so that only our friends
> +  // (AutoPreserveAspectRatioOverride in particular) have access.
> +  void SetImageOverridePreserveAspectRatio(const SVGPreserveAspectRatio& aPAR);
> +  void ClearImageOverridePreserveAspectRatio();
> +
> +  // Set/Clear properties to hold old version of preserveAspectRatio
> +  // when it's being overridden by an <image> element that we are inside of.
> +  bool SetPreserveAspectRatioProperty(const SVGPreserveAspectRatio& aPAR);
> +  const SVGPreserveAspectRatio* GetPreserveAspectRatioProperty() const;
> +  bool ClearPreserveAspectRatioProperty();

This preserveAspectRatio-overriding stuff is specific to root <svg> elements, when an <image> is referencing an entire SVG document.  So ideally they would stay on SVGSVGElement.  (Maybe you could keep GetPreserveAspectRatioProperty here, so that SVGViewportElement::GetPreserveAspectRatioWithOverride can still call it, and get null for <symbol> elements.)

::: dom/svg/SVGViewportElement.h:174
(Diff revision 6)
> +  }
> +
> +  /**
> +   * Returns true if either this is an SVG <svg> element that is the child of
> +   * another non-foreignObject SVG element, or this is a SVG <symbol> element
> +   * this is the root of an use-element shadow tree.

s/an/a/

::: dom/svg/SVGViewportElement.h:182
(Diff revision 6)
> +    const nsIContent *parent = GetFlattenedTreeParent();
> +    return parent && parent->IsSVGElement() &&
> +           !parent->IsSVGElement(nsGkAtoms::foreignObject);
> +  }
> +
> +  SVGViewElement* GetCurrentViewElement() const;

<view> element stuff is specific to <svg> elements.  So can you make this a virtual function here on SVGViewportElement, returning null, then override it on SVGSVGElement with the current method body?  Then you can keep mCurrentViewID on SVGSVGElement.

mSVGView should also stay on SVGSVGElement.  So we should perhaps have another virtual function that on SVGViewportElement returns null, and on SVGSVGElement returns mSVGView.  The functions which have been moved to SVGViewportElement that need to look at mSVGView can then call this new function.

::: dom/svg/SVGViewportElement.h:203
(Diff revision 6)
> +  enum { ATTR_X, ATTR_Y, ATTR_WIDTH, ATTR_HEIGHT };
> +  nsSVGLength2 mLengthAttributes[4];
> +  static LengthInfo sLengthInfo[4];
> +  virtual LengthAttributesInfo GetLengthInfo() override;
> +
> +  enum { ZOOMANDPAN };

This enum should stay right above the mEnumAttributes definition on SVGSVGElement.  (It's meant to be an index into that array.)

::: dom/svg/SVGViewportElement.h:225
(Diff revision 6)
> +  // zoom and pan
> +  // IMPORTANT: see the comment in RecordCurrentScaleTranslate before writing
> +  // code to change any of these!
> +  SVGPoint mCurrentTranslate;
> +  float    mCurrentScale;
> +  SVGPoint mPreviousTranslate;
> +  float    mPreviousScale;

This functionality is <svg> element specific too.  So these, and the methods that get and set their values, should stay up in SVGSVGElement.  Since we need access to their values in places like SVGViewportElement::PrependLocalTransformsTo, we can add a virtual GetCurrentScaleTranslate function that grabs them.

::: dom/svg/SVGViewportElement.h:233
(Diff revision 6)
> +  SVGPoint mCurrentTranslate;
> +  float    mCurrentScale;
> +  SVGPoint mPreviousTranslate;
> +  float    mPreviousScale;
> +
> +  bool     mImageNeedsTransformInvalidation;

Because this also is related to <image> elements referencing an entire SVG document, this is specific to root <svg> elements.  So it would make most sense on SVGSVGElement.  (That should be possible if the preserveAspectRatio-overriding stuff stays there too, since that, and FlushImageTransformInvalidation, is where this boolean is used.)

::: dom/svg/SVGViewportElement.h:239
(Diff revision 6)
> +class MOZ_RAII AutoPreserveAspectRatioOverride
> +{

If the preserveAspectRatio-overriding stuff stays in SVGSVGElement, then this class can stay there too, and keep working on SVGSVGElement objects instead of SVGViewportElement objects.

    
    

    
  
Comment on attachment 8877633 [details]
Bug 265894 - Part 4. Let SVGSymbolElement inherit from SVGViewportElementBase.

https://reviewboard.mozilla.org/r/149082/#review154878

        Attachment #8877633 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8878364 [details]
Bug 265894 - Part 10. Remove SVGSVGElement::HasPreserveAspectRatio.

https://reviewboard.mozilla.org/r/149708/#review154884

        Attachment #8878364 -
        Flags: review?(cam) → review+
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Priority: P3 → P1

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8877632 -
        Flags: review?(cam)

        Attachment #8879076 -
        Flags: review?(cam)

        Attachment #8879077 -
        Flags: review?(cam)

        Attachment #8879078 -
        Flags: review?(cam)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8877632 [details]
Bug 265894 - Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it.

https://reviewboard.mozilla.org/r/149080/#review155472

        Attachment #8877632 -
        Flags: review+

    
    

    
  
Comment on attachment 8879076 [details]
Bug 265894 - Part 3. Move preserveAspectRatio-overriding stuff back to SVGSVGElement.

https://reviewboard.mozilla.org/r/150402/#review155476

        Attachment #8879076 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879077 [details]
Bug 265894 - Part 4. Move translate/scale back to SVGSVGElement.

https://reviewboard.mozilla.org/r/150404/#review155478

        Attachment #8879077 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8879078 [details]
Bug 265894 - Part 5. Move svg-view back to SVGSVGElement.

https://reviewboard.mozilla.org/r/150406/#review155480

        Attachment #8879078 -
        Flags: review?(cam) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8879076 -
        Attachment is obsolete: true

    
  

        Attachment #8879077 -
        Attachment is obsolete: true

    
  

        Attachment #8879078 -
        Attachment is obsolete: true

    
  

        Attachment #8874817 -
        Attachment is obsolete: true

    
  

        Attachment #8877672 -
        Attachment is obsolete: true

    
  
Whiteboard: leave-open

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a01ef15db657
Part 1. Implement NS_IMPL_ELEMENT_CLONE_WITH_INIT_AND_PARSER. r=heycam
https://hg.mozilla.org/integration/autoland/rev/64387ced7f42
Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9f7453c7e536
Part 3. Implement nsSVGViewportFrame, and let nsSVGInnerFrame inherit from it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ae126ea896d3
Part 4. Let SVGSymbolElement inherit from SVGViewportElementBase. r=heycam
https://hg.mozilla.org/integration/autoland/rev/af1ed1ce5c49
Part 5. Implement nsSVGSymbolFrame to layout symbol element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/538098e953bf
Part 6. Create symbol frame in nsCSSFrameConstructor. r=heycam
https://hg.mozilla.org/integration/autoland/rev/69c8936686f6
Part 7. Do not convert symbol element into svg element. r=heycam
https://hg.mozilla.org/integration/autoland/rev/abeaf8ef5a3b
Part 8. Implement -moz-use-shadow-tree-root pseudo class. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6d2b730f5c46
Part 9. Selectors should not cross use-element shadow tree boundary. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bf1c714b2e13
Part 10. Remove SVGSVGElement::HasPreserveAspectRatio. r=heycam

    
    

    
  
Stylo pull request:
https://github.com/servo/servo/pull/17427

    
    

    
  
All the stylo failures on autoland(https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=108512813) are relative to stylo PR(https://github.com/servo/servo/pull/17427). Will be fixed after PR merged.

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43184c6bca45
Part 11. Reftest for style matching in use-element shadow tree. r=heycam

    
  
Whiteboard: leave-open

    
    

    
  
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93e4115d1867
(followup) Remove SVGSVGElement::HasPreserveAspectRatio declaration. r=me

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/43184c6bca45
https://hg.mozilla.org/mozilla-central/rev/93e4115d1867
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox56:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

    
    

    
  
I've just come to look at documenting this bug, and can't really see what exactly I'd need to update in terms of web developer-facing information. It looks like a bugfix/internals update to me.

If there is anything I need to document on MDN, can you give me some hints?

thanks.
Flags: needinfo?(aschen)

    
    

    
  
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #193)
> I've just come to look at documenting this bug, and can't really see what
> exactly I'd need to update in terms of web developer-facing information. It
> looks like a bugfix/internals update to me.
> 
> If there is anything I need to document on MDN, can you give me some hints?

There is a direct reference to this bug in the SVG implementation status page: https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_1.1_Support_in_Firefox

But beyond that, it doesn't look like the main docs about <use> and CSS in SVG were ever detailed enough to have warnings about this bug.

    
    

    
  
(In reply to Amelia Bellamy-Royds from comment #194)
> There is a direct reference to this bug in the SVG implementation status
> page:
> https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_1.1_Support_in_Firefox
> 
> But beyond that, it doesn't look like the main docs about <use> and CSS in
> SVG were ever detailed enough to have warnings about this bug.

Thanks for your feedback.
Yes, it would be great to detail this bug and include the fix info on MDN doc too.
Flags: needinfo?(aschen)

    
    

    
  
(In reply to Amelia Bellamy-Royds from comment #194)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #193)
> > I've just come to look at documenting this bug, and can't really see what
> > exactly I'd need to update in terms of web developer-facing information. It
> > looks like a bugfix/internals update to me.
> > 
> > If there is anything I need to document on MDN, can you give me some hints?
> 
> There is a direct reference to this bug in the SVG implementation status
> page:
> https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_1.1_Support_in_Firefox
> 
> But beyond that, it doesn't look like the main docs about <use> and CSS in
> SVG were ever detailed enough to have warnings about this bug.

Thanks Amelia, this is really helpful.

I don't have time to do a detailed write-up of this stuff, so I've gone for the cheaper option of adding a note to the <use> page that links to your excellent SO writeup for more details:

https://developer.mozilla.org/en-US/docs/Web/SVG/Element/use#Notes

    
  
Depends on: 1390088
Depends on: 1398806

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: