1330303 - Rearranging an existing tab tile in about:newtab is sluggish

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Fanolian]

Attachment: [BAD] 2017-01-11 new tab.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170111030235

Steps to reproduce:

1. In a new profile, make sure browser.newtabpage.compact is set to false (current default).
2. Open about:newtab.
3. Try to move an existing tab tile around.


Actual results:

(Please refer to attached video)
The animation is sluggish. It leaves a bad feedback for users.


Expected results:

The tile animation should follow mouse cursor closely.

Comment 1

[Fanolian]

From Mozregression:
Last good Nightly: 2017-01-04
First bad Nightly: 2017-01-05
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=57ac9f63fc6953f4efeb0cc84a60192d3721251f&tochange=f13abb8ba9f366c9f32a3146245adf642528becd

Further bisection:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f897399fb28a18b34f5200677e2d585e5d4342a2&tochange=c8883a87c6dc7c8d355fc93c9ee08bb75cfbf7db

Regressed by: bug 1322738

Comment 2

[Fanolian]

Attachment: [GOOD] 2017-01-04 new tab.mp4

Animation was smooth before fixing bug 1322738.

Comment 3

[Fanolian]

Attachment: Animation in new (compact) about:newtab layout.mp4

If I set browser.newtabpage.compact to true, the animation is smooth but with a little bit stuttering. Probably it's just my computer.

Comment 4

[:Gijs (he/him)]

Dão, any idea what's going on here? The difference seems quite stark in the video.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Super-slow CSS selectors apparently.

This makes the hover effect for non-compact match compact. Not a big deal.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8825823 [details] [diff] [review]
patch

From IRC:

[15:06:59]	Gijs	dao: looking at your patch, why was the newtab-site selector originally changed to body:not(.compact) .newtab-site, and what effect are we now having on the new-style thumbnails that we weren't before?
[15:07:23]	Gijs	dao: like, I get that if we don't change the selector its specificity might override the now less-specific hover styles
[15:07:42]	Gijs	dao: but I'm confused whether this impacts the .compact styling at all because I don't see anything that overrides those properties for the .compact case


Clearing review for now.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8825823 [details] [diff] [review]
patch

Review of attachment 8825823 [details] [diff] [review]:
-----------------------------------------------------------------

OK, per conversation on IRC and poking at this live against beta, I guess this works. We get a box shadow effect back in the compact case which in the grand scheme of things shouldn't matter.

Comment 8

[:Gijs (he/him)]

We should make sure this hits beta/aurora as well as we don't want to regress anything outside of the funnelcake.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac85b75fea8
Remove body:not(.compact) from selectors to undo dragging performance regression. r=gijs

Comment 10

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]: regression from bug 1322738

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8825823 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322738
[User impact if declined]: see comment 0
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: /
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: it only reverts part of bug 1322738
[String changes made/needed]: /

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ac85b75fea8

Comment 13

[Gerry Chang [:gchang]]

Track 51+ as regression in rearranging an existing tab tile in about:newtab

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8825823 [details] [diff] [review]
patch

Fix a UI regression about rearranging an existing tab tile. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a7701e2bb8e

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a7701e2bb8e

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d6605552545

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.11 using Firefox 51 RC (buildID: 20170116133120), latest Aurora 52.0a2 (buildID: 20170118004007) and latest Nightly 53.0a1 (buildID: 20170117030218).