Closed Bug 1087618 Opened 10 years ago Closed 10 years ago

Implement the new style for about:welcomeback, sessionrestore, and e10s crash pages

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Flags:
Tracking Status
e10s - ---

People

(Reporter: mmaslaney, Assigned: ntim)

References

(Depends on 3 open bugs)

Details

Attachments

(9 files, 27 obsolete files)

InContent-icon-assets.zip
(deleted), application/zip
Details
Part 1 : Introduce new tree style (v1.2)
(deleted), patch
ntim
: review+
ntim
: checkin+
Details | Diff | Splinter Review
Part 5 - Changes to specific stylesheets
(deleted), patch
jaws
: review+
Details | Diff | Splinter Review
about:tabcrashed screenshot
(deleted), image/png
Details
about:welcomeback screenshot
(deleted), image/png
Details
about:sessionrestore screenshot
(deleted), image/png
mmaslaney
: ui-review+
Details
Part 2 : Add icons (v1.4)
(deleted), patch
ntim
: review+
Details | Diff | Splinter Review
Part 3 : Create common stylesheet (v2.2)
(deleted), patch
ntim
: review+
Details | Diff | Splinter Review
Part 4 : Update HTML structure (v1.1)
(deleted), patch
jaws
: review+
Details | Diff | Splinter Review
Flags: qe-verify+
Flags: firefox-backlog+
Points: --- → 5
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8520949 - Flags: ui-review?(mmaslaney)
Tim, are there updated screen shots, or should I review the attachments from comments 2–4?
Flags: needinfo?(ntim007)
(In reply to mmaslaney from comment #17) > Tim, are there updated screen shots, or should I review the attachments from > comments 2–4? The ones from comments 2-4.
Flags: needinfo?(ntim007)
Can you also check if about:blocked and about:robots don't look too bad ?
Comment on attachment 8520949 [details] [diff] [review] Patch v2.5 • "Well this is embarrassing" page: The restore box has to much top padding. • "Success page": The icon appears to close to the headers • Global: All pages are missing the header/content dividing rule.
Attachment #8520949 - Flags: ui-review?(mmaslaney) → ui-review-
I like those changes. Just a random thoughts: * about:blocked buttons don't look enough like buttons. Maybe that's because they have no (visible) border outline, * I have a feeling that the text inside is not vertically aligned. But that might be just an illusion due to a very high contrast between the red and the gray, * does this patch take care of the favicons?
Blocks: 1105254
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Attached patch Part 1 : Introduce new tree style (obsolete) (deleted) — — Splinter Review
Decided to start over from scratch. Here's how I'm going to proceed : - Part 1 : New tree style - Part 2 : Icons - Part 3 : Create about-pages.css - Part 4 : Change HTML structure of about pages - Part 5 : Clean up netError.css - Part 6 : Add stylesheet for about:blocked If you have patches for one of these parts, please, please submit them here.
Assignee: nobody → ntim007
Attachment #8532816 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8532902 - Flags: review?(jaws)
Attached patch Part 2 : Add icons (obsolete) (deleted) — — Splinter Review
Attachment #8532903 - Flags: review?(jaws)
Attached image dialogTable.png (obsolete) (deleted) —
(In reply to Tim Nguyen [:ntim] from comment #25) > Created attachment 8532902 [details] [diff] [review] > Part 1 : Introduce new tree style With this patch the treecols in dialogs are too tall.
Attached patch Part 1 : Introduce new tree style (v1.1) (obsolete) (deleted) — — Splinter Review
Thanks ! Fixed the issue :)
Attachment #8532902 - Attachment is obsolete: true
Attachment #8532902 - Flags: review?(jaws)
Attachment #8532930 - Flags: review?(jaws)
Attachment #8532905 - Attachment is obsolete: true
Attachment #8520874 - Attachment is obsolete: true
Attachment #8520872 - Attachment is obsolete: true
Attachment #8520812 - Attachment is obsolete: true
Attachment #8520811 - Attachment is obsolete: true
Attachment #8520809 - Attachment is obsolete: true
Summary: Implement the new style for about:welcomeback, sessionstore, and e10s crash pages → Implement the new style for about:welcomeback, sessionrestore, and e10s crash pages
Comment on attachment 8532903 [details] [diff] [review] Part 2 : Add icons Review of attachment 8532903 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't work for me, and I'm not sure how it worked for you ;) The patch adds the SVG files to a folder named 'in-content-icons', but all of the jar.mn files reference a non-existent folder named 'incontent-icons' (note difference in number of hyphens). ::: browser/themes/shared/in-content-icons/welcome-back.svg @@ +1,4 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" > + viewBox="0 0 60 60" enable-background="new 0 0 60 60" xml:space="preserve"> > +<linearGradient id="gradient" gradientUnits="userSpaceOnUse" x1="-1.714161e-07" y1="30" x2="60" y2="30"> x1 here is basically 0. Can this be changed to 0 without any visible impact?
Attachment #8532903 - Flags: review?(jaws) → review-
Comment on attachment 8532930 [details] [diff] [review] Part 1 : Introduce new tree style (v1.1) Review of attachment 8532930 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/in-content/common.inc.css @@ +595,5 @@ > + > +xul|richlistbox, > +xul|listbox { > + -moz-appearance: none; > + border: 1px solid #C1C1C1; nit: Please be consistent with upper vs lowercase color values.
Attachment #8532930 - Flags: review?(jaws) → review+
Used lowercase hex codes.
Attachment #8532930 - Attachment is obsolete: true
Attachment #8535060 - Flags: review+
Attached patch Part 2 : Add icons (obsolete) (deleted) — — Splinter Review
Attachment #8535065 - Flags: review?(jaws)
Attachment #8532903 - Attachment is obsolete: true
Comment on attachment 8535065 [details] [diff] [review] Part 2 : Add icons Icons should be in chrome://browser/skin/in-content/*.svg
checkin-needed for part 1 only.
Keywords: checkin-needed, leave-open
(In reply to Tim Nguyen [:ntim] from comment #34) > checkin-needed for part 1 only. Are you sure we should be landing these independently?
Comment on attachment 8535065 [details] [diff] [review] Part 2 : Add icons Review of attachment 8535065 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/jar.mn @@ +48,5 @@ > skin/classic/browser/identity-icons-https-mixed-display.png > skin/classic/browser/identity-icons-https-mixed-display@2x.png > + skin/classic/browser/in-content/session-restore.svg (../shared/incontent-icons/session-restore.svg) > + skin/classic/browser/in-content/tab-crashed.svg (../shared/incontent-icons/tab-crashed.svg) > + skin/classic/browser/in-content/welcome-back.svg (../shared/incontent-icons/welcome-back.svg) I don't see the use for an 'in-content' folder here. Let's just have the jar.mn put these in the /browser/ folder, as we do with almost all of our other icons, irrespective of where they get used today.
Attachment #8535065 - Flags: review?(jaws) → feedback+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35) > (In reply to Tim Nguyen [:ntim] from comment #34) > > checkin-needed for part 1 only. > > Are you sure we should be landing these independently? Yeah, that code also affects about:preferences.
Depends on: 1111236
Attachment #8535060 - Flags: checkin+
Attached patch Part 1b - Tree styling followup (obsolete) (deleted) — — Splinter Review
Small patch that removes redundant code.
Attachment #8536106 - Flags: review?(jaws)
Attached patch Part 2 : Add icons (v1.2) (obsolete) (deleted) — — Splinter Review
Attachment #8535065 - Attachment is obsolete: true
Attachment #8536108 - Flags: review?(jaws)
Attachment #8536110 - Flags: review?(jaws)
Depends on: 1111504
Comment on attachment 8536106 [details] [diff] [review] Part 1b - Tree styling followup Canceling review so I can submit a better patch in bug 1111504.
Attachment #8536106 - Flags: review?(jaws)
Attachment #8536106 - Attachment is obsolete: true
Comment on attachment 8536108 [details] [diff] [review] Part 2 : Add icons (v1.2) Review of attachment 8536108 [details] [diff] [review]: ----------------------------------------------------------------- This part adds the icons and part 3a creates a common stylesheet, but these icons aren't referenced in the stylesheet. I'd rather that you combine these patches and request review once you have a patch that references these new icons.
Attachment #8536108 - Flags: review?(jaws)
Attached patch Part 3 : Create common stylesheet (v2) (obsolete) (deleted) — — Splinter Review
Attachment #8536110 - Attachment is obsolete: true
Attachment #8550916 - Flags: review?(jaws)
Attached patch Part 4 : Update HTML structure (obsolete) (deleted) — — Splinter Review
Attachment #8550917 - Flags: review?(bmcbride)
Notes : - The changes in common.css are to fix a font issue, and to update the header color per spec - I added a partial checkbox style (it looks like a minus) for aboutSessionRestore
Attachment #8550918 - Flags: review?(jaws)
Attachment #8550918 - Flags: review?(bmcbride)
Attachment #8536108 - Flags: review?(jaws)
All the remaining parts will land together.
Keywords: leave-open
Attached image about:tabcrashed screenshot (deleted) —
Because everybody loves screenshots.
Attached image about:welcomeback screenshot (deleted) —
Comment on attachment 8550921 [details] about:welcomeback screenshot Err
Attachment #8550921 - Attachment description: about:sessionrestore screenshot → about:welcomeback screenshot
Attached image about:sessionrestore screenshot (deleted) —
Changes since last ui-review : - Added more spacing at the right of the welcomeback icon - Added divider between title and content - Reduced margin on top of the sessionrestore box
Attachment #8550927 - Flags: ui-review?(mmaslaney)
Attached patch Part 3 : Create common stylesheet (v2.1) (obsolete) (deleted) — — Splinter Review
Attachment #8550916 - Attachment is obsolete: true
Attachment #8550916 - Flags: review?(jaws)
Attachment #8550934 - Flags: review?(jaws)
Attachment #8550918 - Flags: review?(jaws)
Attachment #8550918 - Flags: review?(bmcbride)
Attachment #8550918 - Flags: review+
Comment on attachment 8550917 [details] [diff] [review] Part 4 : Update HTML structure Review of attachment 8550917 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml @@ -73,2 @@ > #ifdef XP_UNIX > - <button id="errorCancel" label="&restorepage.closeButton;" Why did the "errorCancel" ID get removed here? It is still referenced from http://hg.mozilla.org/mozilla-central/annotate/c2df1daf74c3/browser/components/sessionstore/content/aboutSessionRestore.js#l244 as far as I can tell. Same goes for errorTryAgain. I'm guessing that you removed these IDs because they are referenced by the netError.css files, but we'll need to keep the aboutSessionStore.js files (and related tests) working properly.
Attachment #8550917 - Flags: review?(bmcbride) → review-
Comment on attachment 8550934 [details] [diff] [review] Part 3 : Create common stylesheet (v2.1) Review of attachment 8550934 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following change made. The numbers that I provided below work well for me on my machine. ::: toolkit/themes/shared/in-content/info-pages.inc.css @@ +3,5 @@ > +body { > + display: flex; > + box-sizing: padding-box; > + min-height: 100vh; > + padding: 0 48px; When the browser is narrow, the background icon can get clipped half-way. http://screencast.com/t/LMJrdrk4Bso3 To fix this, you can add more padding on the start-side, and then remove the extra padding in the media query below. Something like: padding-top: 0; -moz-padding-end: 48px; padding-bottom: 0; -moz-padding-start: -moz-calc(48px + 4.6em); And then in the media query where the icon is hidden, just set the padding back to `padding: 0 48px;`
Attachment #8550934 - Flags: review?(jaws) → review+
Comment on attachment 8536108 [details] [diff] [review] Part 2 : Add icons (v1.2) Review of attachment 8536108 [details] [diff] [review]: ----------------------------------------------------------------- Please indent the session-restore.svg and welcome-back.svg files the same way that tab-crashed.svg is indented.
Attachment #8536108 - Flags: review?(jaws) → review+
Attached patch Part 2 : Add icons (v1.3) (obsolete) (deleted) — — Splinter Review
Thanks for the reviews ! I'll update the other patches later today.
Attachment #8536108 - Attachment is obsolete: true
Attachment #8552250 - Flags: review+
Attached patch Part 2 : Add icons (v1.4) (deleted) — — Splinter Review
Changes didn't make in my previous patch for some reason.
Attachment #8552250 - Attachment is obsolete: true
Attachment #8552439 - Flags: review+
Addressed comment.
Attachment #8550934 - Attachment is obsolete: true
Attachment #8552444 - Flags: review+
Restored errorTryAgain and errorCancel IDs for about:sessionrestore.
Attachment #8550917 - Attachment is obsolete: true
Attachment #8552445 - Flags: review?(jaws)
Comment on attachment 8552445 [details] [diff] [review] Part 4 : Update HTML structure (v1.1) Review of attachment 8552445 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8552445 - Flags: review?(jaws) → review+
Michael, this is ready to land, Can you give a ui-review any time soon ?
Flags: needinfo?(mmaslaney)
Blocks: 1124838
Blocks: 1097111
Comment on attachment 8550927 [details] about:sessionrestore screenshot The try results came back with no issues and there were no perf regressions found from the Talos suite. http://compare-talos.mattn.ca/?oldRevs=8efe164ce65f&newRev=9ef4400b77bc&server=graphs.mozilla.org&submit=true We can land this, and then any UI issues that are found can be filed as follow-ups.
Flags: needinfo?(mmaslaney)
Attachment #8550927 - Flags: ui-review?(mmaslaney)
Iteration: --- → 38.1 - 26 Jan
Depends on: 1125926
No longer depends on: 1125926
Depends on: 1125952
It would have been awesome if there had been a link between this and bug 1110511 so we didn't have multiple people working on the same thing
Blocks: 1110511
Depends on: 1127519
Depends on: 1127621
Depends on: 1127623
Tested this implementation on latest Nightly 38.0a1 under Windows 7 64-bit, Ubuntu 14.04 32-bit, Mac OS X 10.9.5 and 10.10 - more details are available in this pad: https://etherpad.mozilla.org/Bug1087618 Please note that a number of *potential issues* were encountered during the above testing. A feedback will be greatly appreciated before changing status. Thanks in advance!
Flags: needinfo?(ntim007)
Thanks for the complete testing ! 1) is intentional, to give more space to the content if the window is small. 2) isn't a regression of this bug (according to you), but if there isn't a bug filed yet, please file one. 3) & 4) should defintively get fixed, can you file a bug for both issues ? Thanks ! Feel free to change the status to verified :)
Flags: needinfo?(ntim007)
Depends on: 1128882
Depends on: 1128913
Depends on: 1128914
(In reply to Tim Nguyen [:ntim] from comment #69) > Thanks for the complete testing ! > 1) is intentional, to give more space to the content if the window is small. > 2) isn't a regression of this bug (according to you), but if there isn't a > bug filed yet, please file one. > 3) & 4) should defintively get fixed, can you file a bug for both issues ? > Thanks ! > > Feel free to change the status to verified :) Thanks for your input! Logged bug 1128882 for issues 3 & 4; bug 1128914 was logged for 2. Changing the status accordingly :)
Status: RESOLVED → VERIFIED
No longer depends on: 1128914
Depends on: 1133027
Depends on: 1133246
Release Note Request (optional, but appreciated) [Why is this notable]: Refreshed look for some about pages to match the in-content prefs (also shipping in 38) and the neterror pages. [Suggested wording]: New look for session restore and welcome back pages. [Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
(In reply to Ritu Kothari (:ritu) from comment #72) > Release note added to Firefox 41.0a2 This was meant for Firefox 38, not for 41, can you remove it from the release notes please ? Thanks !
Flags: needinfo?(rkothari)
Thanks Tim. I removed this from the notes.
Flags: needinfo?(rkothari)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #74) > Thanks Tim. I removed this from the notes. This entry still exists in the release notes for 41 https://www.mozilla.org/en-US/firefox/41.0a2/auroranotes/
> http://hg.mozilla.org/mozilla-central/rev/ef454d8c7614#l5.26 > 5.26 + background-image: url("chrome://browser/skin/aboutNetError_info.svg"); This bit is in toolkit and only works if you are Firefox (chrome://browser/...). Consider copying this SVG file to toolkit/themes/
(In reply to Philip Chee from comment #76) > > http://hg.mozilla.org/mozilla-central/rev/ef454d8c7614#l5.26 > > 5.26 + background-image: url("chrome://browser/skin/aboutNetError_info.svg"); > This bit is in toolkit and only works if you are Firefox > (chrome://browser/...). > Consider copying this SVG file to toolkit/themes/ Thanks, filed bug 1190961 to get this fixed.
(In reply to Kostas from comment #75) > (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #74) > > Thanks Tim. I removed this from the notes. > This entry still exists in the release notes for 41 > https://www.mozilla.org/en-US/firefox/41.0a2/auroranotes/ This also appears in the beta release notes : https://www.mozilla.org/en-US/firefox/41.0beta/releasenotes/
Flags: needinfo?(lmandel)
Ritu - Can you please fix the relnotes for 41?
Flags: needinfo?(lmandel) → needinfo?(rkothari)
Removed from FF41 release notes both Aurora and Beta. Reset the relmote-firefox flag.
relnote-firefox: 41+ → ---
Flags: needinfo?(rkothari)
Depends on: 1238181
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: