Bottom edge of SVG cut off

VERIFIED FIXED in Firefox 34

Status

()

Core
ImageLib
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Alice0775 White, Assigned: seth)

Tracking

({regression})

28 Branch
mozilla34
x86_64
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 unaffected, firefox28+ wontfix, firefox29+ wontfix, firefox30- wontfix, firefox31 wontfix, firefox32 wontfix, firefox33 wontfix, firefox34 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8337137 [details]
Screenshot Aurora27.0a2 and Nightly28.0a1

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/dbf94e314cde
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131122030202

When I tested Bug 942174, I found a plroblem  in Nightly280a1

Steps To Reproduce:
1. Open attachment 8336827 [details] of Bug 942174

Actual Results:
Bottom edge of SVG cut off
(Reporter)

Comment 1

4 years ago
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/4c2e31c6d4ef
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118182715
Bad:
http://hg.mozilla.org/mozilla-central/rev/5236947f9090
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118183211
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c2e31c6d4ef&tochange=5236947f9090

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3d3a8f13aa66
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131117233137
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/99108bac6f2d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118003137
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3d3a8f13aa66&tochange=99108bac6f2d

Regressed by: Bug 923341
Blocks: 923341
tracking-firefox28: --- → ?
Component: Layout → ImageLib
(Reporter)

Updated

4 years ago
OS: Windows 7 → All
(Assignee)

Updated

4 years ago
Assignee: nobody → seth
(Assignee)

Comment 2

4 years ago
Taking this but I'm not starting work immediately. Matt, if it's clear to you what's going on here, feel free to jump in.

Updated

4 years ago
status-firefox27: --- → unaffected
status-firefox28: --- → affected
tracking-firefox28: ? → +
Seth is this in your queue for the next few weeks?  It's a new regression in FF28 so it would be great to get something landed to fix this before ever shipping it to users.
Flags: needinfo?(seth)
status-firefox29: --- → affected
status-firefox30: --- → affected
(Assignee)

Comment 4

4 years ago
I'll make a note to take a look at it on Friday. I just finished a big chunk of feature work so now I have some time for fixing bugs and regressions.
Flags: needinfo?(seth)
Unfortunately we're out of time for FF28 cycle, let's see if this can get into Aurora 29 in the coming week or so.
status-firefox28: affected → wontfix
tracking-firefox29: --- → +
tracking-firefox30: --- → +
Seth, did you find the time to work on this? We are in middle of the beta cycle for 29. Thanks
Flags: needinfo?(seth)
Untracking for future releases, we can take a low risk uplift when ready but there's no major user complaints coming in so we're not going to apply a ton of pressure on you to get this done above other things.
status-firefox29: affected → wontfix
status-firefox31: --- → affected
tracking-firefox30: + → -
(Assignee)

Comment 8

4 years ago
I should probably post in this bug more often. I'm actually working on this now.
Flags: needinfo?(seth)
(Assignee)

Comment 9

4 years ago
Note that we're probably not getting many user complaints about this because it's actually kinda hard to reproduce, at least from what I've seen so far. On both high-DPI and low-DPI machines, I only observe it when the image in the STR is zoomed out as far as possible. Definitely not worth tracking based on what we've seen so far.
(Assignee)

Comment 10

4 years ago
Created attachment 8409110 [details]
Self-contained, simplified test-case.

This is a simplified testcase that demonstrates the problem when you zoom out as far as possible.

I could've simplified it a bit further by removing shape-rendering="crispEdges", but while it's still reproducible that way it strains your eyes a bit too much for my taste.
(Assignee)

Updated

3 years ago
Depends on: 1037713
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1004067
(Assignee)

Comment 12

3 years ago
I marked bug 1004067 as a duplicate of this bug. If anyone in the future is debugging a regression of this issue, though, it's worth noting that bug 1004067 also has a very good testcase.
(Assignee)

Comment 13

3 years ago
Created attachment 8463632 [details] [diff] [review]
Add reftests

This patch adds reftests for this issue. They're marked as random, here, since
after all they don't actually pass right now. The reftest patch for bug 1043560
will remove the random annotation.
Attachment #8463632 - Flags: review?(dholbert)
(Assignee)

Comment 14

3 years ago
(To be clear, bug 1043560 resolves this issue.)
Comment on attachment 8463632 [details] [diff] [review]
Add reftests

>diff --git a/image/test/reftest/downscaling/downscale-svg-1a-ref.html b/image/test/reftest/downscaling/downscale-svg-1a-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/downscaling/downscale-svg-1a-ref.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html>
>+<body style="margin: 0px">
>+  <img src="downscale-svg-1-1.0x.png" style="display: block">
>+</body>
>+</html>

Do we really need to use hardcoded PNG in the reference case here?  I don't think we do... I'll bet we could instead use <embed> with the same SVG content -- so, rendering the same SVG, but bypassing the imagelib layer entirely. (so that the reference case doesn't hit this bug, and we're still effectively testing this)

This seems to work for me, though I haven't tried it in the reftest harness:
  <embed src="black-border-rect.svg" style="display: block; width: 80px">

