Closed Bug 1345853 Opened 9 years ago Closed 9 years ago

SVG image pattern not visible in Firefox

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: MartinParenteau1963, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(8 files, 2 obsolete files)

Reporter's testcase
795 bytes, text/html
Details
Bug 1345853 - Part 1. Pass DrawResult from nsSVGPatternFrame::PaintPattern back to nsDisplaySVGGeometry::Paint.
59 bytes, text/x-review-board-request
mstange
: review+
tnikkel
: review+
Details
Bug 1345853 - Part 2. If the transform matrix is singular, return DrawResult::SUCCESS, instead of DrawResult::BAD_ARGS
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1345853 - Part 5. Reftest.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Patch for 54/53/52
4.28 KB, patch
u459114
: review+
jcristau
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta+
jcristau
: approval-mozilla-release-
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review
Bug 1345853 - Part 3. Pass sync docode flag down to nsSVGDisplayableFrame::PaintSVG.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1345853 - Part 4. Pass sync decode flag down to nsSVGPatternFrame::PaintPattern.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Patch for esr52
2.77 KB, patch
Details | Diff | Splinter Review
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
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
Attached file Reporter's testcase —
interesting, I will look into it
Assignee: nobody → cku
Flags: needinfo?(cku)
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.
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.
Attached patch Patch for 54/53/52 (obsolete) — — Splinter Review
Attachment #8845773 - Flags: review?(tnikkel)
Attachment #8845773 - Flags: review?(mstange)
Attachment #8845776 - Flags: review?(tnikkel)
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 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+
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; + }
Please add a reftest for this too.
Flags: needinfo?(cku)
Flags: in-testsuite?
(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.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #27) > Please add a reftest for this too. Yeap. Done.
Flags: needinfo?(cku)
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 on attachment 8847744 [details] Bug 1345853 - Part 5. Reftest. https://reviewboard.mozilla.org/r/120668/#review122892
Attachment #8847744 - Flags: review?(tnikkel) → 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+
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+
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
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.
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)
Comment on attachment 8845799 [details] [diff] [review] Patch for 54/53/52 Review of attachment 8845799 [details] [diff] [review]: ----------------------------------------------------------------- ｃａｎｃｅｌ　ｒｅｑｕｅｓｔ　ｓｉｎｃｅ　ｔｈｅ　ｐａｔｃｈ　ｉｎ　ｎｉｇｈｔｌｙ　ｈａｄ　ｂｅｅｎ　ｂａｃｋｏｕｔ
Attachment #8845799 - Flags: approval-mozilla-esr52?
Attachment #8845799 - Flags: approval-mozilla-beta?
Attachment #8845799 - Flags: approval-mozilla-aurora?
Attachment #8845799 - Flags: approval-mozilla-release?
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
Resolution: FIXED → ---
Attached patch Patch for 54/53/52 (obsolete) — — Splinter Review
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+
Attachment #8849192 - Attachment is obsolete: true
Attachment #8849441 - Flags: review+
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?
(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.
(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).
Attachment #8850888 - Flags: review?(tnikkel)
Attachment #8850888 - Flags: review?(jwatt)
Attachment #8850888 - Flags: review?(tnikkel)
Attachment #8850888 - Flags: review?(jwatt)
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 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 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+
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-
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 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)
Attached patch Patch for esr52 — — Splinter Review
Flags: needinfo?(cku)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #96) > Needs rebasing for ESR52. Please pick up "Patch for esr52" attachment 8852756 [details] [diff] [review]
Attachment #8850888 - Flags: review?(jwatt)
Attachment #8850917 - Flags: review?(jwatt)
See Also: → 1351440
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: