Closed Bug 1146249 Opened 5 years ago Closed 5 years ago

Tiles on the newtab page don't wrap properly

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: ttaubert, Assigned: Mardak)

References

Details

(Keywords: regression, Whiteboard: .?)

Attachments

(7 files)

Attached image Screenshot of issue
Regression from bug 1126188, confirmed by locally backing out. There used to be two rows of four tiles.
Whiteboard: .?
ttaubert, are you zoomed out? Does it fix itself if you resize the window?
No zoom. Properly aligned when I make the window a little smaller but distorted again when going back to the full screen size.
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.
Attached patch v1Splinter Review
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8581483 - Flags: review?(adw)
Attached image before
Attached image v1 screenshot
Didn't try the build but just applied your patch locally. WFM, thanks!
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.
Attachment #8581483 - Flags: review?(adw)
(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 on attachment 8581483 [details] [diff] [review]
v1

See comment 11 for why availSpace computation is okay.
Attachment #8581483 - Flags: review?(adw)
Attachment #8581483 - Flags: review?(adw) → review+
Iteration: --- → 39.3 - 30 Mar
Points: --- → 2
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/8b514f389bf7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
QA Contact: cornel.ionce
Flags: qe-verify?
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.
Yohann, that's probably bug 1146146. The patch just landed on mozilla-central. It's gonna be on tomorrow's Nightly.
Blocks: 1148859
Blocks: 1120311
Reassigning to Karl Thiessen as he seems to be the designated QA contact for fixes related to bug 1120311.
QA Contact: cornel.ionce → kthiessen
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.
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.
QA Contact: kthiessen → cornel.ionce
(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+,
Flags: qe-verify? → qe-verify+
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.
Status: RESOLVED → VERIFIED
Blocks: 1155443
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.