980014 - Allow new tab grid layout to reduce rows/columns if the window can't fit 3x3

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Per bug 975208 design we will try to display a 3x3 grid with a gutter around each tile. If there is not enough space for the fixed size 243x150 tile and 32px gutters, we remove rows and/or columns.

Comment 1

[Ed Lee :Mardak]

adw, do you have any thoughts on how we should approach this? I see that there's the gridPrefs object watching for browser.newtabpage.rows and browser.newtabpage.columns. And I assume there might be issues with the pre-rendering.

I wonder if there's any good way for css to automatically reflow and hide. It seems that we could just make a box of the appropriate size (big enough to fit 3x3 or small enough to fit the window) and display the tiles inline/wrapping as necessary.

Potentially we could still honor the rows/columns prefs in the calculation of the appropriate size box. Where the pref is more of a guideline -- "show up to this many rows/columns."

Comment 2

[Drew Willcoxon :adw]

That sounds ideal, but I'm not sure it's possible to control the number of rows shown with that approach.  It would let you control the number of columns, but not the number of rows I think, which the design in bug 975208 mandates.  It'd be great if I'm wrong, though.  Maybe you could make it work by listening for overflow and underflow events?

It might be more straightforward to listen for the resize event and manually update the DOM appropriately.

(In reply to Ed Lee :Mardak from comment #1)
> Potentially we could still honor the rows/columns prefs in the calculation
> of the appropriate size box. Where the pref is more of a guideline -- "show
> up to this many rows/columns."

I think that's fine.  Don't let those prefs or the current design handcuff you.

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Comment 4

[Ed Lee :Mardak]

Attachment: v2

mzhilyaev fixed an issue with max-height not being set on additional new-tab pages by adding a load event listener. I wonder if bug 972341 would simplify some codepaths by just checking for visibilitychange to call updateHeight?

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8392550 [details] [diff] [review]
v2

>+    this._node.style.maxWidth = gGridPrefs.gridColumns * CELL_WIDTH + "px";
One of mzhilyaev's profiles seem to be incorrectly showing 2 columns on first rendering. The problem goes away on subsequent newtab pages as well as if we manually change maxWidth to +1 (832px instead of 831px). We're supposed to be showing 3 cells each 277px wide that fit exactly, but there might be some rounding errors somehow on a set of browsing history... This sounds like the example here https://wiki.mozilla.org/Mozilla2:Units#Problems_with_the_status_quo fixed by bug 177805.

I'm not sure if we just // XXX +1 to the maxWidth or perhaps +1 to CELL_WIDTH?

Comment 6

[Ed Lee :Mardak]

Comment on attachment 8392550 [details] [diff] [review]
v2

adw, after looking at the telemetry probes, it seems like we probably shouldn't add a new "load" event listener and instead should use the _mutationObserver in page.js?

Also, is there a good way to avoid the clientHeight == 0 check for when the page is initialized (prerendered?)

Grid_updateHeight@chrome://browser/content/newtab/newTab.js:545:5
Grid_render@chrome://browser/content/newtab/newTab.js:619:7
Grid_init@chrome://browser/content/newtab/newTab.js:492:5
Page_init/<@chrome://browser/content/newtab/newTab.js:376:7
executeCallbacks@resource://gre/modules/NewTabUtils.jsm:596:13
Links_populateCache@resource://gre/modules/NewTabUtils.jsm:605:7
Page_init@chrome://browser/content/newtab/newTab.js:386:1
Page_init@chrome://browser/content/newtab/newTab.js:299:7
@chrome://browser/content/newtab/newTab.js:2005:1

Comment 7

[Ed Lee :Mardak]

I have some local commits that updates the height from _mutationObserver, and there's a noticable change in height as the newtab page is opened. This doesn't happen when setting the height from a "load" event.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8392550 [details] [diff] [review]
v2

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

This looks OK, but I have a question about the clientHeight part.

(In reply to Ed Lee :Mardak from comment #5)
> first rendering. The problem goes away on subsequent newtab pages as well as
> if we manually change maxWidth to +1 (832px instead of 831px). We're
> supposed to be showing 3 cells each 277px wide that fit exactly, but there
> might be some rounding errors somehow on a set of browsing history...

It's got to be impossible for browsing history to affect a width somewhere.  Are the browser windows in the profiles the exact same size?

Adding some fudge factor like 1 to maxWidth sounds fine to me if it fixes the problem.  As long as the fudge is < cell width, it shouldn't ruin layout, right?

(In reply to Ed Lee :Mardak from comment #6)
> adw, after looking at the telemetry probes, it seems like we probably
> shouldn't add a new "load" event listener and instead should use the
> _mutationObserver in page.js?

load is what you want I think, because doing it in the mutation observer would cause a jarring, visible change to the page.  Same thing if we listened for visibilitychange instead.  We don't want to make jarring changes to the page after it's already visible.

> Also, is there a good way to avoid the clientHeight == 0 check for when the
> page is initialized (prerendered?)

Even when the page is preloaded and hidden, clientHeight should be > 0.  The two things should be independent.  More below.

::: browser/base/content/newtab/grid.js
@@ +191,5 @@
> +   */
> +  _updateHeight: function Grid_updateHeight() {
> +    // Don't bother setting maxHeight before rendered
> +    let {clientHeight} = document.documentElement;
> +    if (clientHeight == 0) {

In what cases is clientHeight == 0?  (The "rendered" in the code comment a few lines up is hand-wavey, so at a minimum the comment should be made more precise.)  I can't see how that would happen, and I tested with a load listener and clientHeight was always > 0.  For reference, the preloader creates and sizes its hidden browsers here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/BrowserNewTabPreloader.jsm#372  By the time the page receives the load event, the browser should be appropriately sized.

If you really did see cases where clientHeight == 0, when did it subsequently become > 0?  Was the height correctly updated at that time, and if so was the page already visible?

Comment 9

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #8)
> Adding some fudge factor like 1 to maxWidth sounds fine to me if it fixes
> the problem.  As long as the fudge is < cell width, it shouldn't ruin
> layout, right?
Sounds good. The most this could change is the grid is off-center by 1px.

> load is what you want I think
Yeah, from my tests, doing things on "load" causes it to be sized correctly by the time it's shown.

> In what cases is clientHeight == 0?
The stack I included in comment #6 had clientHeight == 0.

This is triggered from the top-level newtab.js script as it's executed before "load" happens:
gPage.init() -> gPage._init() -> gLinks.populateCache() -> .. -> gGrid.init() -> gGrid._render() -> gGrid._updateHeight()

Technically, we don't need this special check for clientHeight == 0 as css will drop it for being a negative maxHeight. I figured it would be better to be explicit check than implicit noop. If you think it'll be less confusing to not have a hand-wavey comment for code that isn't necessary, I can remove it.

Comment 10

[u279076]

Ioana, can you please assign this to someone on your team for sign-off in this iteration?

Comment 11

[Ioana (away)]

Cornel is already working on Tabbed Browser, so he will take care of this.

Comment 12

[Drew Willcoxon :adw]

I see, thanks for explaining.  We could avoid clientHeight == 0 altogether if we eliminate that path... seems like we shouldn't be trying to build the grid at all before load happens.  If you change this line: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.js#58

to do gPage.init() on window load, does that break anything, including tests?  It looks like that ought to be OK, but I don't know for sure.

Comment 13

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Ed Lee :Mardak from comment #4)
> Created attachment 8392550 [details] [diff] [review]
> v2
> 
> mzhilyaev fixed an issue with max-height not being set on additional new-tab
> pages by adding a load event listener. I wonder if bug 972341 would simplify
> some codepaths by just checking for visibilitychange to call updateHeight?

Would it be possible to get a build with this for a ui review?

Comment 14

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #12)
> do gPage.init() on window load, does that break anything
Naively, it breaks various unit tests that I'm assuming also run in page load, so there's some race condition. https://tbpl.mozilla.org/?tree=Try&rev=85e15901ba3c

I'm testing now if only delaying gGrid._render to "load" doesn't cause test failures. Alternatively we could update the tests to wait for a gPage.init-ed event before the main test logic run.

Comment 15

[Ed Lee :Mardak]

Attachment: v3

Comment 16

[Ed Lee :Mardak]

Elbart, can you check if a build from bug 973273 comment 4 fixes your issue that you raised in bug 978338 comment 9?

Comment 17

[Ed Lee :Mardak]

Attachment: v4

Get rid of some consts that can be read calculated from getComputedStyle and offsetHeight/Width. Remove the flexible height of the top-margin (to fit the undo box) as the spacer will stretch.

Comment 18

[Ed Lee :Mardak]

Comment on attachment 8395306 [details] [diff] [review]
v4

Actually... maybe not quite yet ready for r?. Seems like mochitests are failing because delaying gGrid._render sets gGrid.cells, so tests trying to access that aren't guaranteed.

Comment 19

[Elbart]

(In reply to Ed Lee :Mardak from comment #16)
> Elbart, can you check if a build from bug 973273 comment 4 fixes your issue
> that you raised in bug 978338 comment 9?

Only four rows and columns are shown with that build, even when I set them to higher values.

Comment 20

[Ed Lee :Mardak]

Attachment: v5

Move the load/render-dependent code (documentElement.clientHeight, cell.offsetHeight, etc.) to after "load" event so height computation never happens too early.

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 8395492 [details] [diff] [review]
v5

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

So the tests pass with this, no changes to them or newTab.js necessary?  And no jarring visible grid resizes, right?

::: browser/base/content/newtab/grid.js
@@ +74,5 @@
> +        this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop) * 2;
> +        this._cellHeight = refCell.offsetHeight + this._cellMargin;
> +        this._cellWidth = refCell.offsetWidth + this._cellMargin;
> +
> +        // fallthrough to resize

Fall-throughs make me nervous, and it's only one line, so could you please call _resizeGrid here and break?

@@ +135,5 @@
>    },
>  
>    /**
> +   * Calculate the height for a number of rows up to the maximum rows
> +   * @param rows Number of rows defauling to the max

defaulting

@@ +137,5 @@
>    /**
> +   * Calculate the height for a number of rows up to the maximum rows
> +   * @param rows Number of rows defauling to the max
> +   */
> +  _computeHeight: function Grid_computeHeight(rows) {

Since this returns a px string, please either call this _computeCSSHeight, or better yet make it return a number to which the caller appends "px".

Nit: aRows to fit the style here

@@ +139,5 @@
> +   * @param rows Number of rows defauling to the max
> +   */
> +  _computeHeight: function Grid_computeHeight(rows) {
> +    let {gridRows} = gGridPrefs;
> +    rows = rows == null ? gridRows : Math.min(gridRows, rows);

Nit: rows = rows === undefined ? ...

Comment 22

[Ed Lee :Mardak]

Attachment: for checkin

(In reply to Drew Willcoxon :adw from comment #21)
> So the tests pass with this, no changes to them or newTab.js necessary?  And
> no jarring visible grid resizes, right?
Correct. Except for if the user resizes the window /then/ opens a newtab as the pre-rendered grid still has the old height when initially becoming visible. All other consecutive newtab openings have no jumping.

Comment 23

[Ed Lee :Mardak]

To be clear, the jump only happens if the user resized the height enough to change how many rows get shown. If the same number of rows would be shown as when the grid was pre-rendered, the grid will still be correctly centered on first visibility.

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Jenn Chaulk (:jchaulk)]

Hi Cornel, can you please advise when verification will be complete on this iteam, as our iteration is ending on Monday, March 31st.

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.3; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Verified as fixed on latest Nightly (Build ID: 20140326030203).
The number of rows/columns is correctly reduced if the window is resized and it can't fit 3x3 (the default value).

Also verified with browser.newtabpage.columns and browser.newtabpage.rows set to several other values: 2/2, 3/2, 4/4, 4/3 and 6/6 and got the expected result. If the window could not fit an entire row or a column, it was reduced.

Comment 27

[User Dderss]

In FireFox 31 new tab screen has reduced number of columns from 4 to 3, even though there is GIANT space to fit everything.

How can I switch off this feature so it would not cripple new tab?

Comment 28

[Ed Lee :Mardak]

(In reply to User Dderss from comment #27)
> In FireFox 31 new tab screen has reduced number of columns from 4 to 3, even
> though there is GIANT space to fit everything.
Is your preference set to 4? Are you saying there's giant space on the sides to fit another column or are you saying there's space above/below to fit more rows? The fix for this bug should be maintaining full 4 columns if that's what you set in your preference as long as there's enough space to fit in another column.

Comment 29

[User Dderss]

Here is what I see as of recent: http://i83.photobucket.com/albums/j284/denisrs/3x3coloumns.png

There is giant space both for row and coloumn, though I only use additional coloumn ("browser.newtabpage.columns" is set to 4 the whole time, number of rows is set by default to 3).

There has to be some way to force FF 31 to show four coloumns in this case.

Comment 30

[Ed Lee :Mardak]

(In reply to User Dderss from comment #29)
> > There is giant space both for row and coloumn
Your screenshot seems to have enough space for additional columns, can you make sure you're running Nightly 31.0a1 (2014-03-26) or newer?

Comment 31

[User Dderss]

Yes, it is latest build: 31.0a1 (2014-03-27). In see the glitch for about three days or something (I update and restart FF daily).

Comment 32

[Cornel Ionce [:noni] [Hubs QA]]

I cannot reproduce this. If the "browser.newtabpage.columns" is set to 4 then 4 columns are shown in the same Nightly build.

Are you using the en-US build?

Also, please verify if you can still reproduce it using a clean profile.
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 33

[User Dderss]

I found out what the issue is: starting from few years ago, FF 31 has started to count actual number of coloumns to display not with 1, but with 0 (aka "base 0" directive). So to mitigate the glitch in my case I had to manually set number for coloumns to 5 (five) to get 4 (four) visible coloumns.

I tested it an empty/clean profile, and "newtab" only shows 2 (two) coloumns with default 4 (three) value set in "browser.newtabpage.columns".

As far as I know there is no other builds than "en-US" for Nightly. But I use 64-bit Windows version, which is not officially supported, so this glitch can be exclusive to this version. Thus I suppose the bigger issue here is not the glitch per se but the very fact that -- amazingly -- Mozilla does not care to support 64-bit Windows versions at the time when even leading smartphones have fully 64-bit CPUs, OSes and browsers. I was shocked when I first learn about it; I literally can not remember a more backwards move, even though I watch software development for thirty years.

Comment 34

[User Dderss]

CORRECTION: "I found out what the issue is: starting from DAYS years ago"; sorry.

Comment 35

[Ed Lee :Mardak]

(In reply to [e-mail] from Bug 975208 comment #15)
> I've set mine with 6 columns and 5 rows, which can easily fit to my window,
> and since 3 days ago, it shows me 6 colums and 4. It doesn't even show 5th
> row when I zoom it out, and it changes it to 5x5 instead...
> 
> I just want my 6x5... How to make it work?
The zooming out does seem to have some odd behavior, but just checking, when you say "can easily fit to my window," is there enough whitespace to fit a whole row or compared to before it used to fit 5 but now with the fixed height tiles, it only fits 4.

Comment #33 in this bug mentions using a 64-bit windows build and needed to set the number of rows to be 1 larger than desired. Does that do anything for you?

Comment 36

[User Dderss]

Edward, could you please say if there are plans to make previews in the tiles up to date? In my practice pictures in the tiles do not get updated if I visit the site; tiles show snapshots like from a week or two ago. Those tiles get updated very rarely, and this is annoying. FF should check if user accesses any of the sites that in the tiles -- no matter whether by using "newtab" page or otherwise -- and immediately update the tile snapshot picture absolutely every time, not like it happens now. Thanks in advance.

Comment 37

[[e-mail]]

(In reply to Ed Lee :Mardak from comment #35)
> (In reply to [e-mail] from Bug 975208 comment #15)
> > I've set mine with 6 columns and 5 rows, which can easily fit to my window,
> > and since 3 days ago, it shows me 6 colums and 4. It doesn't even show 5th
> > row when I zoom it out, and it changes it to 5x5 instead...
> > 
> > I just want my 6x5... How to make it work?
> The zooming out does seem to have some odd behavior, but just checking, when
> you say "can easily fit to my window," is there enough whitespace to fit a
> whole row or compared to before it used to fit 5 but now with the fixed
> height tiles, it only fits 4.
> 
> Comment #33 in this bug mentions using a 64-bit windows build and needed to
> set the number of rows to be 1 larger than desired. Does that do anything
> for you?

Yes there is enough space, I not to mention when I close bookmarks tab, which I don't need when I have 6x5..

Comment 38

[Elbart]

(In reply to Ed Lee :Mardak from comment #35)
> or compared to before it used to fit 5 but now with the fixed
> height tiles, it only fits 4.
Exactly: http://imgur.com/a/EFtEl

Comment 39

[Alfred Kayser]

The cellMargin is also removed from the availSpace, whilst is also in the _cellHeight itself (which the cell height including top margin twice).

        this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop) * 2;
        this._cellHeight = refCell.offsetHeight + this._cellMargin;

    let availSpace = document.documentElement.clientHeight - this._cellMargin -
                     document.querySelector("#newtab-margin-top").offsetHeight;
    let visibleRows = Math.floor(availSpace / this._cellHeight);

this._cellMargin has value 32 (px) as the cell margin is 16px.
So, removing 32px from the total height available, before dividing by the total cellHeight, means that besides the margins of the cells also 32px is "lost".

Comment 40

[Ed Lee :Mardak]

The design from bug 975208 was to have the same amount of margin/gutter between cells as well as to the edge of the window. There are flex elements around the grid that are at least 16px, so that matches up with the this._cellMargin x2 value. And there's also the space at the top for the undo box contained in "#newtab-margin-top".

Comment 41

[Ed Lee :Mardak]

Alfred, is there something wrong/odd that got you to look into the calculation in the first place? Or just trying to figure out why rows started to disappear when there was still a little bit of space for a row?

Comment 42

[Alfred Kayser]

My comment is about the extra space at the bottom and top that cause the number of rows to be reduced quicker than one would expect. See comment #38.

Comment 43

[Ed Lee :Mardak]

From the last image from comment 38, there's 128px above the tiles, 704px from top of top tile to bottom of bottom tile, 91px below the tiles, so that's a total document.clientHeight 923px.

Each tile is 150px + 1px * 2 (borders) + 16px * 2 (margins) = 184px.

If we used the full 923px space just for tiles, that would fit 5.016 tiles. But we needed to reduce the available space for the "undo box" (40px) as well as the extra gutter above/below so that there's 32px from the bottom of the tile to the bottom of the window.

I suppose there could be an argument for removing the top gutter as most of the time, there won't be a undo box. We just want to make sure it doesn't end up too crowded when the undo box does appear.

Comment 44

[Ed Lee :Mardak]

To be clear, it seems that the designed functionality has been correctly implemented. And as a heads up, the design (layout) is in the process of changing to add a search field above the tiles. See https://bug962490.bugzilla.mozilla.org/attachment.cgi?id=8401003

Comment 45

[User Dderss]

Sorry, but today's and yesterday's Nightly updated has causes new tab site previews completely gone for me, no matter what number I set in "about:config" for this: default 3x3 or any of 1x1, 2x2, 4x4, 5x5.

Interestingly enough, if I leave a new empty tab open and in a parallel tab edit those raws/coloumns figures, page previews DO appear in that empty new tab once I switch to it to check its state. However, if I close it and open a new tab again, I have blank space and no page previews.

How to force FF to show previews as it could two days ago, before couple of latest builds?

Comment 46

[Marat Tanalin]

Just updated to Firefox 31 beta and discovered some of my tiles are not visible:

I use 6x4 matrix for a while via about:config's `browser.newtabpage.rows`/`browser.newtabpage.columns` options, but starting from Fx31beta the layout has changed to 5x4, and I have last four tiles invisible.

The feature is totally broken and should be reverted to Fx30 state as soon as possible.

`browser.newtabpage.rows` and `browser.newtabpage.columns` should not be ignored. There should at least be a way to specify tiles count predictably.

See bug 1005596 and bug 998058.

Comment 47

[u279076]

FYI, this bug has introduced regression bug 1000097.

Comment 48

[Ricardo]

Hello
I started testing Beta channel (v33 currently) and found this change very disgusting.

I would like to know the reasoning for this: "The tiles have fixed size".

It doesn't make sense for me to have this as fixed size.

It also looks even more strange to make it cut off extra rows if they don't fit the window. At least I would expect it to enable window scrolling.

Furthermore, the tile size is too big, wasting a lot of screen space which could be used to display more rows/columns.

Comment 49

[u279076]

Ed should be able to answer your question re: reasoning.

Comment 50

[User Dderss]

The fact that there is gear in the upper right corner and YET there are no flexibility options in it, is appalling. The way how "new tab grid" works now could be fine for many, but utterly inconvenient and mostly useless for me. There is just no way to do anything useful with it as it is now (Nightly, FF 35).

Unless one is Steven Jobs or something, he/she should not completely cut the flexibility. Move it to the gear/options, not throw it away altogether.

Edward, please help users.

Comment 51

[Ricardo]

Even more, "flexibility" is a keyword nowadays; we are in a responsive design era.

Therefore, having web page elements with fixed size doesn't make any sense.

Newtab page worked pretty well on this aspect until v32 (well, disregarding some excessive margins, imo).

Moreover, this change automatically deprecates the options browser.newtabpage.rows and browser.newtabpage.columns.

Comment 52

[Ed Lee :Mardak]

(In reply to Ricardo from comment #48)
> Furthermore, the tile size is too big, wasting a lot of screen space which
> could be used to display more rows/columns.
We tried to increase the number of rows/columns especially for larger screens in bug 1026561, but there were concerns of having too many options for people to choose from. There were also some potential performance concerns for showing more tiles, but that could potentially be avoided with bug 1081536.

The fixed ratio tiles came from two different design specs -- initially for directory tiles bug 975208 then enhanced tiles bug 1030832.