New Tab's height doesn't update the same moment when I delete extra tiles/choose "show blank page"

RESOLVED FIXED in Firefox 44

Status

()

Firefox
New Tab Page
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arni2033, Assigned: maxim zhilyaev)

Tracking

Trunk
Firefox 44
Points:
5

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8654342 [details]
Video - scenario (B) - New Tab's height doesn't update the same moment when I delete extra tiles or choose 'show blank page'.webm

STR:   (Win7_64bit, Nightly 43.0a1 32bit   ID 20150828030205, new profile)

[There's 2 scenarios: (A) and (B). Watch video to see scenario (B) ]

0.   Open New Tab, click Gear button, uncheck "Include suggested sites". Adjust the window's height so that only 4 tiles in a row were visible, and only 2 or 1 rows were fully visible.
1. Fill all visible tiles on New Tab +1 tile. If you already have enough sites there to cause
   overflow, then skip this step.
  1.1. Open attached page, click textarea, press Ctrl+A to select all text, press Ctrl+C to copy
  1.2. Press Ctrl+B to open bookmarks, click "Bookmarks Menu", press Ctrl+V to create bookmarks
  1.3. Right-click "Bookmarks Menu", click "Open all in tabs" menuitem.
2.   Open New Tab, make sure that there's overflow on New Tab (vertical scrollbar is visible)
3.A) Click Gear button, select "Show blank page"
3.B) Delete extra tiles, so that the existing tabs were fully visible (that means that
     there should be no overflow according to bug 1194895)
4.   Click "New tab" button/press Ctrl+T to open new tab
5.   Repeat Step 4.

Result:       After Step 4 scrollbar is still visible. It disappears only after Step 5.
              If I set browser.newtab.preload -> true then scrollbar disappears after Step 4.
Expectations: Scrollbar should disappear after Step 3.
(Reporter)

Comment 1

3 years ago
Created attachment 8654343 [details]
Page with URLs to create history - New Tab's height doesn't update the same moment when I delete extra tiles or choose 'show blank page'.html
(Reporter)

Updated

3 years ago
See Also: → bug 1199801
(Reporter)

Updated

3 years ago
Blocks: 1180387
No longer blocks: 1194895
(Assignee)

Updated

3 years ago
Assignee: nobody → mzhilyaev
Points: --- → 5
(Assignee)

Comment 2

3 years ago
Created attachment 8660939 [details]
v1.  Make gGrid.refresh() also resize

A minor change in grid refresh logic.  Olivier and Tim to validate that it's OK to land in nightly.  Changes are minor and should be easy to port to remove newtab.
Flags: needinfo?(tspurway)
Flags: needinfo?(oyiptong)
Attachment #8660939 - Flags: review?(msamuel)
Comment on attachment 8660939 [details]
v1.  Make gGrid.refresh() also resize

>   /**
>-   * Renders the grid, including cells and sites.
>+   * Renders and resizes the gird. _resizeGrid() call is needed to ensure
"Renders and resizes the grid."

>+   * that scrollbar disappears when the bottom row becomes empty following
>+   * the block action, or tile display is turmed off via cog menu
>    */
>+
>   refresh() {
>+    this._refresh();
is this._refresh defined anywhere?
Flags: needinfo?(oyiptong)
Max, as you know, I would like to see all dev on the newtab page frozen, and any patches applied to the remote new tab page.  The exceptions should be rare and serious.  I don't have a good feeling for how serious this bug is.  oyiptong?
Flags: needinfo?(tspurway) → needinfo?(oyiptong)
Doesn't look that serious, in that it is a cosmetic issue.

We certainly want to fix this bug, however as you say, it is a bug-fix to a feature that has been introduced when the newtab page should've been frozen.

Max: will it fix all scrollbar-related issues? I don't think that bug is acceptable for good UX.

I propose two solutions: we back out the parent change: bug 1194895 and implement scrollbars in the remote new tab page, or we fix this bug for good and port it over to the remote new tab page.

The status quo is a broken window that irks me.
Flags: needinfo?(oyiptong)
also: once you rename refresh to _refreshGrid, that makes the new refresh function dead code, doesn't it?
ah pardon me: it is used in page.js
(Assignee)

Comment 8

3 years ago
Created attachment 8661969 [details] [diff] [review]
v2. fixed typo
Attachment #8660939 - Attachment is obsolete: true
Attachment #8660939 - Flags: review?(msamuel)
Attachment #8661969 - Flags: review?(msamuel)
(Assignee)

Updated

2 years ago
Attachment #8661969 - Flags: review?(msamuel) → review?(mcaceres)
Comment on attachment 8661969 [details] [diff] [review]
v2. fixed typo

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

Looks pretty straight forward.
Attachment #8661969 - Flags: review?(mcaceres) → review+
https://hg.mozilla.org/mozilla-central/rev/535c11ecde00
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Updated

2 years ago
Has STR: --- → yes
You need to log in before you can comment on or make changes to this bug.