1705855 - Proton newtab have way too much padding and cause page scroll

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P2)

Description

[mmis1000]

Attachment: 螢幕擷取畫面 2021-04-17 104713.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

1. open firefox on windows on a screen equal to 1080P with taskbar to always present.
2. open new tab
3. open the config on new tab
4. set the row to 2

Actual results:

The new tab have a useless scrollbar.
All contents are visible, but the extra margin caused a scrollbar.

Expected results:

There shouldn't be a scrollbar when there is only whitespace (did it use margin to replace padding and cause such problem? I remember that Firefox don't cause scrollbar in that case.).

Comment 1

[mmis1000]

That also happen on my mac 13, but given the dimension is smaller on that device.
The proton design new tab probably didn't handle that at first place and thus not a bug.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::New Tab Page' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Simona Badau, Desktop QA]

Setting this issue to NEW, as I can reproduce it on the latest Nightly 90.0a1 and Firefox 89 beta 1 on Windows 10 x64. I can reproduce the issue regardless if the preference "browser.proton.enabled" is set to true or false. I can't reproduce the issue on Firefox 88.

Comment 4

[Romain Testard [:RT]]

Agreed that we should make sure that no scroll appears on 768 pixel height @100dpi on windows with a visible taskbar for default configurations.
Jim, I'm marking this one as a P2a but can you please help confirm scrollbar requirements here?

Comment 5

[Jim T]

CC'ing some folks from the search & newtab team on this one for discussion

Comment 6

[Dão Gottwald [:dao]]

I think we probably shouldn't show the Firefox logo when there's this much content already on the page.

Comment 7

[Juliana Gaibler [:juliana]]

There are multiple paddings on new-tab that cause this overflow. I'm sure we can just remove or replace them with margins as mmis1000 suggested :)

Comment 8

[Dão Gottwald [:dao]]

Yes, we could reduce paddings, but doesn't the page already look pretty crowded in attachment 9216555 [details]? Seems to me it would benefit overall from the logo being hidden and the three sections being more apart.

Comment 9

[Dão Gottwald [:dao]]

That said, even without any changes to the spacing between sections / elements, it looks like the page shouldn't be overflowing in attachment 9216555 [details]. Does removing the bottom margin from .outer-wrapper help? https://searchfox.org/mozilla-central/rev/49e0a928390a8013a4eb08b7b3bbff025f01913a/browser/components/newtab/content-src/components/Base/_Base.scss#9,12

Comment 10

[Juliana Gaibler [:juliana]]

On screens that are 768px high with no display scaling there is still overflow.

If this is critical for 89, we could probably change the .outer-wrapper padding and the main padding and that should do the trick.

Otherwise (or additionally) we can take a closer look at all these paddings and margins with the style changes planned in 90. Check if they all make sense, trim some away and ensure that the layout still looks good on all screen sizes :)

Comment 11

[Dão Gottwald [:dao]]

*sigh* I have a hard time following what a "ds outer wrapper breakpoint override" is so I'm hesitant to touch this stuff... My recommendation is still to hide the Firefox logo under certain circumstances -- for a short-term solution, perhaps a media query taking the view port height into account is enough.

Comment 12

[Scott [:thecount] Downe]

I'm wondering if a lot of the extra padding, causing the scrollbar, is coming from padding-bottom: 68px; on main, which I also suspect was there to ensure there is enough space for snippets.

Comment 13

[:Gijs (he/him)]