Unless I'm missing something, I'd rather we use this ^ instead of the hardcoded PNGs.  It should function as a better reference case than the PNGs, since it avoids hitting this bug itself due to avoiding the imagelib layer, and it should still be a pixel-perfect rendering (so hopefully minimal/no fuzziness annotations will be needed in the reftest.list file).

(I think you'd want to replace "width: 80px" with 40px, 24px, and 8px in the other reference cases, for zoom-factors 0.5, 0.3, and 0.1.)

>diff --git a/image/test/reftest/downscaling/downscale-svg-1a.html b/image/test/reftest/downscaling/downscale-svg-1a.html
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/downscaling/downscale-svg-1a.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html reftest-zoom="1.0">
>+<body style="margin: 0px">
>+  <div style="width: 80px; height: 80px; background: url(black-border-rect.svg) no-repeat">
>+</body>

This is missing a closing </div> (in each of the testcases in this patch, I think).

(The parser fills it in, but might as well include it, since you've got </body></html>.)

Also, these tests should be named "-1", "-2", "-3", "-4", instead of -1a.html, -1b.html, etc, since they don't share a reference case.

This (sort of) matters because we *mostly* honor the invariant that if you're viewing e.g. "downscale-svg-1c.html", you can find the reference case by adding "-ref" after the number (and stripping off any variant suffix like "a","b","c"). No need to look at the reference case -- the naming is predictable.  But the naming here violates that assumption (right now).  So, probably better to just be consistent with the established practice and give these tests different numbers.

(I know the tests are extremely related, which makes them feel like they should share a number.  But they do share the rest of their prefix, after all, which does indicate their relatedness.)

>+# RUN TESTS NOT AFFECTED BY HIGH QUALITY DOWNSCALING:
>+# =================================================
>+random == downscale-svg-1a.html downscale-svg-1a-ref.html
>+fuzzy(93,256) random == downscale-svg-1b.html downscale-svg-1b-ref.html
>+fuzzy(93,144) random == downscale-svg-1c.html downscale-svg-1c-ref.html
>+fuzzy(118,38) random == downscale-svg-1d.html downscale-svg-1d-ref.html
>+
> # RUN TESTS WITH HIGH QUALITY DOWNSCALING DISABLED:
> # =================================================

Nit: the "====" underlining needs two more "=" characters.

Also, "fuzzy(...) random" doesn't really make sense -- if we do need fuzzy annotations after these tests are made to be passing (and I'm hoping we won't, with the embed tweak that I mentioned above), I'd rather add it in the patch that marks the tests as passing over in bug 1043560.  (So, we go from a random (or failing) state to passing-and-fuzzy, instead of starting out random-and-fuzzy, which isn't asserting anything useful.)

Also, since <embed>/SVG rendering behavior is more predictable and platform-independent than PNG-rendering behavior, I'm hoping we can label these as either "fails" or already-passing up-front, instead of "random".  (IIUC, the randomness is due to platform-specific differences in the PNG rendering (?)).  This would function as a good sanity-check (on TBPL) that bug 1043560 *is* actually fixing things here.  Even if some of the tests have platform-specific differences and are easier to mark as 'random' for now, I'm hoping maybe one of the tests fails everywhere -- if so, it'd be great to have at least that one test start out labeled as "fails". Not a huge deal if you don't get to this part, or if it's too complicated, though.
Attachment #8463632 - Flags: review?(dholbert) → review-
> No need to look at the reference case -- the naming is predictable.

(Sorry, I meant to say "No need to look at the reftest.list file (to find out the name of the reference case)")
(Assignee)

Comment 17

3 years ago
Thanks for the review!

I'll address these issues. Using <embed> didn't occur to me here; I'll give it a shot.
(Assignee)

Comment 18

3 years ago
Created attachment 8464172 [details] [diff] [review]
Add reftests

All the review comments are addressed. <embed> was a big improvement!

I've marked all the tests as fail, but I need to push a try job to make sure that's actually true everywhere. I may have to go back and mark some of them random.
Attachment #8464172 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8463632 - Attachment is obsolete: true
Comment on attachment 8464172 [details] [diff] [review]
Add reftests

Looks good - thanks!
Attachment #8464172 - Flags: review?(dholbert) → review+
(Assignee)

Comment 20

3 years ago
Created attachment 8465738 [details] [diff] [review]
Add reftests

As discussed on IRC, I added some new tests so that we now cover all of the downscaling zoom levels you can get via the Firefox UI.
(Assignee)

Updated

3 years ago
Attachment #8464172 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Pushed the tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9382edf1265
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/d9382edf1265
(Assignee)

Comment 23

3 years ago
This was fixed by bug 1037713.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-firefox30: affected → wontfix
status-firefox31: affected → wontfix
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
Keywords: leave-open
Target Milestone: --- → mozilla34
QA Whiteboard: [good first verify]
Reproduced on Nightly 2013-11-22 with STR from comment 0 and Windows 7 x64.

Verified as fixed with Firefox 34 beta 10 (Build ID: 20141117202603) on Windows 7 x64.
Thank you Ikram! Marking as verified based on your results.
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
QA Whiteboard: [good first verify] → [good first verify][bugday-20141119]
You need to log in before you can comment on or make changes to this bug.