1509394 - Unable to drag text or design on Spreadshirt shirt designer

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Adam Stevenson [:adamopenweb]]

Originally reported: https://github.com/webcompat/web-bugs/issues/21706

When creating a t-shirt on Spreadshirt.fr (also tested on Spreadshirt.ca), unable to move around design or text. Works as expected in Firefox 63.

Environment:
OS: Windows, MacOS
Browser: Firefox 64 & 65

Steps to reproduce:
1. Navigate to https://shop.spreadshirt.fr/T-shirt-chic-et-choc/create
2. Click and drag the rectangle "A Day Without My Motorbike..."

Expected behaviour:
The design moves to a new position on the shirt

Actual behaviour:
The design doesn't move, the browser drags the background image and displays a "ghost" of the shirt.

Mozregression returned:
8:24.84 INFO: No more inbound revisions, bisection finished.
8:24.84 INFO: Last good revision: 17f44e6e8f490d93e8846e0c611665203eb0a942
8:24.84 INFO: First bad revision: f5195e1f3615dc806399e5fb58b0ba44d217e87c
8:24.84 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=17f44e6e8f490d93e8846e0c611665203eb0a942&tochange=f5195e1f3615dc806399e5fb58b0ba44d217e87c

Comment 1

[Adam Stevenson [:adamopenweb]]

Jwatt, maybe you can take a look at this?

Comment 2

[Jonathan Watt [:jwatt]]

The <image> element that displays the print is wrapped in a great-great-grandparent-<g clip-path="url(#c5130)">. That id - "c5130" - is not valid (no element in the document has that id). Removing the clip-path attribute gets the page working again.

It looks like the nsSVGUtils::HitTestClip change in https://hg.mozilla.org/integration/mozilla-inbound/rev/f5195e1f3615 made us return false (pointer events do not hit the element) if the clip-path is invalid. I think that was to match nsSVGUtils::PaintFrameWithEffects where we don't paint the element if the clip-path is invalid. That seems very sensible, but actually we do paint the element when the clip-path doesn't match, because we don't take the nsSVGUtils::PaintFrameWithEffects path in this case; we only take that path in non-display SVG, whereas since this is displayed on screen we take the display list path.

So we need to fix two things. First the nsSVGUtils::HitTestClip behavior should be reverted to be web compatible again. Second, the display list painting path and the nsSVGUtils::PaintFrameWithEffects path should be made consistent (probably in a separate bug though, since that's a separate long standing issue.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Setting this as tracking+ for 64, but this is the last week before we start building RCs, so a patch is going to need to materialize pretty soon to make it into the final release.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Bug 1509394. Fix clip-path hit-testing regression. r?longsonr

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Second, the display list
> painting path and the nsSVGUtils::PaintFrameWithEffects path should be made
> consistent (probably in a separate bug though, since that's a separate long
> standing issue.

I filed bug 1509949.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/513b8b0d5683
Fix pointer events regression for SVG clipPath clipped content. r=longsonr

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/513b8b0d5683

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 9027630 [details]
Bug 1509394. Fix clip-path hit-testing regression. r?longsonr

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1495034

User impact if declined: Any content referencing a non-existent or invalid SVG clipPath will not respond to pointer events, breaking sites like the one reported.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This reverts to the old behavior.

String changes made/needed: none

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9027630 [details]
Bug 1509394. Fix clip-path hit-testing regression. r?longsonr

fixing a new regression in 64, approved for 64.0b14

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/390cd25cecd6

Comment 11

[ovidiu boca[:Ovidiu]]

I tested this on Windows 10 x64, Mac OS X 10.14, with FF Nightly 65.0a1(2018-11-28)and the issue is still reproducible. 
Jonathan, can you please take a look? Thanks

Comment 12

[Jonathan Watt [:jwatt]]

Darnit! I made a "trivial" logic change to my original patch to make it less clunky than the original code, and I guess I forgot to confirm it worked.

The issue here is that nsCSSClipPathInstance::HitTestBasicShapeOrPathClip returns false if the clip-path doesn't reference a URL. That should really be returning true IMO, but since it's safer to use the original pre-regression nsSVGUtils::HitTestClip code, I'll do that.

Comment 13

[Jonathan Watt [:jwatt]]

Thanks for catching this, Ovidiu!

Comment 14

[Andrew Overholt [:overholt]]

Jonathan, are you going to be able to create a new patch here?

Comment 15

[Jonathan Watt [:jwatt]]

Thanks for the ping. Yes, I got caught up in Fission prep for the All Hands and this fell off my radar.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Can we get a test for the issue found by QA too?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Per an email thread with jwatt, the website has changed such that this issue no longer actively reproduces. Dropping it from the 65 radar.

Comment 19

[Adrian Florinescu [:aflorinescu]]

Dropping this from the QA radar as well, let's have the qe+ added back when the issue is fixed.

Comment 20

[Robert Longson [:longsonr]]

Fixed by checkin for bug 1531888