(In reply to Scott [:thecount] Downe from comment #12)

I'm wondering if a lot of the extra padding, causing the scrollbar, is coming from padding-bottom: 68px; on main, which I also suspect was there to ensure there is enough space for snippets.

Yeah, I tried looking at this a few days back and this was also my conclusion - minus the space for snippets thing. I just didn't understand why that was there (and where the magical 68px came from), and then apparently forgot to comment here. But if it's just for snippets, presumably it could be applied only when we actually show snippets?

Comment 14

[Scott [:thecount] Downe]

A standard 1 line snippet is roughly 67px, and a two line snippet is 94px.

The first padding height is coming from here: https://searchfox.org/mozilla-central/source/browser/components/newtab/content-src/components/Base/_Base.scss#31 which the comment states, is for snippets.

It looks like this currently resolves to 68px, and the override class has this same value as being static.

I also looked into the history for ds-outer-wrapper-breakpoint-override. There are essentially two sets of breakpoints that can be toggled, depending on the layout. Setting the class on main to ds-outer-wrapper-breakpoint-override triggers the breakpoint set which, is currently used on release. We could probably cleanup the old breakpoint sets and just go with ds-outer-wrapper-breakpoint-override, but I don't think that should happen here.

I think we can leave a lot of this as is, and just have snippets set a class on main if they are active or not, and have both sets of 68px padding bottom select based on that snippet added class.

Comment 15

[Bernard Igiri]

Attachment: Bug 1705855 - Fix newtab padding r=thecount

Used media queries to adjust padding when the browser window height is less than 1010px so that unnecessary scrollbars do not appear on newtab.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c19c6b038a4
Fix newtab padding r=thecount

Comment 17

[Dorel Luca [:dluca]]

Backed out changeset 2c19c6b038a4 (bug 1705855) for Newtab failures in content-src/components/Base/_Base.scss. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=339903237&repo=autoland&lineNumber=110

Push with failure:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=2c19c6b038a4f7741f97ae2fd1723e9e820547e6

Backout:
https://hg.mozilla.org/integration/autoland/rev/9290e60b108f447025011e47cb207d93a0b6800a

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/335bbc7eb086
Fix newtab padding r=thecount

Comment 19

[Natalia Csoregi [:nataliaCs]]

Backed out for causing newtab failures

backout: https://hg.mozilla.org/integration/autoland/rev/828795dd416a3f731a1a9de1fed3c624f1757594

push: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Copt%2Cnode%2Ctests%2Csource-test-node-newtab-unit-tests%2Cnewtab&revision=335bbc7eb086a92dba837a931affc0789e920b74&selectedTaskRun=QJdEunjuS---bBibGREo0A.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=340341275&repo=autoland&lineNumber=105

[task 2021-05-20T17:30:15.000Z] npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
[task 2021-05-20T17:30:15.000Z]
[task 2021-05-20T17:30:15.005Z] added 1279 packages from 1554 contributors and audited 1358 packages in 25.201s
[task 2021-05-20T17:30:15.691Z]
[task 2021-05-20T17:30:15.692Z] 83 packages are looking for funding
[task 2021-05-20T17:30:15.692Z] run npm fund for details
[task 2021-05-20T17:30:15.692Z]
[task 2021-05-20T17:30:15.693Z] found 37 vulnerabilities (18 low, 5 moderate, 14 high)
[task 2021-05-20T17:30:15.693Z] run npm audit fix to fix them, or npm audit for details
[task 2021-05-20T17:30:16.506Z] + node bin/try-runner.js
[task 2021-05-20T17:30:16.581Z] TEST START | checkBundles
[task 2021-05-20T17:30:27.449Z] TEST-UNEXPECTED-FAIL checkBundles | Activity Stream bundle out of date
[task 2021-05-20T17:30:27.451Z] { checkBundles: false }
[task 2021-05-20T17:30:27.452Z] CODE 1
[taskcluster 2021-05-20 17:30:28.009Z] === Task Finished ===
[taskcluster 2021-05-20 17:30:28.009Z] Unsuccessful task run with exit code: 1 completed in 228.225 seconds

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff0aabe33b7d
Fix newtab padding r=thecount

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ff0aabe33b7d

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bigiri, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

[Marius Coman [:mcoman], Ecosystem QA]

I have verified that this issue is no longer reproducible with the latest Firefox Nightly (90.0a1 Build ID - 20210525153449) and the latest Firefox Beta (89.0 Build ID - 20210524222230) installed on Windows 10 x64, Windows 8.1 x64, and Windows 7 x64. Now I can confirm that the scrollbar is no longer displayed if two rows are displayed in the "Shortcuts" section.