shipping compact new tab page view by default

VERIFIED FIXED in Firefox 56

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
3 months ago
9 days ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

(Depends on: 3 bugs, {user-doc-needed})

unspecified
Firefox 56
user-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
Based on PM(pdol)'s request, we'd like to shipping compact newtab page (+ 6 columns + 2 rows) with onboarding overlay by default

in `browser/app/profile/firefox.js` 

```
browser.newtabpage.compact=true
browser.newtabpage.columns=6
browser.newtabpage.row=2
browser.onboarding.disabled=true
```
(Assignee)

Updated

3 months ago
Depends on: 1354046
Whiteboard: [photon-onboarding]

Updated

3 months ago
Flags: qe-verify+
QA Contact: jwilliams
(Assignee)

Updated

3 months ago
Assignee: nobody → gasolin
(Assignee)

Updated

3 months ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 56
(Assignee)

Comment 1

3 months ago
Dao, as comment above, PM would like to ship the compact view by default in 56,

Is there any left work need to be done before shipping to v56?
Flags: needinfo?(dao+bmo)

Comment 2

3 months ago
(In reply to Fred Lin [:gasolin] from comment #1)
> Dao, as comment above, PM would like to ship the compact view by default in
> 56,
> 
> Is there any left work need to be done before shipping to v56?

Not that I know.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 months ago
The reason to pick (+ 6 columns + 2 rows) is we'd like to make it looks like activity-stream, which will only ship to a portion of user in v56, and is expected to replace the about:newtab.

Comment 6

2 months ago
mozreview-review
Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

https://reviewboard.mozilla.org/r/148358/#review152902

I don't understand this patch.

There seem to be 2 parts to this: the compact layout and the onboarding overlay.

The compact layout we can enable. I agree with Dão that it should be usable.

For the second part, the prefs listed in comment #0 suggests that we intend to disable the onboarding overlay. But this patch doesn't do that. The deps of bug 1354046 are also mostly unfixed, so I don't understand why we would want to ship this. Certainly the onboarding overlay is still empty and useless in my nightly. Turning it on off-nightly seems premature.

Furthermore, the code as-is suggests that the browser.onboarding.enabled pref is already set to true (it's not part of this patch, but the context in this file has it set to true), but the code is ifdef'd out to only be available on Nightly (ifdefs removed in this patch). How do the pref and the non-existent code interact on 55 now that that's off-nightly? Should the pref also be ifdef'd for 55?


So, to summarize, while I would r+ only the 2 pref changes to swap over to a compact layout for tiles, I think we should postpone making a decision about shipping the onboarding overlay in 56 to later in the cycle when we are confident that all the relevant bits have landed and we are happy to ship the overlay in its then-current state.
Attachment #8877024 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

2 months ago
this patch is target for 56 so it won't affect 55. 

`browser.onboarding.disabled=true` in Firefox.js is moved to `browser.onboarding.enabled=true` which is on by default in nightly. So we don't need to `enable` it in firefox.js.

There are several overlay tours waiting in queue and will be landed this week. I'll wait until they are ready and send review again.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
Though onboarding has great progress and it will be more stable after ALL Hands.

I think its still worth to ship compact view earlier and see if there's any issue around that.

So I plan to file another bug and send PR once we feel confident to release onboarding overlay to all versions.
Summary: shipping onboarding overlay with compact new tab page view by default → shipping compact new tab page view by default
(Assignee)

Updated

2 months ago
Blocks: 1375793
(Assignee)

Updated

2 months ago
No longer blocks: 1375793
See Also: → bug 1375793

Comment 10

2 months ago
mozreview-review
Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

https://reviewboard.mozilla.org/r/148358/#review157064
Attachment #8877024 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 11

2 months ago
Thanks!

Try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02701ad8c925&selectedJob=109501786
Keywords: checkin-needed, user-doc-needed

Comment 12

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/517ff2a27de6
shipping compact new tab page view by default;r=Gijs
Keywords: checkin-needed
Backed out for failing mochitest-clipboard's browser_newtab_drag_drop_ext.js on Windows and OS X, and browser_newtab_bug752841.js on Windows 8 x64 debug:

https://hg.mozilla.org/integration/autoland/rev/46e569782344795d0c1cdd2df93fe1ce560c5f94

First push running failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=84467f2e7e673ba1ba9995766c4a632f80e10492&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Clipboard failure: https://treeherder.mozilla.org/logviewer.html#?job_id=109685354&repo=autoland
15:51:27     INFO - Leaving test bound setup
15:51:27     INFO - Entering test bound 
15:51:27     INFO - Buffered messages logged at 15:51:26
15:51:27     INFO - TEST-PASS | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | grid status = 0,1,2,3,4,5,6,7p,8p - ["0","1","2","3","4","5","6","7p","8p"] == "0,1,2,3,4,5,6,7p,8p" - 
15:51:27     INFO - TEST-PASS | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | grid status = 99p,0,1,2,3,4,5,7p,8p - ["99p","0","1","2","3","4","5","7p","8p"] == "99p,0,1,2,3,4,5,7p,8p" - 
15:51:27     INFO - Buffered messages logged at 15:51:27
15:51:27     INFO - TEST-PASS | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | grid status = 0,1,2,3,4,5,6,7p,8p - ["0","1","2","3","4","5","6","7p","8p"] == "0,1,2,3,4,5,6,7p,8p" - 
15:51:27     INFO - Buffered messages finished
15:51:27     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js | grid status = 0,1,2,3,4,99p,5,7p,8p - ["0","1","2","3","4","99p"] == "0,1,2,3,4,99p,5,7p,8p" - 
15:51:27     INFO - Stack trace:
15:51:27     INFO - resource://testing-common/content-task.js line 52 > eval:null:20
15:51:27     INFO - Not taking screenshot here: see the one that was previously logged

browser-chrome failure: https://treeherder.mozilla.org/logviewer.html#?job_id=109689561&repo=autoland
16:27:51     INFO - Leaving test bound setup
16:27:51     INFO - Entering test bound 
16:27:51     INFO - Buffered messages finished
16:27:51     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/newtab/browser_newtab_bug752841.js | Grid length of existing page before update is correctly. - Got 12, expected 15
16:27:51     INFO - Stack trace:
16:27:51     INFO - chrome://mochikit/content/browser-test.js:test_is:983
16:27:51     INFO - chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_bug752841.js:null:34
Flags: needinfo?(gasolin)
(Assignee)

Comment 14

2 months ago
Change to 2 rows broke the test, since when the test case (contain 9 items) strict to 3 columns, the grid list is truncated (3x2 < 9)
http://searchfox.org/mozilla-central/source/browser/base/content/test/newtab/browser_newtab_drag_drop_ext.js#38

I'll set the rows to 3 and add extra test for the grid list is truncated case.
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 16

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c8cb08b2aa4d
shipping compact new tab page view by default;r=Gijs
Keywords: checkin-needed

Comment 17

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8cb08b2aa4d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
I have a wide screen (1920 x 1080). This compact newpage looks so empty and the thumbnails are so tiny that they become useless because I can barely see what's in the image. I reverted the pref immediately. Has the UX team really thought about this?
(In reply to Oriol [:Oriol] from comment #18)
> I have a wide screen (1920 x 1080). This compact newpage looks so empty and
> the thumbnails are so tiny that they become useless because I can barely see
> what's in the image. I reverted the pref immediately. Has the UX team really
> thought about this?

FWIW, I agree the thumbnails are too small, but this is what I was asked to implement. Personally, I have zoomed in on the new tab page to 133%.
This issue has been verified on win 10 x64 with today's nightly build.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 21

2 months ago
(In reply to Oriol [:Oriol] from comment #18)
> I have a wide screen (1920 x 1080). This compact newpage looks so empty and
> the thumbnails are so tiny that they become useless because I can barely see
> what's in the image. I reverted the pref immediately. Has the UX team really
> thought about this?

Oriol thanks for the report! I forwarded your concern to activity stream team (who designed that UI) and wish they will help find out the best result.

Updated

2 months ago
Depends on: 1377756
Depends on: 1377323, 1377318, 1377317

Updated

2 months ago
No longer depends on: 1377323

Updated

2 months ago
No longer depends on: 1377318
Some concerns were raised about shipping this experience in 56 given that Activity Stream was coming in 57.
In particular: we'd be changing the experience and then changing it again one release later with different methods of pinning and controlling the content. It would be better to change it once given how frequently this page is shown. In addition, Activity Stream would be setting new top site defaults, so it didn't make sense to ship a new set in 56 (which hasn't been defined per locale).

As a result, I am asking that this change be backed out such that we revert to the previous newtab implementation.
The onboarding tour overlay and notifications would live on top of that implementation (non-compact).

Fred, are you able to make that change?
Status: VERIFIED → REOPENED
Flags: needinfo?(gasolin)
Resolution: FIXED → ---
Let's open a new bug for setting the pref back in another bug since this is not going to be a straightforward backout of the landed patch. Fred, could you file that bug and land it? Thanks!
(Assignee)

Updated

a month ago
Blocks: 1379860
(Assignee)

Comment 24

a month ago
filed the followup bug 1379860, thanks
Status: REOPENED → RESOLVED
Last Resolved: 2 months agoa month ago
Flags: needinfo?(gasolin)
Resolution: --- → FIXED
(In reply to Peter Dolanjski [:pdol] from comment #22)
> Some concerns were raised about shipping this experience in 56 given that
> Activity Stream was coming in 57.
> In particular: we'd be changing the experience and then changing it again
> one release later with different methods of pinning and controlling the
> content. It would be better to change it once given how frequently this page
> is shown.

After the backout, I still see something different than before.
I have a newtab page with 4x3 thumbnails and I need to scroll to see the bottom four.
Previously, the page contained 4x2 thumbnails and I didn't have to scroll at all.

Comment 26

a month ago
(In reply to Marco Castelluccio [:marco] from comment #25)
> (In reply to Peter Dolanjski [:pdol] from comment #22)
> > Some concerns were raised about shipping this experience in 56 given that
> > Activity Stream was coming in 57.
> > In particular: we'd be changing the experience and then changing it again
> > one release later with different methods of pinning and controlling the
> > content. It would be better to change it once given how frequently this page
> > is shown.
> 
> After the backout, I still see something different than before.
> I have a newtab page with 4x3 thumbnails and I need to scroll to see the
> bottom four.
> Previously, the page contained 4x2 thumbnails and I didn't have to scroll at
> all.

The default number of rows was always 3, so if you had 2 rows then you customized that in about:config. You're just seeing this because it switched to 2 (which meant your value in user prefs got blown away because it matched the default) and now back to 3, so now you got migrated to the 'new' default. That's unfortunate, but not something we can do an awful lot about, and it won't hit anyone other than nightly users (not off-nightly because it never changed there) who had this exact matching pref.
(In reply to :Gijs (no reviews, mostly out until Jul 17) from comment #26)
> (In reply to Marco Castelluccio [:marco] from comment #25)
> > (In reply to Peter Dolanjski [:pdol] from comment #22)
> > > Some concerns were raised about shipping this experience in 56 given that
> > > Activity Stream was coming in 57.
> > > In particular: we'd be changing the experience and then changing it again
> > > one release later with different methods of pinning and controlling the
> > > content. It would be better to change it once given how frequently this page
> > > is shown.
> > 
> > After the backout, I still see something different than before.
> > I have a newtab page with 4x3 thumbnails and I need to scroll to see the
> > bottom four.
> > Previously, the page contained 4x2 thumbnails and I didn't have to scroll at
> > all.
> 
> The default number of rows was always 3, so if you had 2 rows then you
> customized that in about:config. You're just seeing this because it switched
> to 2 (which meant your value in user prefs got blown away because it matched
> the default) and now back to 3, so now you got migrated to the 'new'
> default. That's unfortunate, but not something we can do an awful lot about,
> and it won't hit anyone other than nightly users (not off-nightly because it
> never changed there) who had this exact matching pref.

I didn't actually remember the number of thumbnails I had before, I got it by running a Nightly from 2017-04-04 with a clean profile, where the default was 4x2.
To be clear, "browser.newtabpage.rows" is 3 and "browser.newtabpage.columns" is 5 in the previous Nightly, and there are 4x2 thumbnails in about:newtab.
In the new Nightly, the prefs are the same, but there are 4x3 thumbnails in about:newtab.

Comment 29

a month ago
(In reply to Marco Castelluccio [:marco] from comment #28)
> To be clear, "browser.newtabpage.rows" is 3 and "browser.newtabpage.columns"
> is 5 in the previous Nightly, and there are 4x2 thumbnails in about:newtab.
> In the new Nightly, the prefs are the same, but there are 4x3 thumbnails in
> about:newtab.

File a separate bug and needinfo with an accurate description of (a) what the states of the prefs were and (b) what the problem is in your view. Right now I don't understand anymore from the repeated comments with the different information, and in any case it seems like it might not have been caused by this bug considering the backout was pretty straightforward, so continuing here doesn't seem like it makes sense.
(Assignee)

Comment 30

a month ago
As comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1379860#c11 I found the screencast (on macbook air) also shows 4 columns before these changes. But it's due to the tile will reorder according to the screen width. You can test that by zoom in/out the newtab page
Since I have already verified this fix and comment 24 I will close this issue as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.