1194895 - New tab page has unnecessary (or marginally-necessary) vertical scrollbar at various sizes

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Starting in today's Nightly (I think), I'm seeing a vertical scrollbar much more frequently on about:newtab.

This includes cases where:
 (1) the only tiles which are out of view (and forcing the vertical scrollbar) are *empty dotted-border tiles*.  (It makes some sense that these blank tiles would appear when there's space for them, but not when there's no space.)

 (2) the only tiles which are out of view (and forcing the vertical scrollbar) are tiles that we're happy to leave un-displayed at other window-sizes.  E.g. if I make my window wide enough to show 2 tiles side-by-side, then about:newtab renders with 6 tiles (3 rows of 2), regardless of how tall the window is. But if I make my window skinnier, it only shows 3 tiles (3 rows of 1).

The presence of a scrollbar (i.e. the fact that we're showing too-many-tiles-to-fit-my-viewport) seems broken to me, in both (1) and (2), but especially (1).

I'm assuming this is a regression from bug 1180387, based on looking at recent commits that mention "New Tab" in their commit message.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot of blank tiles forcing a scrollbar

(In reply to Daniel Holbert [:dholbert] from comment #0)
> This includes cases where:
>  (1) the only tiles which are out of view (and forcing the vertical
> scrollbar) are *empty dotted-border tiles*.  (It makes some sense that these
> blank tiles would appear when there's space for them, but not when there's
> no space.)

Here's a screenshot of this scenario, BTW. This is with today's nightly, 43.0a1 (2015-08-14), and a fresh profile.

Comment 2

[Daniel Holbert [:dholbert]]

I think what's really happening here is we're requiring the new-tab page to be tall enough to render 3 rows of tiles... (and if your browser's not that big, it overflows -- maybe by a lot, if your browser's only tall enough for 1 row -- and causes a scrollbar).

This seems odd to me. The new tab page is nicely responsive to viewport-size in a lot of ways (e.g. adjusting number of columns nicely, even down to a single column wide).  I think we want it to be responsive vertically, too...?

Comment 3

[maxim zhilyaev]

Attachment: v1. Aviod a row of empty tiles

avoid showing a row of empty tiles

Comment 4

[maxim zhilyaev]

Daniel,

Give a try to the attached patch.  it avoids showing empty row of tiles, but may still still have seemingly unnecessary scroll bar.  The scrollbar may have to be the way it is, because we need an extra space for potential suggested tile category displayed under the tile.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8648491 [details] [diff] [review]
v1.  Aviod a row of empty tiles

(didn't look a the patch; I just tested it out)

The patch does seem to avoid an *entire* row of blank tiles, which seems good. E.g. if I make my window 5 tiles wide, then I can reduce it to 2 rows (10 tiles) and I don't see a scrollbar.

However, if my window is only large enough to fit *4* tiles per row, then it adds a 3rd row, to fit just a single tile (Firefox Sync in my case) plus a bunch of blank ones. That still seems silly. Similarly if my window is only large enough fit 3 or 2 tiles per row, Firefox forces a 3rd row and a scrollbar as a result.

This still feels silly. Do we really need to require a 3rd row? We're OK showing 6 tiles if my window is only 2 tiles wide. Why must we show 9 tiles + 3 blank tiles when my window is 4 tiles wide and 2 rows high? (instead of just showing 8 tiles in the 2 rows that I have space for)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: screenshot with patch applied and window only large enough for 2 rows of 4 tiles

(In reply to Daniel Holbert [:dholbert] from comment #5)
> However, if my window is only large enough to fit *4* tiles per row, then it
> adds a 3rd row, to fit just a single tile (Firefox Sync in my case) plus a
> bunch of blank ones. That still seems silly.

Here's a patch demonstrating this. (Do we think this final 9th tile is important enough that it really has to appear on the page & force a scrollbar? I don't think we do, given that we're OK with showing only 6 tiles at skinnier window-widths.)

Comment 7

[Daniel Holbert [:dholbert]]

(Sorry, by "Here's a patch" I meant "Here's a screenshot") :)

Comment 8

[maxim zhilyaev]

Attachment: Screen Shot 2015-08-17 at 8.06.22 AM.png

Comment 9

[maxim zhilyaev]

So, I think it's not a regression, because in aurora we happily show off empty rows of tiles, see attached screen shot (https://bugzilla.mozilla.org/show_bug.cgi?id=1194895#c8).  The nature of Bug 1180387 fix was to show all configured rows for a user empty or not. We used to clip off rows that do not fit into view-port, but then increasing tile space, caused useful rows be clipped off and got a lot of user upset, so we decided to give them a scrollbar to get access to missing tiles.  But then, on empty profiles, a user may see a row of empty tiles, which is not a regression since empty rows are in aurora.

I recommend to split this bug into two:
- no show of an empty row  (we have a fix for that)
- no show of nearly empty row that Daniel used in his examples.

I am roping in the rest of team, especially aaron to get his opinion on UX issues.

Comment 10

[Aaron]

Max,

- I agree that we should not show empty rows for new Firefox users, and 
- that current users with 3 rows - that do not fit in their view-port - should see a scroll bar.

I would like to request one fundamental change:

- Any user, whether new or current, should always see one empty "+" tile ("add a site"). This may mean that we suppress one Directory Tile for new users, or a third row with one empty tile for current users.

The reason I'm asking for this change is so that all users know that they always have the ability to add or edit any site they see on New Tab.

I'll attach examples shortly...

Comment 11

[Aaron]

Attachment: 01 Directory - Tiles Default.png

Example of one empty "+" tile ("Add a site") to Directory Tiles on New Tab... this would mean suppressing one tile.

Comment 12

[Aaron]

Attachment: History - Default 3.png

Example of a third row for a current user with an empty "+" tile ("Add a site").

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to maxim zhilyaev from comment #9)
> So, I think it's not a regression, because in aurora we happily show off
> empty rows of tiles, see attached screen shot
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1194895#c8).

FWIW, that screenshot (blank tiles filling empty space) seems fine to me. That's unrelated to what prompted me to file this bug.

My primary concern when filing this bug is unnecessary scrollbars on the new-tab page -- not blank tiles (though this bug is particularly egregious when the thing causing the scrollbar *is* blank tiles).

> We used to clip off rows that do not fit into view-port, but then increasing
> tile space, caused useful rows be clipped off and got a lot of user upset,
> so we decided to give them a scrollbar to get access to missing tiles.

For users who *haven't* pinned any sites on the new-tab page, though, shouldn't our behavior stay the same?  (I'm assuming the users who were upset were upset about not having access to pinned tiles, right? [due to an expectation that pinned tiles should always be visible on the new-tab page])

> I recommend to split this bug into two:
> - no show of an empty row  (we have a fix for that)
> - no show of nearly empty row that Daniel used in his examples.

I'd suggest refining #2 to "don't creating a tile-row which we know won't fit in the viewport, unless [users may depend on that row being present]".  (I'm not sure what [users may depend on that row being present] means, but I'm using that to cover whatever user upsetness you were hinting at above. I can imagine it involving pinned tiles, perhaps?)

Comment 14

[Marina Samuel [:emtwo]]

> For users who *haven't* pinned any sites on the new-tab page, though,
> shouldn't our behavior stay the same?  (I'm assuming the users who were
> upset were upset about not having access to pinned tiles, right? [due to an
> expectation that pinned tiles should always be visible on the new-tab page])

It's possible that they are upset about missing frecent sites as well, not just pinned.

Perhaps we could define [users may depend on that row being present] as a history (not directory or suggested) tile?

Comment 15

[Kevin Ghim]

Depending on the accumulation of history (frecent) tiles, the behavior should be different. 

For Directory Tiles, it doesn't make sense to have the scroll bar appear with empty tiles showing.

For History Tiles, there is enough evidence that users are attached to the tiles they visit often and want access to it. Once history accumulates and spills into the 3rd row, the scroll bar should appear based on the the screen resolution. 

I'd like to test out the scrolling functionality to gather evidence on future usecases of introducing added functionality below the fold. Can we instrument a telemetry probe to trigger on scroll down?

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Kevin Ghim from comment #15)
> For Directory Tiles, it doesn't make sense to have the scroll bar appear
> with empty tiles showing.

(Again, I don't think "with empty tiles showing" makes a difference here. IMO we shouldn't show a scrollbar if there are only directory tiles, period.)

Comment 17

[maxim zhilyaev]

follow up from team discussion on this bug.

- remove a row of tiles not fitting into a view port if it does contain a history tile
- rows fitting in view-port are always visible even if empty (this is existing behavior)
- since the fix is refactor of 1180387 fix, it's fix to this bug that will be uplifted to aurora/beta instead of 1180387 patches

Comment 18

[Daniel Holbert [:dholbert]]

(I'm assuming "history tile" in comment 17 includes "pinned tile".)

So it seems we have two dueling expectations:
 (a) Users expect no scrollbars/overflow on new tab page. (This is what prompted me to file this bug.)
 (b) Users expect that their own tiles which *used* to fit on the new tab page should *still* fit, even after we increase per-tile space requirements.

Comment 17 seems like a decent compromise between these expectations. (It makes me sad that we'll be violating expectation (a) for most users [anyone who has some history and a smallish window-size], but we'll be in a better state than we are now I guess.)

Comment 19

[Marina Samuel [:emtwo]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> (I'm assuming "history tile" in comment 17 includes "pinned tile".)

yes

Comment 20

[maxim zhilyaev]

Attachment: v1. Complete refactor of _resizeGrid

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Moving tracking flags from bug 1180387 to this one. See https://bugzilla.mozilla.org/show_bug.cgi?id=1180387#c59 for more info.

Comment 22

[Marina Samuel [:emtwo]]

Comment on attachment 8650063 [details] [diff] [review]
v1.   Complete refactor of _resizeGrid

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

I tested this locally and got some test failures: https://pastebin.mozilla.org/8843376 have you tested on try yet?

::: browser/base/content/newtab/grid.js
@@ +180,5 @@
> +   * @param position Position in _links array
> +   */
> +  _isHistoricalTile: function Grid_isHistoricalTile(aPos) {
> +    let site = this.sites[aPos];
> +    return (site) ? site.isPinned() || (site.link && site.link.type == "history") : false;

nit: no need for () around site.

@@ +241,5 @@
> +          // iterate to the next link
> +          startIndex++;
> +        }
> +      }
> +    }

Is there any way we might be able to simplify some of this if we iterated over gGrid.sites instead?

::: browser/base/content/test/newtab/browser_newtab_bug1194895.js
@@ +40,5 @@
> +  // Testing history tiles
> +  yield setLinks("0,1,2,3,4,5,6,7");
> +  yield addNewTabPageTab();
> +
> +  // should do not see scrollbar since tiles fit into visible space

Should not see*

@@ +50,5 @@
> +  yield addNewTabPageTab();
> +  checkGrid("0,1,2,3,4,5,6,7,8");
> +  ok(hasScrollbar(), "document has scrollbar");
> +
> +  // block the last tie away

tie -> tile

@@ +85,5 @@
> +      return aLink2.frecency - aLink1.frecency ||
> +             aLink2.lastVisitDate - aLink1.lastVisitDate;
> +    }
> +    else {
> +      if (aLink2.type == "history") return 1;

Please use {}

Comment 23

[Ed Lee :Mardak]

(In reply to Marina Samuel [:emtwo] from comment #22)
> ::: browser/base/content/newtab/grid.js
> > +    return (site) ? site.isPinned() || (site.link && site.link.type == "history") : false;
> nit: no need for () around site.
return site && (site.isPinned() || site.link && site.link.type == "history");

That should be equivalent other than if site is undefined returning undefined instead of false.

Comment 24

[Alfred Kayser]

Attachment: newtabscrollbar_issue2.png

Dragging tiles from one row to another also causes the scrollbar to appear briefly.
That appearance of the scrollbar make the whole view jumpy, as the scrollbar forces a resize of the view.

The screenshot shows how temporarily an additional row is displayed causing this scrollbar to appear.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Alfred Kayser from comment #24)
> Dragging tiles from one row to another also causes the scrollbar to appear
> briefly.

(I can reproduce this, with current nightly and a 9x9 grid of tiles. If I click and drag e.g. the center-right tile into any other position (e.g. one slot to the left, or one slot up), then a scrollbar & a 4th row of tiles appear for some reason. I don't think there's any reason we should be showing a 4th row at that point.

This might want its own bug, unless Maxim's patch already happens to fix this issue...)

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8650063 [details] [diff] [review]
v1.   Complete refactor of _resizeGrid

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

Just some drive-by comments. 

Also a question about the changes desired here: It seems we want to always show some amount of "History/Pinned" tiles because users expect them, but what number are we supposed to be showing at a minimum? If there is 1 directory tile and we decide the "history/pinned" tiles minimum is 8, then we would show 9 at all times *plus* a blank one? I think we should be reducing the number of tiles by 1 if we are sure we want to show a blank one, instead of adding a blank one at the end of the list which may be on its own row.

::: browser/base/content/newtab/grid.js
@@ +178,5 @@
>    /**
> +   * Test a tile at a given position for being pinned or history
> +   * @param position Position in _links array
> +   */
> +  _isHistoricalTile: function Grid_isHistoricalTile(aPos) {

The comment for this function says "pinned or history" but the function name is "isHistoricalTitle", so the function name seems a bit misleading.

@@ +219,5 @@
> +    // If a user has a pinned or history tile in the row below visibleRows, we
> +    // should accomodate such row by enabling a scroll bar. We check for that
> +    // condition by iterating through grid sites testing for historical tiles
> +    //
> +    // Start with testing for well populated history to avoid links iteration

What is "links iteration"? Does "links iteration" just describe the `else` block below?

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

The question that I asked in the beginning of comment #26 is a bit confusing. Please disregard it.

It looks like the attached patch doesn't add in the requested "+" tile from comment #10. Was this dropped during offline discussions?

Also, can someone share why we no longer shrink/resize the tiles as the viewport changes? That seems like a pretty reasonable solution that would keep the expected tiles on screen without a scrollbar. Is it related to partner requirements?

Comment 28

[maxim zhilyaev]

(In reply to Aaron from comment #10)

> I would like to request one fundamental change:
> 
> - Any user, whether new or current, should always see one empty "+" tile
> ("add a site"). This may mean that we suppress one Directory Tile for new

Filed a separate feature request for that: Bug 1197339

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Looks like it was bug 975208 that requested that tiles no longer shrink.

Comment 30

[maxim zhilyaev]

Attachment: Screen Shot of the failure showing up in beta 41.0b2

Comment 31

[maxim zhilyaev]

(In reply to Daniel Holbert [:dholbert] from comment #25)

> (In reply to Alfred Kayser from comment #24)
> > Dragging tiles from one row to another also causes the scrollbar to appear
> > briefly.
> 
> (I can reproduce this, with current nightly and a 9x9 grid of tiles. 

This problem shows up in beta 41.0b2 (https://bug1194895.bmoattachments.org/attachment.cgi?id=8651207).
It may be possible that 1180387 fix makes the problem show up more or in a different fashion, but i do not have any evidences of that yet.

Comment 32

[Daniel Holbert [:dholbert]]

I spun off bug 1194895 for the issue Alfred brought up in comment 24, since it seems to be independent of this issue.

Comment 33

[Daniel Holbert [:dholbert]]

(sorry, bug-number typo)
I spun off bug 1197345 for the issue Alfred brought up in comment 24, since it seems to be independent of this issue.

Comment 34

[maxim zhilyaev]

Attachment: v2. Numerous fixes per reviewer comments and manual QA

Per reviewer comments
- refactors history tile finding logic
- fixed a problem with incorrectly computed vertical available space due to search-container vertical margins change
- reworked tests to pass on screens with different DPIs
- added logic to measure and account for vertical scrollbar width for grid resize

As far I can tell, the newtab behaves proper without jitter.

Comment 36

[maxim zhilyaev]

Attachment: v3. fixed jitter by explicitly setting grid width to fit computed columns

Comment 37

[maxim zhilyaev]

Attachment: Newtab right before the third column is lost

Comment 38

[maxim zhilyaev]

Attachment: Newtab right after third column is shown again

Comment 39

[maxim zhilyaev]

Marina and myself took a hard look into various failure cases of this bug. We believe the current patch is a good candidate for landing and uplift: it handles display of historical tiles proper and avoids layout jitter occurring at column-width boundaries.  However, it still has an issue which, we think, is minor and should be handled as a follow up task.

The issue is that a user looses a column and gains a column back at different browser widths: 
- When a user shrinks newtab horizontally, he losses a column when width is 950px (https://bug1194895.bmoattachments.org/attachment.cgi?id=8652154)
- To gain back the the column, a user has to stretch the window to 1060px (https://bug1194895.bmoattachments.org/attachment.cgi?id=8652156)
- If a user opens another tab, the tiles are displayed proper

This behavior is a consequence of how we avoid layout jitter.
Given time constrains for beta uplift, we would like to land the current patch. 
I will file a follow up bug after landing.

Comment 40

[Marina Samuel [:emtwo]]

Comment on attachment 8652142 [details] [diff] [review]
v3.  fixed jitter by explicitly setting grid width to fit computed columns

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

Minor comments but looks good to me overall.

::: browser/base/content/newtab/grid.js
@@ +180,5 @@
> +   * @param position Position in sites array
> +   */
> +  _isHistoricalTile: function Grid_isHistoricalTile(aPos) {
> +    let site = this.sites[aPos];
> +    return site ? site.isPinned() || (site.link && site.link.type == "history") : false;

See comment 23 for simplifying this a bit.

@@ +218,5 @@
> +    let visibleRows = Math.floor(availHeight / this._cellHeight);
> +
> +    // Find the number of columns that fit into view port
> +    let maxGridWidth = gGridPrefs.gridColumns * this._cellWidth + GRID_WIDTH_EXTRA;
> +    // available width is current gird width, but no greater than maxGridWidth

gird -> grid

@@ +219,5 @@
> +
> +    // Find the number of columns that fit into view port
> +    let maxGridWidth = gGridPrefs.gridColumns * this._cellWidth + GRID_WIDTH_EXTRA;
> +    // available width is current gird width, but no greater than maxGridWidth
> +    let availWidth = Math.min(document.querySelector("#newtab-grid").clientWidth,

Comparing to the way availHeight is computed, why can't we compute availWidth similarly? i.e. document.documentElement.clientWidth or similar?

@@ +224,5 @@
> +                              maxGridWidth);
> +    // finally get the number of columns we can fit into view port
> +    let gridColumns =  Math.floor(availWidth / this._cellWidth);
> +
> +    // walk sites backwords until a pinned or history tile is found or visibleRows reached

backwords -> backwards

@@ +235,5 @@
> +    }
> +
> +    // tileIndex now points to a historical tile with highest index
> +    // or to the last index of the visible row, if we found none
> +    let gridRows = Math.floor(tileIndex / gridColumns) + 1;

Can you be more specific in the variable name and/or comment about what gridRows is vs. visibleRows?

Looks like visibleRows is the number of rows that can fit on screen whereas gridRows is how many rows we'll actually show (which may require adding a scrollbar).

@@ +241,5 @@
> +    // we need to set grid width, for otherwise the scrollbar may shrink
> +    // the grid when shown and cause grid layout to be different from
> +    // what being computed above. This, in turn, may cause scrollbar shown
> +    // for directory tiles with no history tile, and introduce jitter when
> +    // grid width is alligned exactly at the column boundary

alligned -> aligned

@@ +242,5 @@
> +    // the grid when shown and cause grid layout to be different from
> +    // what being computed above. This, in turn, may cause scrollbar shown
> +    // for directory tiles with no history tile, and introduce jitter when
> +    // grid width is alligned exactly at the column boundary
> +    this._node.style.width = gridColumns * this._cellWidth + 30 + "px";

What is 30? Please make it into a const.

Comment 41

[maxim zhilyaev]

Attachment: v4. reviewer comments

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/02b72fdb6802

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02b72fdb6802

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Daniel, could you please verify the fix on 8/27 Nightly or later? Thanks.

Comment 45

[Daniel Holbert [:dholbert]]

Can't for a few weeks, sorry - I'm on vacation and away from computers. (Typing this from my phone)

Hopefully someone else can verify?

Comment 46

[maxim zhilyaev]

Attachment: v1. Aurora patch for 1194895 and 1195699

Approval Request Comment

[Feature/regressing bug #]:
Main bug 1180387 - New Tab Page doesn't show websites (wrong number of rows or lines) 
Blocking bugs
Bug 1194895 - New tab page has unnecessary (or marginally-necessary) vertical scrollbar at various sizes 
Bug 1195699 - Port tests fixes from 1180387 uplift

Notes:
- 1180387 changes went through complete refactor in 1194895 fix, hence only 1194895 needs uplift
- 1195699 contains fixes to tests and needs to be uplifted along with 1194895 patch.  The attached patch contains fixes for both bugs.
- Bug 1195321 is a regression of 1180387, I will attach a separate patch to that bug requesting aurora uplift

[User impact if declined]:
Impact is significant as many users disliked Mozilla taken away their third row of tiles. Users are waiting for the fix.  See 1180387 for heated details.
[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab58a4b34f15

[Risks and why]: 
Thoroughly tested manually, added new mochitests to cover placing and removal of scrollbar.  Risk may exists for corner cases that we have not seen yet.  However, until community points us at the problem we are not aware of it, we did not observe horrid UX issues during manual testing

[String/UUID change made/needed]: NONE

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654658 [details] [diff] [review]
v1. Aurora patch for 1194895 and 1195699

Plenty of tests, will make some people happy, taking it for 42.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Please fix your Mercurial config to generate patches with usable commit information.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63e48f89ff5c

Comment 50

[maxim zhilyaev]

Attachment: v2. aurora patch with meruciral commit message

Attached a patch with mercurial commit message - appologies

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #45)
> Can't for a few weeks, sorry - I'm on vacation and away from computers.
> (Typing this from my phone)
> 
> Hopefully someone else can verify?

Sorry. Have a good vacation. :) I did not mean for you to verify right away. Any time after you are back also works to ensure we have covered all the issues.

Comment 52

[Daniel Holbert [:dholbert]]

OK, thanks. I'll take a look once I'm back.

Comment 53

[Daniel Holbert [:dholbert]]

Notes on verification:
 - In a pretty-fresh profile, New Tab page is much more responsive to window-size now. Even after I've populated the history a bit (visiting ~10 different domains), it's happy to show only (for example) a single row of 4 tiles, to fit a short-and-wide Firefox window.

 - But, in my main browsing profile (with lots of history, no blank tiles, just one pinned tile), Firefox insists on always having *at least* 3 rows of tiles on my new tab page, even if the window is shorter than that, and even if the window is  wide enough for like 5 columns. So at most non-fullscreen window-sizes, I'm still ending up with a scrollbar on my new tab page where I wasn't previously.  This still seems odd to me, but I think this is maybe the new expected behavior (though I'm not quite clear on why the fresh-profile-with-a-bit-of-history is more forgiving of short windows. Maybe it thinks most of the history entries aren't tile-worthy?)

needinfo=maxim to sanity-check that this is the correct expected behavior.

Comment 54

[Daniel Holbert [:dholbert]]

> So at most non-fullscreen window-sizes, I'm still ending up with
> a scrollbar on my new tab page where I wasn't previously.

(To be clear, by "previously" here I mean "before this bug was introduced".)

Comment 55

[maxim zhilyaev]

There's browser.newtab.rows preference that controls number of tile rows to show.
"previously" we clipped the bottom rows if they did not fit into view port.

Then we increased the height of the tile to accommodate for some explanation text for targeted tiles displayed under the tile.  This, indeed, made fewer rows fit into the view port and many user got irritated about losing their beloved tiles.

So, we had to place a scroll bar for those who grieve their lost tiles can scroll down to them.
But then if a user has a history tile in a bottom row(s), the scroll bar will always be visible.

Regarding the weird behavior of the fresh profile: "I've populated the history a bit (visiting ~10 different domains), it's happy to show only (for example) a single row of 4 tiles, to fit a short-and-wide Firefox window."

What happens is that directory tiles sometime win over history tiles when newtab figures out the order of tiles.  If you just click on a page link, this pages goes into places database with default frecency=100.  A directory tile has default frecency=1000.  Even though you did click on 10 sites, the directory tiles still occupy top rows of the newtab page, hence when you reduce widnow size you loose tile rows because they DO NOT contain any history tile and therefore do not deserve a scrollbar.

This gets even more weird, when you consider that a user has to click 10 times on the same url to get the page a sporting chance to win over a directory tile.  I suspect that for many users after they start with new profile - all they see are directory tiles over-and-over again, until their history matures enough to push history tiles on top.

Comment 56

[Daniel Holbert [:dholbert]]

(In reply to maxim zhilyaev from comment #55)
> There's browser.newtab.rows preference that controls number of tile rows to
> show.

Gotcha -- though I think you mean "browser.newtabpage.rows" (not "newtab"). That seems to set the maximum number of rows to display (so if I set it to "1", I get only 1 row, ever).  Per our IRL discussion just now, what I *really* want is to restore the old behavior of "hide rows if they don't fit [regardless of whether they have a history tile], instead of overflowing and creating a scrollbar".

(Maybe I'll file a new bug on that, and you or somebody can add a pref if you get a chance? I suspect I may not be the only user who wants to restore this behavior. :))

> Regarding the weird behavior of the fresh profile

Thanks, that explains the difference I was seeing there.

Comment 57

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #56)
> (Maybe I'll file a new bug on that, and you or somebody can add a pref if
> you get a chance? I suspect I may not be the only user who wants to restore
> this behavior. :))

Filed bug 1204955 for this, BTW.