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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: InContent-icon-assets.zip

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

InContent Mocks: http://people.mozilla.org/~mmaslaney/incontent/inContent-restore-v1.png

Specs 1/2:
http://people.mozilla.org/~mmaslaney/incontent/inContent-restore-spec1.png

Specs 2/2:
http://people.mozilla.org/~mmaslaney/incontent/inContent-restore-spec2.png

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch v1

Comment 2

[Tim Nguyen :ntim]

Attachment: about:sessionrestore screenshot

Comment 3

[Tim Nguyen :ntim]

Attachment: about:welcomeback screenshot

Comment 4

[Tim Nguyen :ntim]

Attachment: about:tabcrashed screenshot

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8520808 [details] [diff] [review]
Patch v1

See attached screenshots.

Comment 6

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Removed one line of CSS that prevented about:sessionrestore from being vertically centered.

Comment 7

[Tim Nguyen :ntim]

So here's a summary of my changes :
- Moved netError.css main styles to the toolkit/themes/shared/ directory
- Moved aboutSessionRestore.css and aboutTabCrashed.css to the browser/themes/shared/ directory
- Refactored a bit aboutTabCrashed.xhtml to make the layout work with netError.css
- Added blue button styles (buttons with the class primary)
- Added the icons inside the browser/themes/shared/incontent-icons directory

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v1.2

Just realized I forgot to hg add a file.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v2

Added tree changes.

Comment 10

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Fixed a small issue with the tree.

Comment 11

[Tim Nguyen :ntim]

Attachment: Patch v2.2

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch v2.3

Fixed a small issue.

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v2.4

Improved about:blocked style

Comment 14

[Tim Nguyen :ntim]

Attachment: about:blocked screenshot

So about:blocked and about:robots are also affected. It doesn't look broken with the patch though.

Comment 15

[Tim Nguyen :ntim]

Attachment: about:robots screenshot

Comment 16

[Tim Nguyen :ntim]

Attachment: Patch v2.5

Forgot to hg add another file

Comment 17

[Michael Maslaney [:mmaslaney] ([e-mail])]

Tim, are there updated screen shots, or should I review the attachments from comments 2–4?

Comment 18

[Tim Nguyen :ntim]

