STR: 1. Visit any yelp search results page, e.g. this one: http://www.yelp.com/search?find_desc=mozilla&find_loc=San+Francisco%2C+CA&ns=1 2. Click and drag the map. 3. Watch the "Redo search in map" button that appears. Try to click on it. ACTUAL RESULTS: - Button gets "smeared" as it animates into view. (I can make it re-render correctly by e.g. scrolling, though.) - Button cannot be clicked (clicks seem to be discarded), even after I've made it re-render correctly by scrolling. I'm using latest nightly, 44.0a1 (2015-09-25), on a 64-bit Linux laptop. (NOTE: Yelp picks different map providers on different times that I visit the page -- e.g. Google vs. Bing vs HereMaps. I don't think it matters which map gets used; I've seen this bug with both Google & Bing.) Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54 --> regression from bug 1097464

(Note: this is a decently severe bug for Yelp users. My normal Yelp workflow is to adjust the map to the area I'm concerned with, and then click the "Redo search in map" button to update the results. So if that button's broken, my workflow gets cut off partway through. As a workaround, users can proactively click the "Redo search when map moved" checkbox, *before* adjusting the map. But that only helps if they're expecting in advance to hit this bug. By the time this bug has happened, it's too late to apply that workaround, because the checkbox will have been replaced with an unclickable button.)

I have found the problem of computing bounds of layers causing 1 pixel off problem (not really just one pixel). I am looking in.

Tracking for Firefox 44 because this relates to a popular website and could have potential user impact if left unfixed (especially button being rendered unclickable). Also adding regression keyword, see STR/description for regression range.

Daniel, I have added a patch for bug 1210784. It seems also fix this bug. Could you check it?

I think that patch fixes the animation, but the button is still un-clickable (the cursor still doesn't change like it's supposed to when I hover the button, and clicks on the button have no effect). Should I file a separate bug on that, or leave this one open for that issue?

Thank you! Let's keep this bug open for un-clickable issue.

-- Attachment #8679342 [details] [diff] - Attachment is obsolete: true

Thanks, I'll give that patch a try. [also, assigning bug to Thinker, to reflect reality & silence bugzilla-experimental-UI's "Unassigned bug with patches attached" warning]

I tried the new patch in a enable-debug enable-opt build (enable-opt for good-ish performance), and I can confirm it makes the button clickable again! However, the animation is still jumpy and/or incomplete. See upcoming screencast. Also, I'm still seeing zillions of these errors in my terminal when I move the map (when the button animates into existence): { Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)|[296][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)|[297][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)|[298][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)|[299][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)|[300][GFX1-]: Failed to allocate a surface due to invalid size Size(0,0)[GFX1-]: Failed to allocate a surface due to invalid size Size(0,0) [Parent 11610] WARNING: '!temp', file /scratch/work/builds/mozilla-inbound/mozilla/gfx/layers/basic/BasicCompositor.cpp, line 482 [GFX3-]: Surface width or height <= 0! [GFX3-]: Surface width or height <= 0! } And then when I click the button, I see more of those same errors, surrounded by lots of this one: { [GFX2-]: DrawTargetCairo context in error state: invalid matrix (not invertible)(5) }

(Not sure if those errors are related to the remaining animation quirks; mentioning them just in case they are.

Here's a screencast showing the animation jumpiness / artifacts with attachment 8681634 [details] [diff] [review] applied. Notes on this screencast: - The first time I move the map, we jump straight to a "squashed" near-final frame of animation. (i.e. there is no visible animation, and we don't jump quite to the end) - The second time, we do animate, but we still don't quite complete the animation - it ends up a bit "squished" with a little bit of animated-away text still visible at the top. - The third and fourth times, it looks pretty good. - The fifth time looks kinda like the second time (animates up to ~90% complete and freezes), and then it jumps to the final frame after a bit of a delay. etc.

Daniel, do you applied the patch on bug 1210784 too? I would handle animation issues at that bug.

Sorry, I had not applied that patch. I'll give it a try with both of them.

Could you post an un-bitrotted patch over on bug 1210784? The current patch there doesn't apply cleanly on current mozilla-central, even with fuzz. (And if I update to its old-ish parent cset -- annotated in the patch headers -- then that patch applies cleanly, but then *this* bug's patch won't apply.)

(Though, maybe no more local building + verification is necessary; per comment 5 - comment 6, it sounds like you & I both verified that that bug's patch did fix the animation; and per comment 11 here, it sounds like the clickability is fixed by this bug's patch. So I expect all will be well once both patches land.)

-- Attachment #8681634 [details] [diff] - Attachment is obsolete: true

