Closed Bug 1267499 Opened 8 years ago Closed 8 years ago

New Private Browsing start-page overflows off the window bottom (triggering a scrollbar) for totally-reasonable window sizes

Categories

(Firefox :: Private Browsing, defect, P1)

Product:
Firefox
Component:
Private Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Iteration:
49.2 - May 23
Tracking Flags:
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: dholbert, Assigned: rickychien)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(3 files)

screenshot showing bug (quite large window, but content doesn't fit)
106.44 KB, image/png
Details
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Patch for aurora
1.45 KB, patch
Gijs
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
STR:
 1. Resize your Firefox window to be, say, 800px tall. (This is large enough to be useful, and is near the native resolution-height of some smaller laptop/netbook displays.)

 2. Open a private Browsing window. (Ctrl+Shift+P)

ACTUAL RESULTS:
The private browsing start-page overflows off the bottom of its viewport, triggering an unnecessary scrollbar.

EXPECTED RESULTS:
The page should fit in an 800px-tall window without needing a scrollbar.  Specifically:
 - Perhaps the page should use media queries to reduce the font size as the window gets smaller, at least down to some threshold)
 - Perhaps the default font size should be smaller. (The mockup at http://people.mozilla.org/~bbell/about-page-improvements/about-privateBrowsing.html has a decently-smaller font size, at least.)
Blocks: 1259340
Blocks: 1216897
Whiteboard: [fxprivacy] [triage]
See Also: → 1267501
Ricky, could you help?
Assignee: nobody → rchien
Flags: needinfo?(rchien)
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Sure!
Flags: needinfo?(rchien)
The root cause of this issue is because there are extra paddings and margin-bottom applied on it. I can remove it so that makes it better than before.
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49139/diff/1-2/
Attachment #8745884 - Attachment description: MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom; r?=Gijs → MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom; r?Gijs
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

https://reviewboard.mozilla.org/r/49139/#review46091

I don't think this fixes the issue as described.

The default font-size on Ubuntu is much higher than on other OSes, and so the 1.5em we use for <body> is just too big. You should test it there and come up with an alternative patch.

dholbert, if you inspect with devtools, what's the computed font size of the text in the <p>s in the implemented thing, compared with the font size of the text in the mockup? (ie both measures in px, please!)
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #7)
> dholbert, if you inspect with devtools, what's the computed font size of the
> text in the <p>s in the implemented thing, compared with the font size of
> the text in the mockup? (ie both measures in px, please!)

Here's a grid comparing the computed font-size, for the main title & the first non-title "main text", at the Nightly Private Browsing Startpage, the Mockup, and the neterror Server-Not-Found page that we show when you visit a bogus domain.

           | Nightly PBStartpage |  Mockup  |  Server-Not-Found page
Title text |         55px        |   36px   |      37.5px
 Main text |         22px        |   16px   |       15px
Flags: needinfo?(dholbert)
It would also help if we could get "Private Browsing with Tracking Protection" to appear on a single-line in most cases. Right now it wraps with "Protection" on its own line in en-US.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> It would also help if we could get "Private Browsing with Tracking
> Protection" to appear on a single-line in most cases. Right now it wraps
> with "Protection" on its own line in en-US.

... on Windows. :-)
So, I think it might be helpful for me to
a) apologize / eat some humble pie; and
b) suggest an approach to fix this

My review comments in bug 1259340 made Ricky/you implement the body font-size using em instead of fixing it in pixels. While I still believe we need to wean ourselves off fixed-in-px font-sizes for in-content pages (common.inc.css fixes to 15px, IIRC), it's clear that in the short-term this causes undesirable regressions. So I think I was wrong in suggesting this, and I'm sorry about that.

I think that to address this & other issues, we should remove our own font-size stuff from aboutPrivateBrowsing.css, and optionally adjust font-sizes for the secondary header (I believe the main title has approximately the right font-size set through info-pages already - though I could be wrong). Anything we do set should still be in em, as that's what we do everywhere else in-content: set the "base" font-size on body in px, and use relative (ie em) sizing everywhere else. This effectively boils down to taking some of the changes that ntim suggested in bug 1267047.

