Closed Bug 1390088 Opened 3 years ago Closed 2 years ago

SVG within SVG (embedded) not displaying correctly in 56.0b2.

Categories

(Core :: SVG, defect, P1)

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: Joel.Cedric, Assigned: u459114)

References

Details

(Keywords: regression)

Attachments

(5 files)

Attached file test2.html
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170810180547

Steps to reproduce:

I am currently working on an application that displays nodes and edges of a graph on svg. I reuse a number of icons (those icons are svgs themselves) within the parent svgs. See also attached example svg file; this svg looks correctly in Firefox v55/chrome/edge/opera, but not in 56.0b2. 


Actual results:

The embedded svg icons are not displayed correctly in the latest version 56.0b2.  


Expected results:

The embedded svg icons should just be displayed as in Firefox v55/chrome/opera/edge.
Component: Untriaged → SVG
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
:cjku
Your patch seems to cause the regression.
Flags: needinfo?(cku)
Assignee: nobody → cku
Flags: needinfo?(cku)
Status: NEW → ASSIGNED
Priority: -- → P1
The 2nd patch landed in bug 265894 cause this regression:
Bug 265894 - Part 2. Implement SVGViewportElementBase, and let SVGSVGElement inherit from it.
https://hg.mozilla.org/integration/autoland/rev/64387ced7f42
Attachment #8899366 - Flags: review?(cam)
Attachment #8899367 - Flags: review?(cam)
Attachment #8899368 - Flags: review?(cam)
Attachment #8899366 - Flags: review?(cam)
Attachment #8899367 - Flags: review?(cam)
Attachment #8899368 - Flags: review?(cam)
Attachment #8899366 - Flags: review?(jwatt)
Attachment #8899367 - Flags: review?(jwatt)
Attachment #8899368 - Flags: review?(jwatt)
hi jwatt,
This bug is a regression of Bug 265894. Although that bug is reviewed by heycam, but since heycam's review queues is totally full. Whould you please help to review these patch?
Sure thing.
Comment on attachment 8899366 [details]
Bug 1390088 - Part 1. Change the type of the parameter of nsSVGLength2::GetAnimValue from "SVGSVGElement*" to "SVGViewportElement*"

https://reviewboard.mozilla.org/r/170598/#review176760

::: commit-message-7dddb:4
(Diff revision 4)
> +Bug 1390088 - Part 1. Change the type of the parameter of nsSVGLength2::GetAnimValue from "SVGSVGElement*" to "SVGViewportElement*"
> +
> +There are two overloads of nsSVGLength2::GetAnimValue:
> +1. float nsSVGLength2::GetAnimValue(nsSVGElement*) const;

Put a line break before this list.

::: commit-message-7dddb:9
(Diff revision 4)
> +1. float nsSVGLength2::GetAnimValue(nsSVGElement*) const;
> +2. float nsSVGLength2::GetAnimValue(SVGSVGElement*) const;
> +
> +In Bug 265894, I created SVGViewportElement as a base class of SVGSVGElement.
> +SVGSVGElement::GetViewBoxTransform was moved to SVGViewportElement in that
> +refactory.

That should be "refactoring".

::: commit-message-7dddb:11
(Diff revision 4)
> +
> +In Bug 265894, I created SVGViewportElement as a base class of SVGSVGElement.
> +SVGSVGElement::GetViewBoxTransform was moved to SVGViewportElement in that
> +refactory.
> +
> +There is a local variable "ctx" in that function. before we move

Don't start a new paragraph here. This should be joined to the previous one. In fact remove this paragraph and the following one, and instead add the following to the previous paragraph:

  The local variable 'ctx' in that function was changed from SVGSVGElement to SVGViewportElement, which when passed to nsSVGLength2::GetAnimValue caused us to switch from calling the overload that takes a SVGSVGElement* to the overload that takes a nsSVGElement*, which is not what we want.

::: commit-message-7dddb:19
(Diff revision 4)
> +second overload function.
> +
> +After the change, ctx was declared as SVGViewportElement type[2], so compiler
> +choose the first one, which is not what we expect.
> +
> +We can fix it by replacing param type of the second overload from

Change this to:

This patch changes the argument type of the nsSVGLength2::GetAnimValue overload that takes an SVGSVGElement* to take an SVGViewportElement* instead, which causes the GetAnimValue(ctx) calls in SVGViewportElement::GetViewBoxTransform to call the correct GetAnimValue overload again.

::: dom/svg/nsSVGLength2.h:58
(Diff revision 4)
>  
>  class SVGElementMetrics : public UserSpaceMetrics
>  {
>  public:
>    explicit SVGElementMetrics(nsSVGElement* aSVGElement,
> -                             mozilla::dom::SVGSVGElement* aCtx = nullptr);
> +                             mozilla::dom::SVGViewportElement* aCtx = nullptr);

Please add:

using mozilla::dom::SVGViewportElement SVGViewportElement;

at the top of the class so that you don't need to qualify the type here and further down where mCtx is defined.

In fact there are a bunch of places in this patch where you could remove the namespace qualification. Please do that.

::: dom/svg/nsSVGLength2.h:170
(Diff revision 4)
>    bool mIsBaseSet:1;
>  
>    float GetUnitScaleFactor(nsIFrame *aFrame, uint8_t aUnitType) const;
>    float GetUnitScaleFactor(const UserSpaceMetrics& aMetrics, uint8_t aUnitType) const;
>    float GetUnitScaleFactor(nsSVGElement *aSVGElement, uint8_t aUnitType) const;
> -  float GetUnitScaleFactor(mozilla::dom::SVGSVGElement *aCtx, uint8_t aUnitType) const;
> +  float GetUnitScaleFactor(mozilla::dom::SVGViewportElement *aCtx, uint8_t aUnitType) const;

Please left-hug the "*" while you're touching this line.

::: dom/svg/nsSVGLength2.cpp:226
(Diff revision 4)
>  {
>    return GetUnitScaleFactor(SVGElementMetrics(aSVGElement), aUnitType);
>  }
>  
>  float
> -nsSVGLength2::GetUnitScaleFactor(SVGSVGElement *aCtx, uint8_t aUnitType) const
> +nsSVGLength2::GetUnitScaleFactor(SVGViewportElement *aCtx, uint8_t aUnitType) const

Left-hug the "*".
Attachment #8899366 - Flags: review?(jwatt) → review+
Comment on attachment 8899367 [details]
Bug 1390088 - Part 2. Add a reftest to check percentage width/height on inner-<svg> resolves against the nearest <svg> ancestor.

https://reviewboard.mozilla.org/r/170600/#review176770

Please find a logical places under layout/reftests/svg for this to live. We shouldn't generally be adding to layout/reftests/bugs since it makes it hard to figure out what we do or don't have tests for.

Also please make the passing test be green (compare against lime.svg) since it's best for it to be visually clear when reftests pass.

::: layout/reftests/bugs/1390088.html:8
(Diff revision 4)
> +  <rect x="50" y="50" width="50" height="50" fill="red"/>
> +  <svg width="100" height="100">
> +    <!-- Should read size atrribute from the parent inner svg element,
> +         instead of the root svg element. -->
> +    <svg viewBox="0 0 100 100">
> +	  <rect x="50" y="50" width="50" height="50" fill="white"/>

Please fix the indentation of the <rect> and </svg>.
Attachment #8899367 - Flags: review?(jwatt) → review-
Comment on attachment 8899368 [details]
Bug 1390088 - Part 3. Declare some locale nsSVGLength2 references as const in nsSVGOuterSVGFrame.cpp.

https://reviewboard.mozilla.org/r/170602/#review176778

Nice extra improvement.

::: commit-message-e1fa1:1
(Diff revision 4)
> +Bug 1390088 - Part 3. (Minor) Declare local nsSVGLength2 references as const if we do not intend to modify them.

Just make the summary "Declare some locale nsSVGLength2 references as const in nsSVGOuterSVGFrame.cpp"

No need for the "(Minor)" - it's clear from the summary. The "if we do not intend to modify them" is also redundant since that's clear from the fact that you're making them const. Mentioning nsSVGOuterSVGFrame.cpp is helpful since it gives people scanning commit messages an indication of where the code being touched is without having to look in more detail.

::: layout/svg/nsSVGOuterSVGFrame.cpp:216
(Diff revision 4)
>    // specified and we're embedded inside an nsIObjectLoadingContent.
>  
>    IntrinsicSize intrinsicSize;
>  
>    SVGSVGElement *content = static_cast<SVGSVGElement*>(mContent);
> -  nsSVGLength2 &width  = content->mLengthAttributes[SVGSVGElement::ATTR_WIDTH];
> +  const nsSVGLength2 &width =

