Closed Bug 1330303 Opened 7 years ago Closed 7 years ago

Rearranging an existing tab tile in about:newtab is sluggish

Categories

(Firefox :: New Tab Page, defect)

53 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox51 + verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: Fanolian+BMO, Assigned: dao)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files)

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.
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
Blocks: 1322738
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → New Tab Page
Animation was smooth before fixing bug 1322738.
If I set browser.newtabpage.compact to true, the animation is smooth but with a little bit stuttering. Probably it's just my computer.
Attachment #8825807 - Attachment description: about:newtab before fixing bug 1322738 → [GOOD] 2017-01-04 new tab.mp4
Dão, any idea what's going on here? The difference seems quite stark in the video.
Flags: needinfo?(dao+bmo)
Attached patch patchSplinter Review
Super-slow CSS selectors apparently.

This makes the hover effect for non-compact match compact. Not a big deal.
Assignee: nobody → dao+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dao+bmo)
Attachment #8825823 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8825823 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8825823 - Flags: review+
We should make sure this hits beta/aurora as well as we don't want to regress anything outside of the funnelcake.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac85b75fea8
Remove body:not(.compact) from selectors to undo dragging performance regression. r=gijs
[Tracking Requested - why for this release]: regression from bug 1322738
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]: /
Attachment #8825823 - Flags: approval-mozilla-beta?
Attachment #8825823 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3ac85b75fea8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Track 51+ as regression in rearranging an existing tab tile in about:newtab
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.
Attachment #8825823 - Flags: approval-mozilla-beta?
Attachment #8825823 - Flags: approval-mozilla-beta+
Attachment #8825823 - Flags: approval-mozilla-aurora?
Attachment #8825823 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: