Closed Bug 1356179 Opened 7 years ago Closed 7 years ago

SVG rendering: Thin lines not displayed

Categories

(Core :: Layout, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- fixed
firefox53 - wontfix
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: mnietfeld, Assigned: u459114)

References

Details

(Keywords: regression)

Attachments

(9 files)

Attached 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
Closed: 7 years ago
Resolution: --- → INVALID
you might also want to consider vector-effects="non-scaling-stroke"
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.
(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.
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/).
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.
Sorry, that should be Firefox >= v52 and not Firefox > v52.
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)
This shows the SVG in Chrome on Ubuntu16 at 100% / standard zoom. This is how it should look like.
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.
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.
@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
@Jonathan This is also reproducible on Windows
Blocks: 1299715
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Component: Untriaged → Layout
Product: Firefox → Core
Assignee: nobody → cku
Flags: needinfo?(cku)
It's the third patch in Bug 1299715 cause that problem. I will fix it.
Attached image Simple version
Hmm.. the size of clip-path became zero becasue of rounding in NSRectToSnappedRect
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
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+
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 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+
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
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
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
https://hg.mozilla.org/mozilla-central/rev/6b896faa04a5
https://hg.mozilla.org/mozilla-central/rev/cd1dbc4bf4a8
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(cku)
Should probably seek approval for this to land on aurora/beta since it's a recent regression.
Flags: needinfo?(cku)
[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.
Attached patch Part 2 for betaSplinter Review
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+
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.