Please type-hug the "&" here and elsewhere while you're touching these lines.
Attachment #8899368 - Flags: review?(jwatt) → review+
Comment on attachment 8899367 [details]
Bug 1390088 - Part 2. Add a reftest to check percentage width/height on inner-<svg> resolves against the nearest <svg> ancestor.

https://reviewboard.mozilla.org/r/170600/#review177102

Thanks, that's better.

::: commit-message-e0c70:1
(Diff revision 5)
> +Bug 1390088 - Part 2. A test case of nested svg elements.

This is too generic a summary line. Make it: "Add a reftest to check percentage width/height on inner-<svg> resolves against the nearest <svg> ancestor".

::: commit-message-e0c70:3
(Diff revision 5)
> +Bug 1390088 - Part 2. A test case of nested svg elements.
> +
> +To repro this issue, we need to put an SVG element inside another  inner SVG element.

Then delete this line.

::: layout/reftests/svg/reftest.list:419
(Diff revision 5)
>  == style-property-on-script-element-01.svg pass.svg
>  == style-without-type-attribute.svg pass.svg
>  
>  == svg-in-foreignObject-01.xhtml svg-in-foreignObject-01-ref.xhtml
>  fuzzy-if(skiaContent,1,2600) == svg-in-foreignObject-02.xhtml svg-in-foreignObject-01-ref.xhtml # reuse -01-ref.xhtml
> +== svg-in-inner-svg.svg pass.svg

Call this svg-in-inner-svg-dimensions.svg to give a bit more of a hint of what it does.

::: layout/reftests/svg/svg-in-inner-svg.svg:1
(Diff revision 5)
> +<svg xmlns="http://www.w3.org/2000/svg">

Add the following <title> element:

<title>Test that percentage width/height for inner-&lt;svg&gt; resolves against the nearest &lt;svg&gt; ancestor, not the outer-&lt;svg&gt;</title>

::: layout/reftests/svg/svg-in-inner-svg.svg:6
(Diff revision 5)
> +<svg xmlns="http://www.w3.org/2000/svg">
> +  <rect width="100%" height="100%" fill="lime"/>
> +  <!-- This red rect should be covered by the lime one below -->
> +  <rect x="50" y="50" width="50" height="50" fill="red"/>
> +  <svg width="100" height="100">
> +    <!-- Should read size atrribute from the parent inner svg element, instead

You can then drop this comment.
Attachment #8899367 - Flags: review?(jwatt) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30853c7b5caa
Part 1. Change the type of the parameter of nsSVGLength2::GetAnimValue from "SVGSVGElement*" to "SVGViewportElement*" r=jwatt
https://hg.mozilla.org/integration/autoland/rev/d125a108775c
Part 2. Add a reftest to check percentage width/height on inner-<svg> resolves against the nearest <svg> ancestor. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/e538796068a5
Part 3. Declare some locale nsSVGLength2 references as const in nsSVGOuterSVGFrame.cpp. r=jwatt
Please request Beta approval on this when you get a chance.
Flags: needinfo?(cku)
Flags: in-testsuite+
Comment on attachment 8899366 [details]
Bug 1390088 - Part 1. Change the type of the parameter of nsSVGLength2::GetAnimValue from "SVGSVGElement*" to "SVGViewportElement*"

Approval Request Comment
[Feature/Bug causing the regression]: Bug 265894
[User impact if declined]: The size of nested svg element can be wrong
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:no 
[List of other uplifts needed for the feature/fix]: this patch and part 2/3.
[Is the change risky?]: no
[Why is the change risky/not risky?]: verified by auto and manual test. code change is simple
[String changes made/needed]:N/A
Attachment #8899366 - Flags: approval-mozilla-beta?
Comment on attachment 8899366 [details]
Bug 1390088 - Part 1. Change the type of the parameter of nsSVGLength2::GetAnimValue from "SVGSVGElement*" to "SVGViewportElement*"

Fix an SVG regression and include a test. Beta56+.
Attachment #8899366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to C.J. Ku[:cjku](UTC+8) from comment #32)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:no 

Setting qe-verify- based C.J. Ku's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.