Rearranging an existing tab tile in about:newtab is sluggish

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Fanolian+bugzilla, Assigned: dao)

Tracking

({regression, reproducible})

53 Branch
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox51+ verified, firefox52 verified, firefox53 verified)

Details

Attachments

(4 attachments)

Reporter

Description

3 years ago
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.
Reporter

Comment 1

3 years ago
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
Reporter

Comment 2

3 years ago
Animation was smooth before fixing bug 1322738.
Reporter

Comment 3

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

Updated

3 years ago
Attachment #8825807 - Attachment description: about:newtab before fixing bug 1322738 → [GOOD] 2017-01-04 new tab.mp4

Comment 4

3 years ago
Dão, any idea what's going on here? The difference seems quite stark in the video.
Flags: needinfo?(dao+bmo)
Assignee

Comment 5

3 years ago
Posted 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 6

3 years ago
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 7

3 years ago
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+

Comment 8

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

Comment 9

3 years ago
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
Assignee

Comment 10

3 years ago
[Tracking Requested - why for this release]: regression from bug 1322738
Assignee

Comment 11

3 years ago
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?

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ac85b75fea8
Status: ASSIGNED → RESOLVED
Closed: 3 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.