Closed Bug 920621 Opened 6 years ago Closed 6 years ago
Street Map's i D map editor's pop-up pie menu items are blank, on retina display or with full-page-zoom
99.33 KB, image/png
107.83 KB, image/png
113.20 KB, image/png
1.86 KB, patch
|Details | Diff | Splinter Review|
18.23 KB, patch
|Details | Diff | Splinter Review|
STR: 1. Load OpenStreetMap's iD map editor: http://www.openstreetmap.org/edit?editor=id#map=18/37.91720/-122.17657 2. Click on a line or point. RESULT: The pop-up pie menu items are blank white circles. They should contain tool icons. See the attached screenshots. This bug is a regression affecting Firefox 24-27, but the exact regression point is difficult to identify because of two overlapping bugs. In Nightly 24 build 2013-06-15, the menu items changed from tool icons to blue artifacts: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b197bed90a98&tochange=3d16d59c9317 In Aurora 24 build 2013-07-24, the menu items changed again from blue artifacts to blank circles: https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=e18d6a0ab17c&tochange=615758994f36
Seth, could these blank tooltips be a regression from your SVG bug 885939? Your fix was uplifted to Aurora on the same day these tooltips when blank.
Also, your SVG bug 600207 landed in Nightly build with this bug's first tooltip regression from the expected tool icons to blue artifacts. btw, I am using a Retina MBP.
Just took a look and this is an embedded SVG element rather than SVG-as-image, so the issue isn't the changes in VectorImage that those bugs focus on. I did change a small amount of code related to scaling in the main SVG rendering code, which is almost certainly the source of the problem. Should be able to narrow it down pretty quickly since that change only affected a few lines of code.
OK, actively working on this as of now. It's worth noting that the bug only appears in 2x/retina mode; if you have an external monitor on a Retina MBP, you can drag the window between the laptop screen and the external screen and see the controls appear and disappear.
Do you think the fix will be safe enough to uplift to Aurora 26 and Beta 25? This regression affects all Firefox release channels, but it must not be a commonly used feature or else someone would have reported this issue sooner than four months after the regression. :)
One change by the patch in bug 600207 was to stop correcting for full page zoom in nsSVGImageFrame::TransformContextForPainting for VectorImages, because we started handling that in VectorImage directly. However, bug 885939 changed how VectorImage's scaling worked to be closer to the old behavior. I didn't realize it, but we should've reverted that change to nsSVGImageFrame as part of bug 885939. I didn't realize it because it's apparently not that easy to trigger this bug. =) This patch essentially does a revert of the part of bug 600207 that affected nsSVGImageFrame, although it's not _literally_ a revert since the code changed a bit. More of a rebased revert. Holding off on review until I see try results, but locally everything seems ok. We also definitely need a test for this (which will be part 2) since obviously we have no coverage for this issue. I'll get that taken care of tomorrow. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=c962dd1a8a3a
(In reply to Chris Peterson (:cpeterson) from comment #6) > Do you think the fix will be safe enough to uplift to Aurora 26 and Beta 25? > This regression affects all Firefox release channels, but it must not be a > commonly used feature or else someone would have reported this issue sooner > than four months after the regression. :) I think it will be safe, since this is pretty much just a partial revert. Unless something else pops up, this is about as low-risk as it gets.
Here are the appropriate tests. The 'sprite' tests which use SVG images do not pass without part 1 applied, and do pass with it. I noticed that one of the tests needs a fuzzy due to what appears to be some filtering of the rendered SVG. This concerned me since bug 600207 was all around _preventing_ things like filtering and temporary surfaces for SVGs. However, I've stepped through the code and verified that this is not happening in nsSVGImageFrame, VectorImage, or DrawPixelSnapped. No temporary surface is being created and VectorImage is eliminating the scaling correctly. There also isn't a non-integer translation. I can only conclude at this point that whatever filtering appears to be occurring is happening somewhere in either RenderDocument or the layer system. This is puzzling, but I think it's outside the scope of this bug. Regardless, the tests show that part 1 fixes this bug, and I do not believe that part 1 introduces any regressions. IMO we should land part 1 and uplift it to beta if it passes review.
Attachment #810884 - Flags: review?(dholbert)
Comment on attachment 810352 [details] [diff] [review] (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.) I've satisfied myself that this patch is the right thing, so review away. =)
Attachment #810352 - Flags: review?(dholbert)
Comment on attachment 810352 [details] [diff] [review] (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.) (In reply to Seth Fowler [:seth] from comment #5) > It's worth noting that the bug only > appears in 2x/retina mode FWIW: the bug reproduces on my Linux laptop using full-page-zoom, too (which makes sense, given what the patch does, and also given the reftests' "reftest-zoom" usage). Anyway - patch looks correct to me, given that this is where this code used to be, and given that reducing raster-vs-vector special-case code (outside of imagelib) is a Good Thing.
Attachment #810352 - Flags: review?(dholbert) → review+
6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Summary: OpenStreetMap's iD map editor's pop-up pie menu items are blank → OpenStreetMap's iD map editor's pop-up pie menu items are blank, on retina display or with full-page-zoom
Comment on attachment 810884 [details] [diff] [review] (Part 2) - Tests for zoom with the svg:image element. >diff --git a/layout/reftests/svg/image/reftest.list b/layout/reftests/svg/image/reftest.list >+== image-svg-inline-sprite-zoom-out-01.html image-svg-inline-sprite-zoom-out-ref.html >+== image-svg-inline-sprite-zoom-out-02.html image-svg-inline-sprite-zoom-out-ref.html Please name these -01a/-01b/-ref, instead of -01/-02/-ref/ That'll allows us to come along and add additional tests to this family (optionally with their own reference cases) down the line, without there being any ambiguity about which test is supposed to match which reference case. (Similar for the other new tests.) >+++ b/layout/reftests/svg/image/image-svg-inline-sprite-zoom-out-ref.html >@@ -0,0 +1,8 @@ >+<html reftest-zoom="0.5"> It's better to avoid "reftest-zoom" in reference cases, when we don't need it (per the "reference cases should be dumb, static content" rule-of-thumb). So, if it's not too much trouble, just make the test's content already be correctly-sized to match the zoomed testcase. (probably worth setting "margin:0" on the <body> in the testcase & reference, to avoid having to hard-code the zoomed default margin amount) (Same for image-svg-inline-sprite-zoom-in-ref.html) >diff --git a/layout/reftests/svg/image/image-svg-inline-zoom-in-01.html b/layout/reftests/svg/image/image-svg-inline-zoom-in-01.html >new file mode 100644 >--- /dev/null >+++ b/layout/reftests/svg/image/image-svg-inline-zoom-in-01.html >@@ -0,0 +1,16 @@ >+<html reftest-zoom="1.5"> [...] >+ <!-- SVG with no intrinsic dimensions --> >+ <image xlink:href="../pass.svg" width="100%" height="100%"/> Don't use pass.svg as part of your reftest. pass.svg is itself a reference case, and it's awkward to have it also be part of testcases. I'd prefer that you "hg cp" pass.svg into this directory (or just create a new file), and name it "lime-no-dimensions.svg" or whatever you want to name it, and use that. r=me with the above
Attachment #810884 - Flags: review?(dholbert) → review+
(In reply to Seth Fowler [:seth] from comment #9) > IMO we should land part 1 and uplift > it to beta Agreed! (though I think we should uplift both parts -- part 2's tests will be how we convince ourselves that part 1 does actually fix it on the branches. :))
(In reply to Daniel Holbert [:dholbert] from comment #11) > FWIW: the bug reproduces on my Linux laptop using full-page-zoom, too (which > makes sense, given what the patch does, and also given the reftests' > "reftest-zoom" usage). Heh, sorry, that's totally true. I meant at default zoom. I think in part this is why this has gone unnoticed for so long.
Addressed review concerns.
Attachment #810884 - Attachment is obsolete: true
Final try job here: (Since inbound's closed anyway.) https://tbpl.mozilla.org/?tree=Try&rev=96b6f8dcf4fc
Hmm, one of the new tests failed. I may be reduced to fuzzy, but before I turn to that here's a try job with reftest-zoom restored on the references: https://tbpl.mozilla.org/?tree=Try&rev=ead4e69b4bb9
Is this issue specific to OpenStreetMap or do we expect it to impact other major web properties as well? This is w/r/t a tracking decision.
This bug will affect any web property that uses the SVG <image> element to render SVG content, with transforms in play. (and only when viewed on a retina display or when zoomed in.) So: should probably only affect web properties that use SVG heavily. This could easily also affect SVG-based graphing / charting libraries.
One more attempt at avoiding fuzzy: https://tbpl.mozilla.org/?tree=Try&rev=42c7d2dad620
Success! By adding |style="shape-rendering: crispEdges"| to all the SVGs, all of the fuzzy annotations can be removed! Looks like this is now ready to land.
Attachment #811372 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 810352 [details] [diff] [review] (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.) [Approval Request Comment] Bug caused by (feature/regressing bug #): 600207 User impact if declined: SVG images may render incorrectly or simply disappear in some circumstances. OpenStreeetMap is known to be affected. Testing completed (on m-c, etc.): On m-c. Risk to taking this patch (and alternatives if risky): Very low risk. This simply reverts part of the patch for bug 600207. String or IDL/UUID changes made by this patch: None.
Comment on attachment 813413 [details] [diff] [review] (Part 2) - Tests for zoom with the svg:image element. [Approval Request Comment] (See above. These are tests for the issue.)
Comment on attachment 810352 [details] [diff] [review] (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.) Looks like a low risk fix to take for a regression from FF24 so let's get this in.
Tracking since it's a regression from 24, and though we may not have a lot of reports there are quite a few retina displays out now so don't want to assume edge case. Worth taking to prevent wider effect.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0 Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0 Verified on Firefox 25 beta 10 (build ID: 20131021191948) and on latest Aurora (build ID: 20131021004002) with full-page-zoom and the STR provided in comment 0. The issue is no longer reproducing, the pop-up pie menu items are correctly displayed. Chris, can you or someone using a retina display please verify if this is also fixed on Hi-DPI using Firefox 25 beta 10 and latest aurora?
VERIFIED FIXED in Aurora 26.0a2 (2013-10-22) and Beta 25.0b10 (2013-10-21).
You need to log in before you can comment on or make changes to this bug.