Closed Bug 1199795 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: arni2033, Assigned: mzhilyaev)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
See Also: → 1199801
Blocks: 1180387
No longer blocks: 1194895
Assignee: nobody → mzhilyaev
Points: --- → 5
Attached file v1. Make gGrid.refresh() also resize (obsolete) —
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
Attached patch v2. fixed typoSplinter Review
Attachment #8660939 - Attachment is obsolete: true
Attachment #8660939 - Flags: review?(msamuel)
Attachment #8661969 - Flags: review?(msamuel)
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Has STR: --- → yes
You need to log in before you can comment on or make changes to this bug.