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

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: alexhenrie24, Assigned: alexhenrie24)

Tracking

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted file 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

Blocks: 884718
Posted patch Proposed patch (obsolete) — Splinter Review

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

Assignee: nobody → alexhenrie24
Attachment #9035660 - Flags: review?(jwatt)
Attachment #9035660 - Flags: review?(cam)
Attachment #9035660 - Attachment description: Propo → Proposed patch
Posted patch Proposed patch v2 (obsolete) — Splinter Review

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

Attachment #9035660 - Attachment is obsolete: true
Attachment #9035660 - Flags: review?(jwatt)
Attachment #9035660 - Flags: review?(cam)
Attachment #9035846 - Flags: review?(jwatt)
Attachment #9035846 - Flags: review?(cam)
Attachment #9035846 - Flags: review?(longsonr)
Attachment #9035846 - Flags: review?(dholbert)
Attachment #9035846 - Flags: review?(bbirtles)

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

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

Attachment #9035846 - Flags: review?(longsonr) → review-

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

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:

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.

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.

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

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

Attachment #9035846 - Attachment is obsolete: true
Attachment #9035846 - Flags: review?(jwatt)
Attachment #9035846 - Flags: review?(dholbert)
Attachment #9035846 - Flags: review?(cam)
Attachment #9035846 - Flags: review?(bbirtles)
Attachment #9037138 - Flags: review?(longsonr)
Attachment #9037138 - Flags: review?(dholbert)
Attachment #9037138 - Flags: review?(longsonr) → review+

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

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

Attachment #9037138 - Flags: review+ → review?(longsonr)

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

Attachment #9037138 - Flags: review?(longsonr) → review+
Priority: -- → P3
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]
Attachment #9037138 - Flags: review?(dholbert)
Keywords: checkin-needed

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/193394b152f2
Apply context scale when inflating mRect of SVG text. r=longsonr

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
See Also: → 1521425
See Also: → 1529182
Regressions: 1544432
You need to log in before you can comment on or make changes to this bug.