(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.

Comment 19

[Tim Nguyen :ntim]

Can you also check if about:blocked and about:robots don't look too bad ?

Comment 20

[Michael Maslaney [:mmaslaney] ([e-mail])]

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.

Comment 21

[Tomasz Kołodziejski [:tomasz]]

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?

Comment 22

[Tim Nguyen :ntim]

(In reply to mmaslaney from comment #20)
> 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.

Thanks, will take care of those. For the first comment, I guess you mean there's too much margin on top of the restore window list.

(In reply to Tomasz Kołodziejski [:tomasz] from comment #21)
> 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 agree, I'll fix that in my next patch.

> * 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,
Yeah, but this is also an issue for about:neterror, which is why I want to leave that to another bug.

> * does this patch take care of the favicons?
Nope, this is handled in bug 988959 (or it's blocking bugs).

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8520949 [details] [diff] [review]
Patch v2.5

Review of attachment 8520949 [details] [diff] [review]:
-----------------------------------------------------------------

(Apologies for the lag - been a rough and busy few weeks)

::: browser/base/content/aboutTabCrashed.xhtml
@@ +22,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
>      <link rel="stylesheet" type="text/css" media="all"
> +          href="chrome://global/skin/netError.css"/>

Sadly, there's a long history of abusing netError.css :( We need to move away from that, using netError.css only in netError.xhtml. Bug 676795 comment 16 was the original plan for dealing with this.

This (and any other page) should only be loading it's own stylesheet, which in turn imports common.css. netError.css should only be used for netError.xhtml and only contain rules specific to that page. If there's anything in netError.css that should be shared, it should be moved into common.css.

Bug 1009370 did to about:privatebrowsing what you're doing here to these pages. So aboutPrivateBrowsing.css now some layout stuff that could be shared too.

We have a lot of pages like this - as we go through converting them, I don't want to end up in a situation where we duplicate CSS for every page, nor end up re-using netError.css.

These pages have a different layout than the more application-style layout of about:preferences and about:addons, but almost every other style is shared. So I figure there's two paths we could take:
* Use an attribute on the root element in these pages to select the page-style layout;
* Or have them import a common-lite.css (or whatever) that contains the stuff common to these pages but not in common.css - and have that in turn import common.css

(I think my personal preference is the 2nd option, as it reduces the overall amount of CSS imported but not used.)

@@ +37,2 @@
>  
> +      <div id="errorLongContent">

This is the sort of thing that re-using netError.css leads to - these pages shouldn't have to conform to how netError.xhtml is structured.

Comment 24

[Tim Nguyen :ntim]

Attachment: WIP

For anyone that wants to work on it, here's a rough WIP. It's pretty much a rebased version of Patch v2.5 with netError.css renamed to in-content/about-pages.css .

What you need to do :
- Change about-pages.css selectors to class selectors
- Change the structure of all pages depending on netError.css
- Change the stylesheet reference of all pages depending on netError.css
- Address issues from comment 20 and comment 21

Comment 25

[Tim Nguyen :ntim]

Attachment: Part 1 : Introduce new tree style

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.

Comment 26

[Tim Nguyen :ntim]

Attachment: Part 2 : Add icons

Comment 27

[Richard Marti (:Paenglab)]

Attachment: dialogTable.png

(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.

Comment 28

[Tim Nguyen :ntim]

Attachment: Part 1 : Introduce new tree style (v1.1)

Thanks ! Fixed the issue :)

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 31

[Tim Nguyen :ntim]

Attachment: Part 1 : Introduce new tree style (v1.2)

Used lowercase hex codes.

Comment 32

[Tim Nguyen :ntim]

Attachment: Part 2 : Add icons

Comment 33

[Tim Nguyen :ntim]

Comment on attachment 8535065 [details] [diff] [review]
Part 2 : Add icons

Icons should be in chrome://browser/skin/in-content/*.svg

Comment 34

[Tim Nguyen :ntim]

checkin-needed for part 1 only.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

(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 36

[Jared Wein [:jaws] (please needinfo? me)]

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/[e-mail]
> +  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.

Comment 37

[Tim Nguyen :ntim]

(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.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/63527327468c

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63527327468c

Comment 40

[Tim Nguyen :ntim]

Attachment: Part 1b - Tree styling followup

Small patch that removes redundant code.

Comment 41

[Tim Nguyen :ntim]

Attachment: Part 2 : Add icons (v1.2)

Comment 42

[Tim Nguyen :ntim]

Attachment: Part 3a - Create common stylesheet for "info" pages

Comment 43

[Tim Nguyen :ntim]

Comment on attachment 8536106 [details] [diff] [review]
Part 1b - Tree styling followup

Canceling review so I can submit a better patch in bug 1111504.

Comment 44

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 45

[Tim Nguyen :ntim]

Attachment: Part 3 : Create common stylesheet (v2)

Comment 46

[Tim Nguyen :ntim]

Attachment: Part 4 : Update HTML structure

Comment 47

[Tim Nguyen :ntim]

Attachment: Part 5 - Changes to specific stylesheets

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

Comment 48

[Tim Nguyen :ntim]

All the remaining parts will land together.

Comment 49

[Tim Nguyen :ntim]

Attachment: about:tabcrashed screenshot

Because everybody loves screenshots.

Comment 50

[Tim Nguyen :ntim]

Attachment: about:welcomeback screenshot

Comment 51

[Tim Nguyen :ntim]

Comment on attachment 8550921 [details]
about:welcomeback screenshot

Err

Comment 52

[Tim Nguyen :ntim]

Attachment: about:sessionrestore screenshot

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

Comment 53

[Tim Nguyen :ntim]

Attachment: Part 3 : Create common stylesheet (v2.1)

Comment 54

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

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;`

Comment 56

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 57

[Tim Nguyen :ntim]

Attachment: Part 2 : Add icons (v1.3)

Thanks for the reviews ! I'll update the other patches later today.

Comment 58

[Tim Nguyen :ntim]

Attachment: Part 2 : Add icons (v1.4)

Changes didn't make in my previous patch for some reason.

Comment 59

[Tim Nguyen :ntim]

Attachment: Part 3 : Create common stylesheet (v2.2)

Addressed comment.

Comment 60

[Tim Nguyen :ntim]

Attachment: Part 4 : Update HTML structure (v1.1)

Restored errorTryAgain and errorCancel IDs for about:sessionrestore.

Comment 61

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8552445 [details] [diff] [review]
Part 4 : Update HTML structure (v1.1)

Review of attachment 8552445 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 62

[Tim Nguyen :ntim]

Michael, this is ready to land, Can you give a ui-review any time soon ?

Comment 63

[Tim Nguyen :ntim]

Try pushes :
- With patches : https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ef4400b77bc

- Plain : https://treeherder.mozilla.org/#/jobs?repo=try&revision=8efe164ce65f

Comment 64

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d7545352a647
https://hg.mozilla.org/integration/fx-team/rev/ef454d8c7614
https://hg.mozilla.org/integration/fx-team/rev/e17f0262e63f
https://hg.mozilla.org/integration/fx-team/rev/7e64e2068d8a

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7545352a647
https://hg.mozilla.org/mozilla-central/rev/ef454d8c7614
https://hg.mozilla.org/mozilla-central/rev/e17f0262e63f
https://hg.mozilla.org/mozilla-central/rev/7e64e2068d8a

Comment 67

[Dave Townsend [:mossop]]

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

Comment 68

[Ada [:adalucinet]]

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!

Comment 69

[Tim Nguyen :ntim]

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 :)

Comment 70

[Ada [:adalucinet]]

(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 :)

Comment 71

[Tim Nguyen :ntim]

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)]:

Comment 72

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Release note added to Firefox 41.0a2

Comment 73

[Tim Nguyen :ntim]

(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 !

Comment 74

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Tim. I removed this from the notes.

Comment 75

[Kostas]

(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/

Comment 76

[Philip Chee]

> 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/

Comment 77

[Tim Nguyen :ntim]

(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.

Comment 78

[Tim Nguyen :ntim]

(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/

Comment 79

[Lawrence Mandel [:lmandel] (use needinfo)]

Ritu - Can you please fix the relnotes for 41?

Comment 80

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Removed from FF41 release notes both Aurora and Beta. Reset the relmote-firefox flag.