SVG image pattern not visible in Firefox

RESOLVED FIXED in Firefox -esr52

Status

()

Core
SVG
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: MartinParenteau1963, Assigned: cjku)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla55
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 2 obsolete attachments)

795 bytes, text/html
Details
59 bytes, text/x-review-board-request
mstange
: review+
tnikkel
: review+
Details | Review
59 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
59 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
4.28 KB, patch
cjku
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
59 bytes, text/x-review-board-request
tnikkel
: review+
Details | Review
2.77 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 months ago
strtestcase
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

-In an HTML page, fill an SVG rectangle with an image pattern (see markup below)
-Clear the browser cache (Options | Advanced | Network | Cached Web Content | Clear Now)
-Restart the browser
-Open the page in the browser

HTML markup to test the bug:

<div style="left: 0px; top: 0px; width: 240px; height: 180px;">
    <svg height="100%" width="100%" x="0%" y="0%" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
        <defs>
            <pattern id="pattern1" patternUnits="objectBoundingBox" preserveAspectRatio="xMidYMid slice" width="1" height="1" x="0" y="0" viewBox="0 0 4000 3000">
                <image xlink:href="http://www.public-domain-photos.com/free-stock-photos-3/landscapes/lakes/archipelago-4.jpg" width="4000" height="3000" x="0" y="0" preserveAspectRatio="xMidYMid slice"></image>
            </pattern>
        </defs>
        <rect fill="url('#pattern1')" stroke="black" stroke-width="2" x="0%" y="0%" width="100%" height="100%" fill-opacity="1.0" />
    </svg>
</div>


Actual results:

-In Chrome and IE11, the image is displayed correctly in the rectangle
-In Firefox 51 and 52 on Windows, the image is not displayed the first time
-If we reload the page in Firefox (the image is cached), then the image is displayed
-To workaround the problem, I have to preload every image that is to be used in an SVG pattern
-Note: I haven't tested with other versions or on another OS


Expected results:

In Firefox, the image should appear the first time the page is loaded

Comment 1

7 months ago
regression-window
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=369cab5b9f2e1cc89c06bd35e96bd7fe0f26754b&tochange=c0700bedb4f765a2fd55ab2bc71af4d6b322084d

Regressed by: Bug 1258510
Blocks: 1258510
Status: UNCONFIRMED → NEW
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
Component: Untriaged → SVG
Ever confirmed: true
Flags: needinfo?(cku)
Keywords: regression
Product: Firefox → Core
Version: 52 Branch → 50 Branch

Comment 2

7 months ago
testcase
Created attachment 8845465 [details]
Reporter's testcase
(Assignee)

Comment 3

7 months ago
interesting, I will look into it
Assignee: nobody → cku
Flags: needinfo?(cku)
(Assignee)

Comment 4

7 months ago
Rollback the change here, bug will be fixed
http://searchfox.org/mozilla-central/source/layout/svg/nsSVGPatternFrame.cpp#415
You probably want to return whatever was drawn there regardless of the DrawResult. SVGMaskFrame has the same problem. Keep in mind that DrawResult is only reporting on how drawing of any images went, not the drawing of anything else. Also looking over the patches from bug 1258510 I see a couple places where BAD_ARGS is returned if the transform matrix is singular. We would want to return SUCCESS in that case I think, because we drew what we were instructed to draw.
I suspect the difference is that if the matrix is singular, something in the drawing has to change, either the CSS or the markup itself which will then trigger a redraw. With the image itself, maybe it's still loading so nothing about the image changes.
Comment hidden (mozreview-request)
I don't think you want to use SUCCESS as the only succesful result. For example, WRONG_SIZE means we drew the image fine, but it wasn't a high quality scaled version at the exact size requested, we should still show this to the user.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

7 months ago
Created attachment 8845799 [details] [diff] [review]
Patch for 54/53/52
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8845773 - Flags: review?(tnikkel)
Attachment #8845773 - Flags: review?(mstange)
Attachment #8845776 - Flags: review?(tnikkel)
(Assignee)

Comment 16

7 months ago
Comment on attachment 8845799 [details] [diff] [review]
Patch for 54/53/52

Review of attachment 8845799 [details] [diff] [review]:
-----------------------------------------------------------------

Make the change in aurora/beta/release be simpler
Attachment #8845799 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8845776 [details]
Bug 1345853 - Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS

https://reviewboard.mozilla.org/r/118928/#review120972
Attachment #8845776 - Flags: review?(tnikkel) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Doesn't nsSVGMaskFrame has the same problem as nsSVGPatternFrame? It also returns nullptr if the DrawResult was not success:

+    if (result != DrawResult::SUCCESS) {
+      return nullptr;
+    }
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Please add a reftest for this too.
Flags: needinfo?(cku)
Flags: in-testsuite?
(Assignee)

Comment 28

7 months ago
(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> Doesn't nsSVGMaskFrame has the same problem as nsSVGPatternFrame? It also
> returns nullptr if the DrawResult was not success:
> 
> +    if (result != DrawResult::SUCCESS) {
> +      return nullptr;
> +    }

Return nullptr is fine. The bug in nsSVGPatternFrame is because we return too early:
nsSVGPatternFrame::PaintPattern(const DrawTarget* aDrawTarget,...
  ....
    DrawResult result = nsSVGUtils::PaintFrameWithEffects(kid, *gfx, tm);
    if (result != DrawResult::SUCCESS) {
      return nullptr; << If we return here, RemoveStateBits bellow will not be called.
    }
  }
  patternWithChildren->RemoveStateBits(NS_FRAME_DRAWING_AS_PAINTSERVER); <<<

And we didn't pass DrawResult back to display item.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

7 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)
> Please add a reftest for this too.

Yeap. Done.
Flags: needinfo?(cku)
(Assignee)

Updated

7 months ago
Attachment #8847744 - Flags: review?(tnikkel)
Sounds like we could use a RAII wrapper for patternWithChildren->AddStateBits(NS_FRAME_DRAWING_AS_PAINTSERVER); and its cancelling. Seems nsSVGIntegrationUtils.cpp may be broken.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

7 months ago
mozreview-review
Comment on attachment 8847744 [details]
Bug 1345853 - Part 5. Reftest.

https://reviewboard.mozilla.org/r/120668/#review122892
Attachment #8847744 - Flags: review?(tnikkel) → review+

Comment 41

7 months ago
mozreview-review
Comment on attachment 8845773 [details]
Bug 1345853 - Part 1. Pass DrawResult from nsSVGPatternFrame::PaintPattern back to nsDisplaySVGGeometry::Paint.

https://reviewboard.mozilla.org/r/118926/#review123126
Attachment #8845773 - Flags: review?(mstange) → review+
Attachment #8845799 - Flags: review?(mstange) → review+
Duplicate of this bug: 1347922

Comment 43

7 months ago
mozreview-review
Comment on attachment 8845773 [details]
Bug 1345853 - Part 1. Pass DrawResult from nsSVGPatternFrame::PaintPattern back to nsDisplaySVGGeometry::Paint.

https://reviewboard.mozilla.org/r/118926/#review123220
Attachment #8845773 - Flags: review?(tnikkel) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

7 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/836d16519edf
Part 1. Pass DrawResult from nsSVGPatternFrame::PaintPattern back to nsDisplaySVGGeometry::Paint. r=mstange,tnikkel
https://hg.mozilla.org/integration/autoland/rev/a46f3da8aba6
Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/fd5e7c8cbb6d
Part 3. Reftest. r=tnikkel

Comment 48

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/836d16519edf
https://hg.mozilla.org/mozilla-central/rev/a46f3da8aba6
https://hg.mozilla.org/mozilla-central/rev/fd5e7c8cbb6d
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

7 months ago
Duplicate of this bug: 1347771

Comment 50

7 months ago
Already 2 duplicates, if the bugfix is not risky, could we uplift the patch in ESR 52 to fix this recent regression?
Probably a bit big and risky for consideration as a dot release ride-along next week (I'd be open to hearing arguments to the contrary, though), but agreed that it would be nice to get this into 53/54/ESR52 if possible at least.
Flags: needinfo?(cku)
Flags: in-testsuite?
Flags: in-testsuite+
Oh wait, I missed that we have a way simpler branch patch attached. Yeah, I think we should consider this for dot release inclusion then too. Please request Aurora/Beta/Release/ESR52 approval on this when you get a chance.
(Assignee)

Comment 53

7 months ago
Comment on attachment 8845799 [details] [diff] [review]
Patch for 54/53/52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: user may not see graphic objects using a big image as a pattern of brush.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): In this simpler  version, I rollback original behavior of  nsSVGPatternFrame::PaintPattern in FF 49, should be very safe to do this.
String or UUID changes made by this patch: NA

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1258510
[User impact if declined]: User may not see a graphic object using a big image as a pattern of brush. 
[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]:none
[Is the change risky?]: no
[Why is the change risky/not risky?]: In this simpler version, I only rolled back original behavior of nsSVGPatternFrame::PaintPattern before bug 1258510 landed.
[String changes made/needed]: N/A
Flags: needinfo?(cku)
Attachment #8845799 - Flags: approval-mozilla-release?
Attachment #8845799 - Flags: approval-mozilla-esr52?
Attachment #8845799 - Flags: approval-mozilla-beta?
Attachment #8845799 - Flags: approval-mozilla-aurora?
The branch patch should include the fix to nsSVGMaskFrame::GetMaskForMaskedFrame, it has the same issue. Comparing the return value of PaintFrameWithEffects to SUCCESS isn't correct.
Flags: needinfo?(cku)
Backed out for frequently failing its own test pattern-big-image.html

https://hg.mozilla.org/integration/autoland/rev/dbabc189256e
https://hg.mozilla.org/integration/autoland/rev/5728195b4bc1
https://hg.mozilla.org/integration/autoland/rev/a54fc2172514

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=84574126&repo=autoland
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/svg/pattern-big-image.html == http://localhost:49305/1489739549856/516/pattern-big-image-ref.html | image comparison, max difference: 235, number of differing pixels: 100
(image seems to be missing)
Duplicate of this bug: 1348247
(Assignee)

Comment 57

7 months ago
Comment on attachment 8845799 [details] [diff] [review]
Patch for 54/53/52

Review of attachment 8845799 [details] [diff] [review]:
-----------------------------------------------------------------

cancel request since the patch in nightly had been backout
Attachment #8845799 - Flags: approval-mozilla-esr52?
Attachment #8845799 - Flags: approval-mozilla-beta?
Attachment #8845799 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

7 months ago
Attachment #8845799 - Flags: approval-mozilla-release?
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
Resolution: FIXED → ---
(Assignee)

Comment 58

7 months ago
Created attachment 8849192 [details] [diff] [review]
Patch for 54/53/52

Update patch according to tn's comment in comment 57
Attachment #8845799 - Attachment is obsolete: true
Flags: needinfo?(cku)
Comment on attachment 8849192 [details] [diff] [review]
Patch for 54/53/52

Thanks! I'll grant review just in case.
Attachment #8849192 - Flags: review+
Comment hidden (spam)
(Assignee)

Comment 61

7 months ago
Created attachment 8849441 [details] [diff] [review]
Patch for 54/53/52
Attachment #8849192 - Attachment is obsolete: true
Attachment #8849441 - Flags: review+
(Assignee)

Comment 62

7 months ago
Comment on attachment 8849441 [details] [diff] [review]
Patch for 54/53/52

Approval Request Comment
[Feature/Bug causing the regression]: bug 1258510
[User impact if declined]: user may not see graphic objects using a big image as a pattern image of brush
[Is this code covered by automated tests?]: no
[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]:no
[Is the change risky?]:no
[Why is the change risky/not risky?]:In this simpler  version, I rollback original behavior of  nsSVGPatternFrame::PaintPattern in FF 49, should be very safe I think.
[String changes made/needed]:N/A

I am still working on the patch for nightly. The test case in Part 3 fails occasionally.

That test case might fail even before Bug 1258510 landed. The symptom is that reftest can not always catch a snapshot after pattern image decoded completely, so blank image is captured sometimes. It's a reftest only failure.

The branch patch for 52/53/54 rollback 49's behavior and work correctly on those branch, so I think we should uplift it first. I will keep working on better solution in 55
Attachment #8849441 - Flags: approval-mozilla-release?
Attachment #8849441 - Flags: approval-mozilla-beta?
Attachment #8849441 - Flags: approval-mozilla-aurora?
(In reply to C.J. Ku[:cjku](UTC+8) from comment #62)
> That test case might fail even before Bug 1258510 landed. The symptom is
> that reftest can not always catch a snapshot after pattern image decoded
> completely, so blank image is captured sometimes. It's a reftest only
> failure.

This is the main problem that returning DrawResult's is supposed to solve. We can look at the implementation in nsImageFrame.cpp to see how it works. When nsDisplayImage::Paint Draw()'s the image it calls nsDisplayItemGenericImageGeometry::UpdateDrawResult with the DrawResult it got from the Draw call. nsDisplayImage overrides AllocateGeometry to return an nsDisplayItemGenericImageGeometry. And then in nsDisplayImage::ComputeInvalidationRegion we invalidate a region (so that we try to draw the image again) if we are sync decoding images (reftests sync decode images, regular painting doesn't) and the geometry tells us that we should (it figures this out based on the DrawResults we have passed it). I'm not sure if this can be adapted to svg patterns.
Comment on attachment 8849441 [details] [diff] [review]
Patch for 54/53/52

As this isn't a regression in 52 I'm going to say no for an uplift to release.  It might be worth considering for 52 ESR though.
Attachment #8849441 - Flags: approval-mozilla-release?
Attachment #8849441 - Flags: approval-mozilla-release-
Attachment #8849441 - Flags: approval-mozilla-esr52?
status-firefox52: affected → wontfix
(Assignee)

Comment 65

7 months ago
(In reply to Timothy Nikkel (:tnikkel) from comment #63)
> This is the main problem that returning DrawResult's is supposed to solve.
> We can look at the implementation in nsImageFrame.cpp to see how it works.
> When nsDisplayImage::Paint Draw()'s the image it calls
> nsDisplayItemGenericImageGeometry::UpdateDrawResult with the DrawResult it
> got from the Draw call. nsDisplayImage overrides AllocateGeometry to return
> an nsDisplayItemGenericImageGeometry. And then in
> nsDisplayImage::ComputeInvalidationRegion we invalidate a region (so that we
> try to draw the image again) if we are sync decoding images (reftests sync
> decode images, regular painting doesn't) and the geometry tells us that we
If aBuilder->ShouldSyncDecodeImages() return true, then we are running reftest. Is that correct? 
> should (it figures this out based on the DrawResults we have passed it). I'm
> not sure if this can be adapted to svg patterns.
Hmm.. svg pattern does the same thing, like image frame. I also trace how FrameLayerBuilder use ComputeInvalidationRegion.
I think it might because of nsSVGPatternFrame is not display directly. 
BTW, thanks for this sharing, I will look deeper.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> If aBuilder->ShouldSyncDecodeImages() return true, then we are running
> reftest. Is that correct? 

Correct.

Although it is a bit more complicated than that. We still paint to the screen normally during reftests, and those paints don't have ShouldSyncDecodeImages return true. But when the reftest harness takes it's snapshot (using drawWindow) it passes the sync decode flag so then ShouldSyncDecodeImages is true.
(Assignee)

Comment 67

7 months ago
(In reply to Timothy Nikkel (:tnikkel) from comment #66)
> (In reply to C.J. Ku[:cjku](UTC+8) from comment #65)
> > If aBuilder->ShouldSyncDecodeImages() return true, then we are running
> > reftest. Is that correct? 
> 
> Correct.
> 
> Although it is a bit more complicated than that. We still paint to the
> screen normally during reftests, and those paints don't have
> ShouldSyncDecodeImages return true. But when the reftest harness takes it's
> snapshot (using drawWindow) it passes the sync decode flag so then
> ShouldSyncDecodeImages is true.

I force nsDisplaySVGGeometry::Paint and nsDisplayImage::Paint returning DrawResult::NOT_READY
void nsDisplaySVGGeometry::Paint()
{
  //...
  nsDisplayItemGenericImageGeometry::UpdateDrawResult(this, DrawResult::NOT_READY);
}

Run the test:
./mach reftest layout/reftests/svg/pattern-big-image.html 

In theory, reftest should keep waiting. But in reality, the test is finished in just several seconds.
(In reply to C.J. Ku[:cjku](UTC+8) from comment #67)
> I force nsDisplaySVGGeometry::Paint and nsDisplayImage::Paint returning
> DrawResult::NOT_READY
> void nsDisplaySVGGeometry::Paint()
> {
>   //...
>   nsDisplayItemGenericImageGeometry::UpdateDrawResult(this,
> DrawResult::NOT_READY);
> }
> 
> Run the test:
> ./mach reftest layout/reftests/svg/pattern-big-image.html 
> 
> In theory, reftest should keep waiting. But in reality, the test is finished
> in just several seconds.

What I described (DrawResult's, UpdateDrawResult, etc) doesn't affect what the reftest harness will wait for. The purpose is so that when the reftest harness takes a snapshot and pass the sync decode flag that we will actually try to redraw the images. Because we don't redraw anything unless it was invalidated. If the image decode hasn't generated any invalidations then we won't redraw the image, and thus there will be no opportunity to even sync decode the image (we ask for sync decoding in the Draw call).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8850888 - Flags: review?(tnikkel)
Attachment #8850888 - Flags: review?(jwatt)
(Assignee)

Updated

7 months ago
Attachment #8850888 - Flags: review?(tnikkel)
Attachment #8850888 - Flags: review?(jwatt)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8850888 - Flags: review?(tnikkel)
Attachment #8850888 - Flags: review?(jwatt)
Attachment #8850917 - Flags: review?(tnikkel)
Attachment #8850917 - Flags: review?(jwatt)
Comment on attachment 8849441 [details] [diff] [review]
Patch for 54/53/52

fix svg regression in aurora/beta
Attachment #8849441 - Flags: approval-mozilla-beta?
Attachment #8849441 - Flags: approval-mozilla-beta+
Attachment #8849441 - Flags: approval-mozilla-aurora?
Attachment #8849441 - Flags: approval-mozilla-aurora+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 79

7 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/36f165ceceba
https://hg.mozilla.org/releases/mozilla-beta/rev/cbb7501f39ce
status-firefox53: affected → fixed
status-firefox54: affected → fixed

Comment 80

7 months ago
mozreview-review
Comment on attachment 8850888 [details]
Bug 1345853 - Part 3. Pass sync docode flag down to nsSVGDisplayableFrame::PaintSVG.

https://reviewboard.mozilla.org/r/123402/#review126018
Attachment #8850888 - Flags: review?(tnikkel) → review+

Comment 81

7 months ago
mozreview-review
Comment on attachment 8850917 [details]
Bug 1345853 - Part 4. Pass sync decode flag down to nsSVGPatternFrame::PaintPattern.

https://reviewboard.mozilla.org/r/123430/#review126020
Attachment #8850917 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/releases/mozilla-aurora/rev/12a4f938b5aa
(Assignee)

Updated

7 months ago
Blocks: 1350793
Setting qe-verify- based on C.J. Ku's assessment on manual testing needs (see Comment 62), but this would be a good candidate for the community.
QA Whiteboard: [good first verify]
Flags: qe-verify-
Blocks: 703806
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 92

7 months ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97c9e04af0f1
Part 1. Pass DrawResult from nsSVGPatternFrame::PaintPattern back to nsDisplaySVGGeometry::Paint. r=mstange,tnikkel
https://hg.mozilla.org/integration/autoland/rev/4b075674f6f5
Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/87c961bde81e
Part 3. Pass sync docode flag down to nsSVGDisplayableFrame::PaintSVG. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/7427352abc97
Part 4. Pass sync decode flag down to nsSVGPatternFrame::PaintPattern. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/67166a1e4ebc
Part 5. Reftest. r=tnikkel

Comment 93

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/97c9e04af0f1
https://hg.mozilla.org/mozilla-central/rev/4b075674f6f5
https://hg.mozilla.org/mozilla-central/rev/87c961bde81e
https://hg.mozilla.org/mozilla-central/rev/7427352abc97
https://hg.mozilla.org/mozilla-central/rev/67166a1e4ebc
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED

Comment 94

7 months ago
mozreview-review
Comment on attachment 8845776 [details]
Bug 1345853 - Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS

https://reviewboard.mozilla.org/r/118928/#review126990

::: commit-message-263fd:1
(Diff revision 13)
> +Bug 1345853 - Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS

Perhaps indicate that this is in SVG code, and what the problem is. Maybe "Ignore SVG under singular matrices when painting instead of returning imagelib failure codes". This is helpful for people looking at commit messages in the larger context of all commits going into the repository, such as when looking at regression ranges. It gives a better idea of whether they can ignore that change or need to examine it in more detail.

And the longer description doesn't need to read like "he said, she said". ;) I doubt tn wrote his bugzilla comment with the intent of making it a good clean commit message. Maybe use something like:

"DrawResult is only used to indicate the result of imagelib drawing operations. Currently we are improperly returning DrawImage error codes from some parts of the SVG code to indicate non-imagelib failures. We need to stop doing that since it could cause the code that acts on the returned DrawResult values to act incorrectly. In fact we don't even want to return an error code when we encounter SVG under a singular matrix, we just want to silently ignore it."
Comment on attachment 8849441 [details] [diff] [review]
Patch for 54/53/52

fix svg regression, esr52+
Attachment #8849441 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Needs rebasing for ESR52.
Flags: needinfo?(cku)
(Assignee)

Comment 97

7 months ago
Created attachment 8852756 [details] [diff] [review]
Patch for esr52
Flags: needinfo?(cku)
(Assignee)

Comment 98

7 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #96)
> Needs rebasing for ESR52.
Please pick up "Patch for esr52" attachment 8852756 [details] [diff] [review]
(Assignee)

Updated

7 months ago
Attachment #8850888 - Flags: review?(jwatt)
Attachment #8850917 - Flags: review?(jwatt)
(Assignee)

Updated

7 months ago
See Also: → bug 1351440

Comment 99

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/5fa27dc4c4a3
status-firefox-esr52: affected → fixed

Updated

5 months ago
Duplicate of this bug: 1365622
You need to log in before you can comment on or make changes to this bug.