Comment on attachment 8682915 [details] [diff] [review] Do HitTest with skipping non-leaf preserve-3d transform items, v3 Review of attachment 8682915 [details] [diff] [review]: ----------------------------------------------------------------- Needs a test. It should be easy to write one using elementFromPoint. ::: layout/generic/nsFrame.cpp @@ +1996,5 @@ > > nsRect dirtyRectOutsideTransform = dirtyRect; > if (isTransformed) { > const nsRect overflow = GetVisualOverflowRectRelativeToSelf(); > + if ((aBuilder->IsForPainting() || aBuilder->IsForEventDelivery()) && How about we just eliminate (aBuilder->IsForPainting() || aBuilder->IsForEventDelivery()) entirely?

-- Attachment #8682915 [details] [diff] - Attachment is obsolete: true

Robert, I have added a mochitest testcase.

-- Attachment #8685959 [details] [diff] - Attachment is obsolete: true

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > ::: layout/generic/nsFrame.cpp > @@ +1996,5 @@ > > > > nsRect dirtyRectOutsideTransform = dirtyRect; > > if (isTransformed) { > > const nsRect overflow = GetVisualOverflowRectRelativeToSelf(); > > + if ((aBuilder->IsForPainting() || aBuilder->IsForEventDelivery()) && > > How about we just eliminate (aBuilder->IsForPainting() || > aBuilder->IsForEventDelivery()) entirely? What about this comment?

I think it can be removed! Update it later.

-- Attachment #8685963 [details] [diff] - Attachment is obsolete: true

-- Attachment #8686449 [details] [diff] - Attachment is obsolete: true

Comment on attachment 8686966 [details] [diff] [review] Do HitTest with skipping non-leaf preserve-3d transform items, v6 r=roc

Thinker, does this need to be uplifted to Aurora44?

NI'ing Ken as well. Please see my last comment.

yes, please!

Comment on attachment 8686966 [details] [diff] [review] Do HitTest with skipping non-leaf preserve-3d transform items, v6 Approval Request Comment [Feature/regressing bug #]: 1208673 [User impact if declined]: Users can not interact with the content in a preserve3d context. [Describe test coverage new/current, TreeHerder]: Pass reftests except some intermittents [Risks and why]: [String/UUID change made/needed]:

Daniel, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

I'm actually still seeing bugginess here, similar to what was described in comment 6 -- the animation plays fine, but the button is un-clickable. :-/ Not sure if I should reopen, or file a new bug (or if bug 1226904 will fix this).

Good news! (sort of) The issue I'm seeing in current nightlies -- with the button being un-clickable -- is an independent regression that happened 10 days after this bug's patch landed on central. (mozregression says it's a regression from bug 1168263, specifically, and I'm about to file a bug for it.) So, this bug's patch *was* effective when it landed, and it does seem to be worth backporting as a fix for this issue. (unless we were planning on backporting bug 1168263, but I don't think we are.)

(I filed bug 1230693 on the new regression described in comment 38 and 39.)

In terms of backporting, note that this fix is being blamed on a number of regressions (the depends on field of this bug).

Oh, true. I withdraw my "worth backporting" comment at the end of comment 39, given the regressions here (unless we're confident we can also backport fixes for those regressions). Thinker, what do you think? (Perhaps we should avoid shipping this bug with Firefox 44 by backing out bug 1097464 from that branch, rather than by taking this bug's patch & introducing other regressions?)

It is ok for me to pull off it from Firefox 44. Now, I think I need more time to fix bugs.

Comment on attachment 8686966 [details] [diff] [review] Do HitTest with skipping non-leaf preserve-3d transform items, v6 OK, thanks -- canceling your approval request for this bug's patch & my pending needinfo=Ritu, then.

Bug 1097464 was backed out from Fx44, which should be available on the beta channel in the next day or so. Fx45+ remain affected, however. Given the pile of regressions this bug caused, I think it should probably still be tracked on Fx45, however.

I don't think fixing the known regressions site by site is a good approach. If we broke this many things (ESPN, Yelp, DuckDuckGo, Pinterest, etc) there are probably many more sites affected. We should not be shipping this work on 45 or even 46. But at this point, the backout done for 44 on this and bug 1097464 and whatever else is involved, may not be exactly what we need to be backing out for 45/46 since there may be extra work done since in an attempt to fix the regressions. What about putting this work behind a nightly-only ifdef in order to give us some time to figure out the problem without having to back out increasing cascades of regressions ? What can we point to exactly that is (still increasing, as far as I can see) fallout from bug 1097464? Should this work be in 45 at all?

I agree with you Liz. I asked Thinker to prepare a list of necessary backouts on all branches (including m-c) to come back to the 44 state. Then, work on a new patch which will have a preferences/#define to turn it back on when ready.

This along with the change that caused this regression were backed out from Firefox 45. https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

This is the cause of https://github.com/webcompat/web-bugs/issues/2245, which reveals a larger issue: All hyperlinks in Reveal.js slidedecks are busted (see http://lab.hakim.se/reveal-js/#/3).

(oops, I didn't see Bug 1230774 was already reported -- nevermind me)