Unable to drag text or design on Spreadshirt shirt designer

REOPENED
Assigned to

Status

()

defect
P2
normal
REOPENED
8 months ago
4 months ago

People

(Reporter: adamopenweb, Assigned: jwatt, NeedInfo)

Tracking

(Blocks 1 bug, {regression})

64 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 wontfix, firefox64+ wontfix, firefox65- wontfix, firefox66- fix-optional)

Details

()

Attachments

(1 attachment)

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
Jwatt, maybe you can take a look at this?
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(jwatt)
Keywords: regression
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.
Assignee: nobody → jwatt
Flags: needinfo?(jwatt)
Priority: -- → P2
Hardware: Desktop → All
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.
See Also: → 1509949
(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.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/513b8b0d5683
Fix pointer events regression for SVG clipPath clipped content. r=longsonr
https://hg.mozilla.org/mozilla-central/rev/513b8b0d5683
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
Attachment #9027630 - Flags: approval-mozilla-beta?
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
Attachment #9027630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: needinfo?(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(jwatt)
Resolution: FIXED → ---
Thanks for catching this, Ovidiu!
Jonathan, are you going to be able to create a new patch here?
Thanks for the ping. Yes, I got caught up in Fission prep for the All Hands and this fell off my radar.
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.
Can we get a test for the issue found by QA too?
Target Milestone: mozilla65 → ---

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

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

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.