1146249 - Tiles on the newtab page don't wrap properly

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot of issue

Regression from bug 1126188, confirmed by locally backing out. There used to be two rows of four tiles.

Comment 1

[Ed Lee :Mardak]

ttaubert, are you zoomed out? Does it fix itself if you resize the window?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

No zoom. Properly aligned when I make the window a little smaller but distorted again when going back to the full screen size.

Comment 3

[Ed Lee :Mardak]

I can reproduce this by changing about:config newtab columns pref to 4 and making the window larger on an external screen.

emtwo, the bug is here:

https://hg.mozilla.org/mozilla-central/rev/41d6e4de79be#l1.32

In particular:

    1.33 +      this._cellMargin = parseFloat(getComputedStyle(refCell).marginTop) +
    1.34 +        parseFloat(getComputedStyle(refCell).marginBottom);
    1.36        this._cellWidth = refCell.offsetWidth + this._cellMargin;

The height/width calculations assumed the margins were similar on top and sides. But now it has a much larger bottom margin.

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Comment 5

[Ed Lee :Mardak]

Attachment: before

Comment 6

[Ed Lee :Mardak]

Attachment: v1 screenshot

Comment 7

[Ed Lee :Mardak]

ttaubert, does this build fix this bug for you? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6c5947aadce7/try-macosx64/firefox-39.0a1.en-US.mac.dmg

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Didn't try the build but just applied your patch locally. WFM, thanks!

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8581483 [details] [diff] [review]
v1

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

I'm not sure this is right either.  It fixes the

  this._cellWidth = refCell.offsetWidth + this._cellMargin;

line, but it breaks the availSpace assignment, no?

Seems like what we want is:

if (!this._cellVerticalMargin) {
  let cellNonBottomMargin = parseFloat(getComputedStyle(refCell).marginTop);
  let cellBottomMargin = parseFloat(getComputedStyle(refCell).marginBottom);
  this._cellVerticalMargin = cellNonBottomMargin + cellBottomMargin;
  this._cellHeight = refCell.offsetHeight + this._cellVerticalMargin;
  this._cellWidth = refCell.offsetWidth + cellNonBottomMargin;
}
let availVerticalSpace = document.documentElement.clientHeight -
                         this._cellVerticalMargin -
                         document.querySelector("#newtab-search-container").offsetHeight;

Let me know if I'm wrong.

Comment 11

[Ed Lee :Mardak]

Attachment: v1 screenshot minimal 2 row height

(In reply to Drew Willcoxon :adw from comment #10)
> I'm not sure this is right either.  It fixes the
>   this._cellWidth = refCell.offsetWidth + this._cellMargin;
> line, but it breaks the availSpace assignment, no?
You are indeed right that the availSpace computation is not the same as before. But it's fine that availSpace is computed as:

[document height] - [one cell margin] - [search container height]

The availSpace computation is used to figure out how many rows can fit in the flexible space, so that's why the fixed height of the search container is removed. Additionally a margin's worth of space is subtracted to provide a minimum amount of extra whitespace above/below the tiles.

There's a couple reasons for the smaller margin is fine:
- there's a larger margin below each tile to allow for suggested text
- the search container has its own fixed margins to leave a gap before tiles

I've attached what things look like when the window is resized to just barely fit 2 rows.

Comment 12

[Ed Lee :Mardak]

Comment on attachment 8581483 [details] [diff] [review]
v1

See comment 11 for why availSpace computation is okay.

Comment 13

[Drew Willcoxon :adw]

OK, thanks Ed.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b514f389bf7

Comment 15

[Yohann Jardin]

With this modification, I can only see two row unless being in fullscreen, while I was able to see three rows previously.
In addition, the height needed to display 3 rows in fullscreen fit in the height available without being in fullscreen.

Comment 16

[Yohann Jardin]

Attachment: Display 2 rows having space for 3

Comment 17

[Yohann Jardin]

Attachment: Display in fullscreen, 3 rows showing correctly

Comment 18

[Guilherme Lima]

Yohann, that's probably bug 1146146. The patch just landed on mozilla-central. It's gonna be on tomorrow's Nightly.

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reassigning to Karl Thiessen as he seems to be the designated QA contact for fixes related to bug 1120311.

Comment 20

[Karl Thiessen [:kthiessen, he/him]]

Got it.  Florin, I've mostly been involved in server-side testing for Tiles up to now.  We should probably meet up to see if we can find a division of client/server side work that works for everybody.

Comment 21

[Karl Thiessen [:kthiessen, he/him]]

I apologise if my putting my name in the Wiki stepped on any toes -- I am certainly open to sharing responsibility here, and Cornel is doing fine on the client side, per Ed Lee, so I see no reason to take him off this bug.

Comment 22

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Karl Thiessen [:kthiessen] from comment #21)
> I apologise if my putting my name in the Wiki stepped on any toes -- I am
> certainly open to sharing responsibility here, and Cornel is doing fine on
> the client side, per Ed Lee, so I see no reason to take him off this bug.

Thanks Karl for the info, we'll have Cornel monitor and work on fixes for the suggested tiles features. Setting this as qe-verify+,

Comment 23

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-beta/rev/6892b485a7e0

Comment 24

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

The tiles are now properly displayed (default/custom preferences) on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using:
- latest Nightly, build ID: 20150406030204;
- latest Aurora, build ID: 20150407004006
- Firefox 38 beta 2, build ID: 20150406174117.