Closed
Bug 1390088
Opened 7 years ago
Closed 7 years ago
SVG within SVG (embedded) not displaying correctly in 56.0b2.
Categories
(Core :: SVG, defect, P1)
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)
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.
Updated•7 years ago
|
Component: Untriaged → SVG
Product: Firefox → Core
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=edf38274225e72c180b87fde8774c4954b3cc595&tochange=bf1c714b2e1394aca3590ac7ad661bdc735bbb35 Regressed by: Bug 265894
Blocks: 265894
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Keywords: regressionwindow-wanted → regression
Updated•7 years ago
|
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8899366 -
Flags: review?(cam)
Attachment #8899367 -
Flags: review?(cam)
Attachment #8899368 -
Flags: review?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8899366 -
Flags: review?(cam)
Attachment #8899367 -
Flags: review?(cam)
Attachment #8899368 -
Flags: review?(cam)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8899366 -
Flags: review?(jwatt)
Attachment #8899367 -
Flags: review?(jwatt)
Attachment #8899368 -
Flags: review?(jwatt)
Assignee | ||
Comment 17•7 years ago
|
||
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?
Comment 18•7 years ago
|
||
Sure thing.
Comment 19•7 years ago
|
||
mozreview-review |
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 20•7 years ago
|
||
mozreview-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 21•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
mozreview-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-<svg> resolves against the nearest <svg> ancestor, not the outer-<svg></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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
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
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30853c7b5caa https://hg.mozilla.org/mozilla-central/rev/d125a108775c https://hg.mozilla.org/mozilla-central/rev/e538796068a5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 30•7 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(cku)
Flags: in-testsuite+
Assignee | ||
Comment 31•7 years ago
|
||
try result on beta https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2824c19ebfd4252c5108f7c58365c8a8e32f64
Flags: needinfo?(cku)
Assignee | ||
Comment 32•7 years ago
|
||
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 33•7 years ago
|
||
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+
Comment 34•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6dc06b2e7ed1 https://hg.mozilla.org/releases/mozilla-beta/rev/03ac0153495d https://hg.mozilla.org/releases/mozilla-beta/rev/b7c7741c9be5
Comment 35•7 years ago
|
||
(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.
Description
•