If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Tiles on the newtab page don't wrap properly

VERIFIED FIXED in Firefox 38

Status

()

Firefox
New Tab Page
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: Mardak)

Tracking

({regression})

Trunk
Firefox 39
regression
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: .?)

Attachments

(7 attachments)

(Reporter)

Description

3 years ago
Created attachment 8581466 [details]
Screenshot of issue

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

Updated

3 years ago
Whiteboard: .?
(Assignee)

Comment 1

3 years ago
ttaubert, are you zoomed out? Does it fix itself if you resize the window?
(Reporter)

Comment 2

3 years ago
No zoom. Properly aligned when I make the window a little smaller but distorted again when going back to the full screen size.
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8581483 [details] [diff] [review]
v1
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8581483 - Flags: review?(adw)
(Assignee)

Comment 5

3 years ago
Created attachment 8581484 [details]
before
(Assignee)

Comment 6

3 years ago
Created attachment 8581485 [details]
v1 screenshot
(Assignee)

Comment 7

3 years ago
ttaubert, does this build fix this bug for you? http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/edilee@gmail.com-6c5947aadce7/try-macosx64/firefox-39.0a1.en-US.mac.dmg
(Reporter)

Comment 8

3 years ago
Didn't try the build but just applied your patch locally. WFM, thanks!
Duplicate of this bug: 1146231

Comment 10

3 years ago
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)
(Assignee)

Comment 11

3 years ago
Created attachment 8583642 [details]
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.
(Assignee)

Comment 12

3 years ago
Comment on attachment 8581483 [details] [diff] [review]
v1

See comment 11 for why availSpace computation is okay.
Attachment #8581483 - Flags: review?(adw)

Updated

3 years ago
Attachment #8581483 - Flags: review?(adw) → review+

Comment 13

3 years ago
OK, thanks Ed.
(Assignee)

Updated

3 years ago
Iteration: --- → 39.3 - 30 Mar
Points: --- → 2
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/8b514f389bf7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
QA Contact: cornel.ionce

Updated

3 years ago
Flags: qe-verify?

Comment 15

3 years ago
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

3 years ago
Created attachment 8585044 [details]
Display 2 rows having space for 3

Comment 17

3 years ago
Created attachment 8585045 [details]
Display in fullscreen, 3 rows showing correctly

Comment 18

3 years ago
Yohann, that's probably bug 1146146. The patch just landed on mozilla-central. It's gonna be on tomorrow's Nightly.
(Assignee)

Updated

3 years ago
Blocks: 1148859
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/6892b485a7e0
status-firefox38: --- → fixed
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
status-firefox38: fixed → verified
status-firefox39: fixed → verified

Updated

3 years ago
Blocks: 1155443

Updated

3 years ago
No longer blocks: 1155443
You need to log in before you can comment on or make changes to this bug.