1369287 - shipping compact new tab page view by default

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[[e-mail]]

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
```

Comment 1

[[e-mail]]

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?

Comment 2

[Dão Gottwald [:dao]]

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

Comment 3

[[e-mail]]

Attachment: Bug 1369287 - shipping compact new tab page view by default;

Review commit: https://reviewboard.mozilla.org/r/148358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148358/

Comment 4

[[e-mail]]

Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148358/diff/1-2/

Comment 5

[[e-mail]]

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

[:Gijs (he/him)]

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.

Comment 7

[[e-mail]]

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 8

[[e-mail]]

Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148358/diff/2-3/

Comment 9

[[e-mail]]

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.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

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

Comment 11

[[e-mail]]

Thanks!

Try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02701ad8c925&selectedJob=109501786

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/517ff2a27de6
shipping compact new tab page view by default;r=Gijs

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 14

[[e-mail]]

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.

Comment 15

[[e-mail]]

Comment on attachment 8877024 [details]
Bug 1369287 - shipping compact new tab page view by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148358/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8cb08b2aa4d
shipping compact new tab page view by default;r=Gijs

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Oriol Brufau [:Oriol]]

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?

Comment 19

[Dão Gottwald [:dao]]

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

Comment 20

[Justin [:JW_SoftvisionQA]]

This issue has been verified on win 10 x64 with today's nightly build.

Comment 21

[[e-mail]]

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

Comment 22

[Peter Dolanjski [:pdol]]

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?

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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!

Comment 24

[[e-mail]]

filed the followup bug 1379860, thanks

Comment 25

[Marco Castelluccio [:marco]]

(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

[:Gijs (he/him)]

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

Comment 27

[Marco Castelluccio [:marco]]

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

Comment 28

[Marco Castelluccio [:marco]]

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

[:Gijs (he/him)]

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

Comment 30

[[e-mail]]

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

Comment 31

[Justin [:JW_SoftvisionQA]]

Since I have already verified this fix and comment 24 I will close this issue as verified.