Closed
Bug 1356179
Opened 7 years ago
Closed 7 years ago
SVG rendering: Thin lines not displayed
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: mnietfeld, Assigned: u459114)
References
Details
(Keywords: regression)
Attachments
(9 files)
123.04 KB,
image/svg+xml
|
Details | |
21.66 KB,
image/png
|
Details | |
43.34 KB,
image/png
|
Details | |
46.43 KB,
image/png
|
Details | |
53.08 KB,
image/png
|
Details | |
815 bytes,
image/svg+xml
|
Details | |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
jcristau
:
approval-mozilla-esr52+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
3.20 KB,
patch
|
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20170303013005 Steps to reproduce: I can reproduce through the following steps: Open Firefox 52, click Open File, open attached SVG (invisible_bonds.svg). Actual results: Some vertical lines are not displayed in 100% scale. This changes upon modifying the zoom scale with Strg-+ or -. When changing zoom scale, some lines appear, some disappear. Only happens in scales < 133%, in scales above the graphics apparently is "large" enough to be displayed correctly. Expected results: All hexagons are closed, so all lines in all zoom scales should be displayed. We produce scientific journals and omitting bonds in chemical structures changes the scientific meaning of significantly.
Comment 1•7 years ago
|
||
draw fatter lines or set shape-rendering="crispEdges". There's nothing that says lines are guaranteed to exist if you draw them too thin at some point they will be too thin to render.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Comment 2•7 years ago
|
||
you might also want to consider vector-effects="non-scaling-stroke"
Reporter | ||
Comment 3•7 years ago
|
||
We have 45000+ legacy SVGs on our website that have been displayed correctly so far in Chrome, Firefox (until version 52) and even IE. Drawing lines fatter is not an option for us. Setting shape-rendering or vector-effect on paths did not work either. I wouldn't consider this bug resolved, on the contrary: The fact that the lines have been displayed correctly in Firefox versions <= 51 to me looks like some kind of issue was introduced in 52.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Robert Longson from comment #2) > you might also want to consider vector-effects="non-scaling-stroke" The workarounds you mentioned unfortunately do not work. Please reopen or clone this bug, this issue is neither invalid nor resolved. It definitely is a defect as the svg used to be displayed correctly in FF <= 51.
Comment 6•7 years ago
|
||
I still don't think there's a bug here. If you do, your next step would be to determine a regression range by using moz-regression (http://mozilla.github.io/mozregression/).
Comment 7•7 years ago
|
||
I filed the duplicate bug 1363032. I have tried all the workarounds suggested in this thread, and have also tried the various image-rendering options, but none of them resolve this issue. As described by Markus, the last version where this was working correctly was version 51 (specifically 51.0b9). The defective behaviour was introduced in v52. The bonds render correctly in all other major, modern browsers at all sizes, and not rendering chemical bonds correctly renders Firefox > v52 unusable for our users.
Comment 8•7 years ago
|
||
Sorry, that should be Firefox >= v52 and not Firefox > v52.
Reporter | ||
Comment 9•7 years ago
|
||
Using mozregression, it says the defective behaviour was introduced between Nightly Builds 2016-10-12 (good) and 2016-10-13 (bad). Here's the output: 7:14.06 INFO: Oh noes, no (more) inbound revisions :( 7:14.06 INFO: Last good revision: 8df85989b975c0d32d9a0ad78066576b803cf858 7:14.06 INFO: First bad revision: 4e2908794b6c9859048d81c2cf598bfca89a8ea2 7:14.06 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8df85989b975c0d32d9a0ad78066576b803cf858&tochange=4e2908794b6c9859048d81c2cf598bfca89a8ea2 7:15.60 INFO: Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1299715 Can you confirm this, Rob? I agree with Rob: A chemical bond which is there in the svg but not rendered is not just about omitting a single little line. It changes the chemical structure completely and falsifies the scientific information. Therefore this issue is critical to us and we cannot use SVG or recommend Firefox to our users as long as this is not fixed.
Comment 10•7 years ago
|
||
Is this a Linux only problem? Can someone attach a screenshot of the broken rendering? On Mac the rendering seems identical to Chrome for me.
Comment 11•7 years ago
|
||
C.J., according to comment 9 you're changes for bug 1299715 may have broken something.
Flags: needinfo?(cku)
Reporter | ||
Comment 12•7 years ago
|
||
This shows the SVG in Chrome on Ubuntu16 at 100% / standard zoom. This is how it should look like.
Reporter | ||
Comment 13•7 years ago
|
||
This shows the SVG in Firefox 53 on Ubuntu 16 at 100% zoom. Note the missing bond in the hexagon in the upper right corner.
Reporter | ||
Comment 14•7 years ago
|
||
This shows the SVG in Firefox 53 on Ubuntu 16 at 110% zoom. Note that compared to the 100% display the formerly missing bond has reappeared and now two other bonds are missing.
Comment 15•7 years ago
|
||
@Markus I have confirmed that it is the same commit which introduced the defect for me: 22:23.17 INFO: Narrowed inbound regression window from [0ca22c46, 4e290879] (3 revisions) to [8df85989, 4e290879] (2 revisions) (~1 steps left) 22:23.18 INFO: Oh noes, no (more) inbound revisions :( 22:23.18 INFO: Last good revision: 8df85989b975c0d32d9a0ad78066576b803cf858 22:23.18 INFO: First bad revision: 4e2908794b6c9859048d81c2cf598bfca89a8ea2 22:23.18 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8df85989b975c0d32d9a0ad78066576b803cf858&tochange=4e2908794b6c9859048d81c2cf598bfca89a8ea2 22:24.58 INFO: Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1299715
Comment 16•7 years ago
|
||
@Jonathan This is also reproducible on Windows
Updated•7 years ago
|
Updated•7 years ago
|
Component: Untriaged → Layout
Product: Firefox → Core
Assignee | ||
Comment 17•7 years ago
|
||
It's the third patch in Bug 1299715 cause that problem. I will fix it.
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
Hmm.. the size of clip-path became zero becasue of rounding in NSRectToSnappedRect
Assignee | ||
Comment 20•7 years ago
|
||
before rounding left-top = {x = -0.0234222412, y = 75.5449829} right-bottom = {x = 601.341858, y = 76.3179932} after rounding: left-top = {x = , y = 76} right-bottom = {x = 601, y = 76} So the height of this clip-path become 0.
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
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) |
Attachment #8866246 -
Flags: review?(mstange)
Attachment #8866247 -
Flags: review?(mstange)
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8866246 [details] Bug 1356179 - Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. https://reviewboard.mozilla.org/r/137870/#review141116 Do we need this clip to actually clip drawing in some cases, or is it only used to limit the size of intermediate surfaces? If the latter, could you just use RoundOut?
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8866247 [details] Bug 1356179 - Part 2. Reftest for applying clip-path onto thin objects. https://reviewboard.mozilla.org/r/137872/#review141118
Attachment #8866247 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8866246 [details] Bug 1356179 - Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. https://reviewboard.mozilla.org/r/137870/#review141116 The latter.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8866246 [details] Bug 1356179 - Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. https://reviewboard.mozilla.org/r/137870/#review141430
Attachment #8866246 -
Flags: review?(mstange) → review+
Comment 35•7 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e9a6e426b1e Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. r=mstange https://hg.mozilla.org/integration/autoland/rev/b854f24a7e4c Part 2. Reftest for applying clip-path onto thin objects. r=mstange
Comment 36•7 years ago
|
||
backed out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=98270784&repo=autoland&lineNumber=9057
Flags: needinfo?(cku)
Comment 37•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0971ec3bae29 Backed out changeset b854f24a7e4c for reftest failures in mask-img-ref.html https://hg.mozilla.org/integration/autoland/rev/86754a7acc0e Backed out changeset 9e9a6e426b1e
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 42•7 years ago
|
||
Pushed by cku@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6b896faa04a5 Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. r=mstange https://hg.mozilla.org/integration/autoland/rev/cd1dbc4bf4a8 Part 2. Reftest for applying clip-path onto thin objects. r=mstange
Comment 43•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6b896faa04a5 https://hg.mozilla.org/mozilla-central/rev/cd1dbc4bf4a8
Status: NEW → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 44•7 years ago
|
||
Should probably seek approval for this to land on aurora/beta since it's a recent regression.
Flags: needinfo?(cku)
Assignee | ||
Comment 45•7 years ago
|
||
[Tracking Requested - why for this release]:
Assignee | ||
Comment 47•7 years ago
|
||
Comment hidden (spam) |
Assignee | ||
Comment 49•7 years ago
|
||
Try result on beta https://treeherder.mozilla.org/#/jobs?repo=try&revision=486fa29471713f2c65235f7be9c62804fc291003
Assignee | ||
Comment 50•7 years ago
|
||
Comment on attachment 8867625 [details] [diff] [review] Part 2 for beta Approval Request Comment [Feature/Bug causing the regression]: bug 1299715 [User impact if declined]: Clipped SVG element may disappear if the size of a clipped region is too narrow [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 1" [Is the change risky?]: no [Why is the change risky/not risky?]: pass manual and auto test [String changes made/needed]: N/A
Attachment #8867625 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox53:
--- → wontfix
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Flags: in-testsuite+
Keywords: regression
Comment 52•7 years ago
|
||
Personally, I(In reply to Ryan VanderMeulen [:RyanVM] from comment #51) > Small patch - worth considering for ESR52 too? I'd say yes.
Assignee | ||
Comment 53•7 years ago
|
||
try on esr52 https://treeherder.mozilla.org/#/jobs?repo=try&revision=09a7fe57d5da4307d78ff4f3aa0b0440fd4204ff
Comment 54•7 years ago
|
||
Comment on attachment 8867625 [details] [diff] [review] Part 2 for beta Fix a regression. Beta54+. Should be in 54 beta 9.
Attachment #8867625 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 55•7 years ago
|
||
Comment on attachment 8867625 [details] [diff] [review] Part 2 for beta [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: smal patch, fix an SVG regression User impact if declined: Clipped SVG element may disappear if the size of a clipped region is too narrow Fix Landed on Version:55 Risk to taking this patch (and alternatives if risky): Low rish easy patch, fix had been landed on nightly String or UUID changes made by this patch: NA
Flags: needinfo?(cku)
Attachment #8867625 -
Flags: approval-mozilla-esr52?
Comment 56•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f823cd086b3c https://hg.mozilla.org/releases/mozilla-beta/rev/00ee53863f3c
Comment 57•7 years ago
|
||
Comment on attachment 8866246 [details] Bug 1356179 - Part 1. Roundout clip region to make sure we have enough space for mask/clip-path painting. fix svg regression in 52, with associated test, esr52.2+
Attachment #8866246 -
Flags: approval-mozilla-esr52+
Updated•7 years ago
|
Attachment #8867625 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 58•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/66d8893f37f0 https://hg.mozilla.org/releases/mozilla-esr52/rev/f80a49c81a40
Comment 59•7 years ago
|
||
(In reply to C.J. Ku[:cjku](UTC+8) from comment #50) > [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 on 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
•