Closed Bug 1269485 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: Private Browsing, defect, P1)

Product:
Firefox
Component:
Private Browsing
Version:
48 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 49
Iteration:
49.2 - May 23
Tracking Flags:
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: dholbert, Assigned: rickychien)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(3 files)

screenshot
(deleted), image/png
Details
screencast
(deleted), video/ogg
Details
MozReview Request: Bug 1269485 - Improve New Private Browsing start-page background styles r?Gijs
(deleted), text/x-review-board-request
Gijs
: review+
lizzard
: approval-mozilla-aurora+
Details
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.
Attached image screenshot (deleted) —
Attached video screencast (deleted) —
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>.
(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
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
No longer blocks: 1267501
Reused in-content-page-background variable and introduce .private in html tag.
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 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)
(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)
(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; }
(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/
(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.
https://hg.mozilla.org/mozilla-central/rev/1ded8982a100
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
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!
(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!!
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
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: --- → verified
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: