Closed Bug 1509394 Opened 3 years ago Closed 2 months ago
Unable to drag text or design on Spreadshirt shirt designer
47 bytes, text/x-phabricator-request
|Details | Review|
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
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
Priority: -- → P2
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.
(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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/513b8b0d5683 Fix pointer events regression for SVG clipPath clipped content. r=longsonr
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
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
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?
Status: REOPENED → RESOLVED
Closed: 3 years ago → 2 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.