SVG rendering: Thin lines not displayed

RESOLVED FIXED in Firefox -esr52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mnietfeld, Assigned: u459114)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53- wontfix, firefox54+ fixed, firefox55 fixed)

Details

Attachments

(9 attachments)

Reporter

Description

2 years ago
Posted image invisible_bonds.svg
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.
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
Last Resolved: 2 years ago
Resolution: --- → INVALID
you might also want to consider vector-effects="non-scaling-stroke"
Reporter

Comment 3

2 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

2 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.
Duplicate of this bug: 1363032
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

2 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

2 years ago
Sorry, that should be Firefox >= v52 and not Firefox > v52.
Reporter

Comment 9

2 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.
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.
C.J., according to comment 9 you're changes for bug 1299715 may have broken something.
Flags: needinfo?(cku)
Reporter

Comment 12

2 years ago
This shows the SVG in Chrome on Ubuntu16 at 100% / standard zoom. This is how it should look like.
Reporter

Comment 13

2 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

2 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

2 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

2 years ago
@Jonathan This is also reproducible on Windows
Blocks: 1299715
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Component: Untriaged → Layout
Product: Firefox → Core
Assignee

Updated

2 years ago
Assignee: nobody → cku
Flags: needinfo?(cku)
Assignee

Comment 17

2 years ago
It's the third patch in Bug 1299715 cause that problem. I will fix it.
Assignee

Comment 18

2 years ago
Posted image Simple version
Assignee

Comment 19

2 years ago
Hmm.. the size of clip-path became zero becasue of rounding in NSRectToSnappedRect
Assignee

Comment 20

2 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.
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)
Assignee

Updated

2 years ago
Attachment #8866246 - Flags: review?(mstange)
Attachment #8866247 - Flags: review?(mstange)
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 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

2 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 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

2 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 37

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b896faa04a5
https://hg.mozilla.org/mozilla-central/rev/cd1dbc4bf4a8
Status: NEW → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Flags: needinfo?(cku)
Should probably seek approval for this to land on aurora/beta since it's a recent regression.
Flags: needinfo?(cku)
Assignee

Comment 45

2 years ago
[Tracking Requested - why for this release]:
Flags: needinfo?(cku)
Track 54+ as the SVG issue and track 53- as we won't fix this in 53.
Assignee

Comment 47

2 years ago
Comment hidden (spam)
Assignee

Comment 50

2 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?
Flags: in-testsuite+
Keywords: regression
Small patch - worth considering for ESR52 too?
Flags: needinfo?(cku)
Personally, I(In reply to Ryan VanderMeulen [:RyanVM] from comment #51)
> Small patch - worth considering for ESR52 too?

I'd say yes.
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

2 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 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+
Attachment #8867625 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(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.