This should address the size issue on Linux for now (at least to make it no worse than other in-content things).

Ideally I would still like to look at obeying OS and therefore user-set font-sizes for these pages somehow, but we should probably be more deliberate about that change in a separate bug.
Attachment #8745884 - Attachment is obsolete: true
New patch is ready and I think it is great on Ubuntu & Mac.
I'll submit it again once Gijs back May 3rd
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49139/diff/2-3/
Attachment #8745884 - Attachment description: MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom; r?Gijs → MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs
Attachment #8745884 - Attachment is obsolete: false
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

https://reviewboard.mozilla.org/r/49139/#review47027

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 3)
>  }
>  
>  body {
> -  padding: 40px;
>    background-color: var(--color-background-light);
> -  font-size: 1.5em;

Do we not need to adjust the font-size of any of the headers?

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:62
(Diff revision 3)
> +section:last-child.section-main {
> +  margin-bottom: 0;
> +}
> +

I don't think we still need this? What does it accomplish?

Both here and in the previous rule I don't think we need the tag name in the selector.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:91
(Diff revision 3)
>    line-height: 1.5em;
>    color: var(--color-darkTheme-purple-text);
>    min-height: 64px;
>    -moz-margin-start: 0;
>    -moz-padding-start: calc(var(--icon-margin) + 24px);
> +  width: 100%;

Why do we need this?
https://reviewboard.mozilla.org/r/49139/#review47027

> Do we not need to adjust the font-size of any of the headers?

I noticed that current results aren't so bad on Linux / MacOS (Windows is similar to Linux but I haven't tried) after just removing this font-size.

> I don't think we still need this? What does it accomplish?
> 
> Both here and in the previous rule I don't think we need the tag name in the selector.

In order to resolve this problem on bug description:

EXPECTED RESULTS:
The page should fit in an 800px-tall window without needing a scrollbar

Thus, it will remove reduntant whitespace at the bottom.

> Why do we need this?

