New Private Browsing start-page has white/gray-text-on-white-background, in overflowed area off the right side of the window

VERIFIED FIXED in Firefox 48

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
10 months ago

People

(Reporter: dholbert, Assigned: rickychien)

Tracking

(Blocks 1 bug, {regression})

48 Branch
Firefox 49
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite +
qe-verify ?

Firefox Tracking Flags

(firefox47 unaffected, firefox48+ verified, firefox49 verified, firefox50 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(3 attachments)

Reporter

Description

3 years ago
STR:
 1. Open a private browsing window.

 2. Make your window skinny enough that some content overflows off of the right side (so that you can scroll horizontally).  (It may help if you first do full-page-zoom so that content overflows in a more extreme way.)

 3. Scroll the page to the right, horizontally.

ACTUAL RESULTS: The region of the page that you scroll into view has a *white background*.  So, you end up with white/gray text on a white background, which is hard to read (and looks pretty broken).

EXPECTED RESULTS: Page background should be consistently the same color, even if some content overflows.
Reporter

Comment 1

3 years ago
Posted image screenshot
Reporter

Comment 2

3 years ago
Posted video screencast
Reporter

Comment 3

3 years ago
The simplest way to fix this is to set the page's "background-color" on the <html> element instead of the <body> element.

Right now, the purple background-color is set on the <body>, and the <body> is only as wide as the viewport. (It does not shrinkwrap overflowing content in deeply-nested descendants -- though that overflowing content does create scrollable overflow.)

To actually set the page background beyond the <body>'s bounds, you need to set "background-color" on <html> instead of <body>.
Reporter

Comment 4

3 years ago
(Not sure if this may be fixed via some other bug, as Gijs hinted at in his comments on bug 1267501. In any case, we need to make sure this is addressed before we ship this new private browsing startpage UI.)
Agree with you. There is another example such as normal start-page set its background color in <html> and it get rid of those unexpected UI appear when window is shrunk.
Assignee: nobody → rchien
Status: NEW → ASSIGNED

Comment 6

3 years ago
We should take the approach from bug 1267047 and set the private class on .html, and then use the in-content-page-background variable. Then with the patch from bug 1267499 we can get rid of the body {} selector, and we should update the variable when `html` has the `private` class.
Blocks: 1259340
Reporter

Updated

3 years ago
No longer blocks: 1267501
Reused in-content-page-background variable and introduce .private in html tag.

Comment 9

3 years ago
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

https://reviewboard.mozilla.org/r/50389/#review47183

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css
(Diff revision 1)
>    --icon-margin: 64px;
>  }
>  
> -body {
> +html {
> -  padding: 40px;
> -  background-color: var(--color-background-light);

This is now unused, so please remove the variable as well.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:30
(Diff revision 1)
>    color: var(--color-darkTheme-purple-text);
>    background-color: var(--color-background-dark-purple);

Instead, can you modify the relevant in-content variables ?
Attachment #8748544 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50389/diff/1-2/
Attachment #8748544 - Flags: review?(gijskruitbosch+bugs)

Comment 11

3 years ago
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

https://reviewboard.mozilla.org/r/50389/#review47471

With the below points fixed, r=me

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:8
(Diff revision 2)
>    --color-background-dark: #303033;
>    --color-background-dark-purple: #1c023c;

These can be removed as they are now unused.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:28
(Diff revision 2)
> -  background-color: var(--color-background-light);
>    font-size: 1.5em;
>  }
>  
> -body.private {
> -  color: var(--color-darkTheme-purple-text);
> +html.private {
> +  --in-content-page-color: #beb8cc;

Please also set `in-content-text-color` to the same value.

::: browser/themes/shared/privatebrowsing/aboutPrivateBrowsing.css:81
(Diff revision 2)
>    background-image: url("chrome://browser/skin/privatebrowsing/private-browsing.svg");
>    background-size: 64px;
>    background-position: left, center;
>    font-weight: lighter;
>    line-height: 1.5em;
>    color: var(--color-darkTheme-purple-text);

With in-content-text-color set, you can omit this property, and gt rid of the variable definition at the top.
Attachment #8748544 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50389/diff/2-3/
Final update for addressing above issues.

I'm curious in styles which defined in info-pages.css that makes normal mode (about:privatebrowsing in non-private-browsing window)'s content off the right side of the middle. However, all of these properties inherited from info-pages.css. Is that correct?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 14

3 years ago
(In reply to Ricky Chien [:rickychien] from comment #13)
> Final update for addressing above issues.
> 
> I'm curious in styles which defined in info-pages.css that makes normal mode
> (about:privatebrowsing in non-private-browsing window)'s content off the
> right side of the middle. However, all of these properties inherited from
> info-pages.css. Is that correct?

Ah. No. Please leave the body { padding: 40px; } for now. That's what's avoiding this, and we should keep it for now, especially because removing it isn't necessary to fix this bug.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

3 years ago
(In reply to Ricky Chien [:rickychien] from comment #13)
> Final update for addressing above issues.
> 
> I'm curious in styles which defined in info-pages.css that makes normal mode
> (about:privatebrowsing in non-private-browsing window)'s content off the
> right side of the middle. However, all of these properties inherited from
> info-pages.css. Is that correct?

The default padding set on body: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/info-pages.inc.css#14

That padding is used to keep the icon fully visible at small sizes until it's hidden by the media query at 675px. Otherwise, a part of the icon will go offscreen, and the other part on screen, which looks strange.

What you could do is: 
html:not(.private) body {
  padding: 40px;
}

Comment 16

3 years ago
(In reply to Tim Nguyen :ntim from comment #15)
> (In reply to Ricky Chien [:rickychien] from comment #13)
> > Final update for addressing above issues.
> > 
> > I'm curious in styles which defined in info-pages.css that makes normal mode
> > (about:privatebrowsing in non-private-browsing window)'s content off the
> > right side of the middle. However, all of these properties inherited from
> > info-pages.css. Is that correct?
> 
> The default padding set on body:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/info-pages.inc.css#14
> 
> That padding is used to keep the icon fully visible at small sizes until
> it's hidden by the media query at 675px. Otherwise, a part of the icon will
> go offscreen, and the other part on screen, which looks strange.
> 
> What you could do is: 
> html:not(.private) body {
>   padding: 40px;
> }

The misalignment also happens in private mode, it's just slightly harder to see. We should just keep the padding as-is for now.
(In reply to :Gijs Kruitbosch from comment #16)
> The misalignment also happens in private mode, it's just slightly harder to
> see. We should just keep the padding as-is for now.

You're right! There is different padding value I saw after looking into devtools's box model, keeping it as-is is better.
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50389/diff/3-4/

Comment 19

3 years ago
(In reply to :Gijs Kruitbosch from comment #16)
> The misalignment also happens in private mode, it's just slightly harder to
> see. We should just keep the padding as-is for now.

I think you're missing the point. The padding is there for a reason. It fixes what's mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1087618#c55 . about:privatebrowsing doesn't seem to have the issue, because it doesn't make full use of the classes.
Anyway, this is out of scope of this bug, I'll fix it in bug 1267047 once this bug lands.

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ded8982a100
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Blocks: 1216897
Iteration: --- → 49.2 - May 23
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [fxprivacy]
Gijs, I got a quick question here. Is this necessary to uplift to firefox 48? If so, how do I request for a 48 uplift? thanks!

Comment 23

3 years ago
(In reply to Ricky Chien [:rickychien] from comment #22)
> Gijs, I got a quick question here. Is this necessary to uplift to firefox
> 48? If so, how do I request for a 48 uplift? thanks!

Yes, we should uplift for 48. You can do so by going to the attachment details page for the mozreview request: https://bugzilla.mozilla.org/attachment.cgi?id=8748544&action=edit and setting the 'approval-mozilla-aurora' flag to '?', and filling in the details in the form that shows up in the comment box, and then submitting that comment.

Note that we should also request uplift on the other patches that we've added since the original landing of bug 1259340. I'll go through the other ones and request uplifts there, too. Can you do this one?
Flags: needinfo?(rchien)
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

Approval Request Comment
[Feature/regressing bug #]: bug 1259340
[User impact if declined]: UI improvement
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: very low, tiny UI change
[String/UUID change made/needed]: nope
Flags: needinfo?(rchien)
Attachment #8748544 - Flags: approval-mozilla-aurora?
Thank you for this help!!
Minor regression but recent, tracking
Comment on attachment 8748544 [details]
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs

We want private browsing to look nice, let's uplift!
Attachment #8748544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Version: unspecified → 48 Branch
I have reproduced this bug with Nightly 49.0a1 (2016-05-02) on Windows 7 , 64 Bit!

This bug's fix is verified on latest Nightly , Aurora.


 Build ID   20160714030208

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



 Build ID   20160708004052

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

Updated

3 years ago
Status: RESOLVED → VERIFIED
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.