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

VERIFIED FIXED in Firefox 25

Status

()

Core
Graphics
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: seth)

Tracking

({regression, verifyme})

Trunk
mozilla27
regression, verifyme
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 wontfix, firefox25+ verified, firefox26+ verified, firefox27 verified, firefox-esr24 wontfix)

Details

(URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 809989 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 809990 [details]
Screenshot_2013-06-15_Nightly_BAD_BLUE.png
(Reporter)

Comment 2

5 years ago
Created attachment 809992 [details]
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.
Flags: needinfo?(seth)
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
Flags: needinfo?(seth)
(Assignee)

Updated

5 years ago
Assignee: nobody → seth
(Assignee)

Comment 5

5 years ago
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.
(Reporter)

Comment 6

5 years ago
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. :)
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
(Assignee)

Comment 7

5 years ago
Created attachment 810352 [details] [diff] [review]
(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
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 810884 [details] [diff] [review]
(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.
Attachment #810884 - Flags: review?(dholbert)
(Assignee)

Comment 10

5 years ago
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+
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. :))
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 811372 [details] [diff] [review]
(Part 2) - Tests for zoom with the svg:image element.

Addressed review concerns.
(Assignee)

Updated

5 years ago
Attachment #810884 - Attachment is obsolete: true
(Assignee)

Comment 16

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

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

Comment 17

5 years ago
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.
(Assignee)

Comment 20

4 years ago
One more attempt at avoiding fuzzy:

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

Comment 21

4 years ago
Created attachment 813413 [details] [diff] [review]
(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.
(Assignee)

Updated

4 years ago
Attachment #811372 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c3a4efc8623c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Comment 24

4 years ago
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.
Attachment #810352 - Flags: approval-mozilla-beta?
Attachment #810352 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

4 years ago
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.)
Attachment #813413 - Flags: approval-mozilla-beta?
Attachment #813413 - Flags: approval-mozilla-aurora?
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.
Attachment #810352 - Flags: approval-mozilla-beta?
Attachment #810352 - Flags: approval-mozilla-beta+
Attachment #810352 - Flags: approval-mozilla-aurora?
Attachment #810352 - Flags: approval-mozilla-aurora+
Attachment #813413 - Flags: approval-mozilla-beta?
Attachment #813413 - Flags: approval-mozilla-beta+
Attachment #813413 - Flags: approval-mozilla-aurora?
Attachment #813413 - Flags: approval-mozilla-aurora+
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.
status-firefox27: affected → fixed
tracking-firefox25: ? → +
tracking-firefox26: ? → +

Updated

4 years ago
Keywords: verifyme
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?
Flags: needinfo?(cpeterson)
(Reporter)

Comment 30

4 years ago
VERIFIED FIXED in Aurora 26.0a2 (2013-10-22) and Beta 25.0b10 (2013-10-21).
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
status-firefox26: fixed → verified
status-firefox27: fixed → verified
(Reporter)

Updated

4 years ago
Flags: needinfo?(cpeterson)

Updated

4 years ago
Depends on: 933057
You need to log in before you can comment on or make changes to this bug.