1519144 - viewBox attribute on <svg> element can cause clickable area of <text> elements to be way bigger than it should be

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Alex Henrie]

Attachment: Example

When the viewBox attribute on an SVG causes 1 "pixel" to correspond to many device pixels, the area around <text> elements is inflated far beyond the text itself. See the attached example, which works correctly in Chrome.

This is a regression caused by the hacky fix for bug 884718. I don't really understand what the problem was that led to this hack, but it doesn't appear to be necessary any more: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a10b99d66623f57430f1288b734900b71e580f1

Comment 1

[Alex Henrie]

Attachment: Proposed patch

This patch removes the hack for bug 884718 and adds a test to ensure that the problem does not recur.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54868e6b40dda63eca6b8bc4e61eea49f6b118a3

Comment 2

[Alex Henrie]

Attachment: Proposed patch v2

I tweaked the test to hover above the text and then hover to the left of the text instead of hovering to the upper-left of the text. This makes the test a little more stringent, so no one will be tempted to "just inflate the width" or "just inflate the height" of the clickable area. I also fixed the charset declaration.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 9035846 [details] [diff] [review]
Proposed patch v2

You're going to need to fix the try failures.

Comment 4

[Alex Henrie]

(In reply to Robert Longson [:longsonr] from comment #3)

Comment on attachment 9035846 [details] [diff] [review]
Proposed patch v2

You're going to need to fix the try failures.

As far as I can see, all of the try failures are known intermittent problems unrelated to this change. Which specific tests are you looking at?

Comment 5

[Daniel Holbert [:dholbert]]

Some of the try failures have intermittent bugs linked on TreeHerder, but those are just "suggested/possible" known intermittent issues - they're not necessarily an instance of that exact intermittent issue. (And in e.g. cases where your Try failure is a TEST-UNEXPECTED-FAIL whereas the suggested bug is TEST-UNEXPECTED-PASS, then it's definitely not a match. That's the case for both of the tests failures that I'm mentioning below. Though, generally, I'd be suspicious of all the text-related failures as being "real" and caused by this change, even if they had a possibly-matching intermittent failure.)

In particular, it looks like these tests are regressed in the try run:

• "mask-text-001.svg" failed on several platforms (and the failure looks "real" -- the solid-black rectangle in the testcase is 1px wider than the black rectangle in the testcase):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221260568
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221246315
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221252617

• "webkit-text-stroke-property-001.html" has a subtle failure on several platforms. (For this one, I think the test is already marked as "fuzzy" in its reftest.list file, and the fuzzy annotation probably just needs an adjustment, since the failure here is really subtle & perhaps not important.)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221258958
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221260568

Comment 6

[Daniel Holbert [:dholbert]]

This one looks "real", too (for dynamic-textPath-02.svg):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2546ed63a5deec9222370611e55cf61281f492&selectedJob=221263605

Its failure snapshot shows some yellow antialiasing "fringes" that are misplaced (too high up) in the testcase vs. the reference case.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing reftest comparison icon

Alex: just in case you're not familiar with looking at reftest failures on treeherder -- you can get to a "comparison snapshot view" for a given test-failure by clicking the little bar-chart-looking icon, circled in my screenshot here.

Once that comparison view loads, its "Circle Differences" checkbox is pretty handy -- and then you can carefully hover those circled pixels to see their differences in the zoomed-in grid at the top left. And for differing pixels, the testcase's pixel is shown in the upper-left half of the grid-pixel, vs. the reference case's pixel is in the bottom-right half of the grid pixel. And the rgb values are displayed for whichever exact pixel you're hovering over.

Comment 8

[Alex Henrie]

Thanks for the advice! It's been a while since I've contributed to Firefox and I never really got the hang of Treeherder.

Comment 9

[Alex Henrie]

Attachment: Proposed patch v3

This patch inflates mRect as before, but applies the context scale to ensure that mRect is only inflated by one device pixel. As far as I can tell, all of the tests are passing now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48ce9d33aefd461ae77420ed7d12b4d16d64a2b3

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 9037138 [details] [diff] [review]
Proposed patch v3

Are you sure this is the right value? Why not mFontSizeScaleFactor

Comment 11

[Alex Henrie]

(In reply to Robert Longson [:longsonr] from comment #10)

Comment on attachment 9037138 [details] [diff] [review]
Proposed patch v3

Are you sure this is the right value? Why not mFontSizeScaleFactor

In most cases, mFontSizeScaleFactor == mLastContextScale. However, if the font size needs to be adjusted, they will be different. In ReflowSVG we're trying to inflate mRect by 1 device pixel, which has nothing to do with the font size, so there's no reason to take into account the font size adjustment.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 9037138 [details] [diff] [review]
Proposed patch v3

Review of attachment 9037138 [details] [diff] [review]:
-----------------------------------------------------------------

[canceling review request, as longsonr's r+ is sufficient on its own]

Comment 13

[Alex Henrie]

Requesting checkin of attachment 9037138 [details] [diff] [review]

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/193394b152f2
Apply context scale when inflating mRect of SVG text. r=longsonr

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/193394b152f2

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/193394b152f2