As comment 10 mentioned, it achieves requirement if we'd like to get "Private Browsing with Tracking Protection" into a single-line in most cases.
Flags: needinfo?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/49139/#review47479

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 3)
>  
>    --icon-margin: 64px;
>  }
>  
>  body {
> -  padding: 40px;

As noted in the other bug, we need to keep this for now.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:62
(Diff revision 3)
> +section:last-child.section-main {
> +  margin-bottom: 0;
> +}
> +

OK, let's keep this then.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:91
(Diff revision 3)
>    line-height: 1.5em;
>    color: var(--color-darkTheme-purple-text);
>    min-height: 64px;
>    -moz-margin-start: 0;
>    -moz-padding-start: calc(var(--icon-margin) + 24px);
> +  width: 100%;

This causes a horizontal scrollbar when the window is too narrow, because you're sizing to 100% of the container but the element has margins.

If the window is too narrow, the text won't fit on one line and that's OK. By default the <h1> has width: auto ( https://developer.mozilla.org/en/docs/Web/CSS/width ) which for a block element will mean it'll just fit to the container, taking the margins into account. So please just remove this line.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49139/diff/3-4/
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

https://reviewboard.mozilla.org/r/49139/#review47751

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:63
(Diff revision 4)
>    margin-bottom: 48px;
>    -moz-margin-start: var(--icon-margin);
>    -moz-padding-start: 24px;
>  }
>  
> +section:last-child.section-main {

Nit: leave the :last-child at the end of the selector.

As I said before now, I don't think we need the tagname in either of these `.section-main` selectors - do we?
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49139/diff/4-5/
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

https://reviewboard.mozilla.org/r/49139/#review48037

Thanks!
Attachment #8745884 - Flags: review?(gijskruitbosch+bugs) → review+
needs rebasing i guess

patching file browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
Hunk #1 FAILED at 19
1 out of 2 hunks FAILED -- saving rejects to file browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(rchien)
Keywords: checkin-needed
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49139/diff/5-6/
Flags: needinfo?(rchien)
Conflict resolved!
Keywords: checkin-needed
Status: NEW → ASSIGNED
Comment on attachment 8745884 [details]
MozReview Request: Bug 1267499 - Fix Private Browsing start-page overflows off the window bottom r?Gijs

Approval Request Comment
[Feature/regressing bug #]: bug 1259340
[User impact if declined]: the font size of about:privatebrowsing is much too large on Ubuntu, and on Windows if the user has configured different font sizes in their OS. This can render the explanation about privatebrowsing that about:privatebrowsing offers unreadable/unusable.
[Describe test coverage new/current, TreeHerder]: no, styling issue
[Risks and why]: low, small CSS-only change (do we still have blanket approval for these things?)
[String/UUID change made/needed]: no.
Attachment #8745884 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/091396722107
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Iteration: --- → 49.2 - May 23
Flags: qe-verify?
Priority: P3 → P1
Um, why did this land without the changes to font-size? Now the font-size is still going to be wrong on Linux...
Flags: needinfo?(rchien)
Per discussion with Jared on IRC, landed a followup:

remote:   https://hg.mozilla.org/integration/fx-team/rev/c331e892afc7

I'll upload a separate aurora patch later today.
Flags: needinfo?(rchien) → needinfo?(gijskruitbosch+bugs)
Oops! sorry about that. Probably something was wrong when rebasing branch. I'll take care of this next time, thank you Gijs:)
Attached patch Patch for aurora — — Splinter Review 
Approval Request Comment
[Feature/regressing bug #]: bug 1259340
[User impact if declined]: the font size of about:privatebrowsing is much too large on Ubuntu, and on Windows if the user has configured different font sizes in their OS. This can render the explanation about privatebrowsing that about:privatebrowsing offers unreadable/unusable.
[Describe test coverage new/current, TreeHerder]: no, styling issue
[Risks and why]: low, small CSS-only change (do we still have blanket approval for these things?)
[String/UUID change made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8753717 - Flags: review+
Attachment #8753717 - Flags: approval-mozilla-aurora?
Attachment #8745884 - Flags: approval-mozilla-aurora?
Sylvestre, approval ping?
status-firefox48: --- → affected
Flags: needinfo?(sledru)
Comment on attachment 8753717 [details] [diff] [review]
Patch for aurora

Regression in 48, css fix for private browsing page to look right.
OK to uplift.
Attachment #8753717 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
from comment #10:
> It would also help if we could get "Private Browsing with Tracking Protection" to appear on a
> single-line in most cases. Right now it wraps with "Protection" on its own line in en-US.

I tested russian version of Nightly, and this bug is still actual. When I enable private browsing (assuming it was disabled earlier), text "Приватный просмотр с Защитой от отслеживания" appears on 2 lines instead of "Приватный просмотр" (1 line), and the page becomes long enough to gain the scrollbar. Therefore enabling/disabling tracking protection causes noticeable twitching. Is that expected result?
Tested on:   Win7_64, Nightly 49 ru, 32bit, ID 20160526082509, screen 1366x768, DPI = 100%

If this isn't expected, I think the best solution would be to always show only "Private Browsing"
(ideal world). Also it's possible to preserve extra space to prevent any twitching. See CSS hack below

#titleTracking.hide{
    display: inline !important;
    visibility: hidden !important;
}
.title{
    background-position: left top !important;
    position: relative !important;
}
#title{
    position: absolute !important;
}
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to arni2033 from comment #37)
> from comment #10:
> > It would also help if we could get "Private Browsing with Tracking Protection" to appear on a
> > single-line in most cases. Right now it wraps with "Protection" on its own line in en-US.
> 
> I tested russian version of Nightly, and this bug is still actual. When I
> enable private browsing (assuming it was disabled earlier), text "Приватный
> просмотр с Защитой от отслеживания" appears on 2 lines instead of "Приватный
> просмотр" (1 line), and the page becomes long enough to gain the scrollbar.
> Therefore enabling/disabling tracking protection causes noticeable
> twitching. Is that expected result?

I don't know because I don't know what is twitching and what you mean. Please file a separate bug with more details.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(arni2033)
Flags: needinfo?(arni2033)
See Also: → 1259340
See Also: 1259340 → 1278548
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: