1208673 - Yelp "Redo search in map" button gets smeared while animating into view (corruption / artifacts), and is un-clickable

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Thinker Li [:sinker]]

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

Comment 4

[Kate Glazko]

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.

Comment 5

[Thinker Li [:sinker]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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?

Comment 7

[Thinker Li [:sinker]]

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

Comment 8

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items

Comment 9

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v2

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

Comment 10

[Daniel Holbert [:dholbert]]

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]

Comment 11

[Daniel Holbert [:dholbert]]

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)
}

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug w/ most recent patch (attachment 8681634) applied

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.

Comment 14

[Thinker Li [:sinker]]

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

Comment 15

[Daniel Holbert [:dholbert]]

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

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

[Daniel Holbert [:dholbert]]

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

Comment 18

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v3

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 21

[Thinker Li [:sinker]]

Attachment: fix-hittest-collecting-leaves.diff

Comment 22

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v4

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

Comment 23

[Thinker Li [:sinker]]

Robert, I have added a mochitest testcase.

Comment 24

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v5

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

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 26

[Thinker Li [:sinker]]

I think it can be removed!  Update it later.

Comment 27

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v6

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

Comment 28

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83391a1bd799

Comment 29

[Thinker Li [:sinker]]

Attachment: Do HitTest with skipping non-leaf preserve-3d transform items, v6

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

Comment 30

[Thinker Li [:sinker]]

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

r=roc

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7d0d74ced6

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac7d0d74ced6

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thinker, does this need to be uplifted to Aurora44?

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 35

[Thinker Li [:sinker]]

yes, please!

Comment 36

[Thinker Li [:sinker]]

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]:

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 38

[Daniel Holbert [:dholbert]]

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

Comment 39

[Daniel Holbert [:dholbert]]

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

Comment 40

[Daniel Holbert [:dholbert]]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

[Daniel Holbert [:dholbert]]

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

Comment 43

[Thinker Li [:sinker]]

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

Comment 44

[Daniel Holbert [:dholbert]]

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.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 46

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

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?

Comment 47

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

Comment 49

[Mike Taylor [:miketaylr]]

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

Comment 50

[Mike Taylor [:miketaylr]]

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