920621 - OpenStreetMap's iD map editor's pop-up pie menu items are blank, on retina display or with full-page-zoom

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Screenshot_2013-06-14_Nightly_GOOD.png

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

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Screenshot_2013-06-15_Nightly_BAD_BLUE.png

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Screenshot_2013-07-24_Aurora_BAD_BLANK.png

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.

Comment 3

[Chris Peterson [:cpeterson]]

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Chris Peterson [:cpeterson]]

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. :)

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.)

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

Comment 8

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Tests for zoom with the svg:image element.

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.

Comment 10

[Seth Fowler [:seth] [:s2h]]

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. =)

Comment 11

[Daniel Holbert [: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.

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

(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. :))

Comment 14

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Tests for zoom with the svg:image element.

Addressed review concerns.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Final try job here: (Since inbound's closed anyway.)

https://tbpl.mozilla.org/?tree=Try&rev=96b6f8dcf4fc

Comment 17

[Seth Fowler [:seth] [:s2h]]

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

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Daniel Holbert [:dholbert]]

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.

Comment 20

[Seth Fowler [:seth] [:s2h]]

One more attempt at avoiding fuzzy:

https://tbpl.mozilla.org/?tree=Try&rev=42c7d2dad620

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Tests for zoom with the svg:image element.

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.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85cb12480a3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a4efc8623c

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c3a4efc8623c

Comment 24

[Seth Fowler [:seth] [:s2h]]

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 25

[Seth Fowler [:seth] [:s2h]]

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 26

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a2632a328b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/b20e12690dcb

https://hg.mozilla.org/releases/mozilla-beta/rev/2ec0ef324c38
https://hg.mozilla.org/releases/mozilla-beta/rev/5e76b2f77eb7

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

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?

Comment 30

[Chris Peterson [:cpeterson]]

VERIFIED FIXED in Aurora 26.0a2 (2013-10-22) and Beta 25.0b10 (2013-10-21).