Based on copy and mocks found in Bug 1358915

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review179390 ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:49 (Diff revision 1) > - <li>&restorepage.startNew;</li> > + <li>&restorepage.startNew;</li> > - </ul> > + </ul> > + <p>&restorepage.stillProblems;</p> > + </div> > + </div> > + <p id="showTabs">&restorepage.showTabs; <img src="chrome://global/skin/icons/arrow-dropdown-16.svg"/></p> This lacks keyboard access. ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:67 (Diff revision 1) > <xul:treechildren flex="1"/> > </xul:tree> > </div> > - <div class="button-container"> > + <div > #ifdef XP_UNIX > - <xul:button id="errorCancel" > + class="button-container unix" Why this change? Changing the button order directly in the markup seems more straightforward. ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:9 (Diff revision 1) > > <!ENTITY restorepage.tabtitle "Restore Session"> > > <!-- LOCALIZATION NOTE: The title is intended to be apologetic and disarming, expressing dismay > and regret that we are unable to restore the session for the user --> > -<!ENTITY restorepage.errorTitle "Well, this is embarrassing."> > +<!ENTITY restorepage.errorTitle "Sorry. We're having trouble getting your pages back."> You need to change the entity names when changing the string, otherwise other locales might miss this change. ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:14 (Diff revision 1) > -<!ENTITY restorepage.errorTitle "Well, this is embarrassing."> > -<!ENTITY restorepage.problemDesc "&brandShortName; is having trouble recovering your windows and tabs. This is usually caused by a recently opened web page."> > -<!ENTITY restorepage.tryThis "You can try:"> > -<!ENTITY restorepage.restoreSome "Removing one or more tabs that you think may be causing the problem"> > -<!ENTITY restorepage.startNew "Starting an entirely new browsing session"> > +<!ENTITY restorepage.errorTitle "Sorry. We're having trouble getting your pages back."> > +<!ENTITY restorepage.problemDesc "We are having trouble restoring your last browsing session. Sometimes a page or two won’t load."> > +<!ENTITY restorepage.tryThis "Here are two things that might help:"> > +<!ENTITY restorepage.restoreSome "Choose Restore Tabs to try again."> > +<!ENTITY restorepage.startNew "Choose Close to restart without a session restore."> > +<!ENTITY restorepage.stillProblems "Still having problems getting your pages back? View previous tabs and remove the checkmark from the tabs you don’t need to recover or think might be causing a problem and then restore."> The second sentence seems overly long and cumbersome to parse. Can we break this up or make it shorter? Overall the old about:sessionrestore text seems clearer and easier to read to me. :/ ::: toolkit/themes/shared/in-content/info-pages.inc.css:128 (Diff revision 1) > width: 100%; > } > + > +/* New Illustrated Info Pages */ > +.illustrated .title-text { > + font-weight: 300; Does this actually make a difference? Over here this seems to have the same effect as "font-weight: lighter" which we already set on h1. ::: toolkit/themes/shared/in-content/info-pages.inc.css:133 (Diff revision 1) > + font-weight: 300; > +} > + > +.illustrated .title { > + margin-inline-start: 0em; > + padding-inline-start: 0em; nit: remove "em"

:dao Thanks for the review. About the copy - that was decided on by :mheubusch and her team, they are working hard to get the voice to be on brand for all of the error and info pages. If you'd like me to break it into smaller chunks of text for the localizers I can do that, but as for changing the way it is written or displayed I'd have to run it through a few people first. Just a note, the image will be changing slightly once I get a new version from :shorlander

The new illustration has been added and it should be ready to go now.

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review182690 ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:9 (Diff revision 3) > > <!ENTITY restorepage.tabtitle "Restore Session"> > > <!-- LOCALIZATION NOTE: The title is intended to be apologetic and disarming, expressing dismay > and regret that we are unable to restore the session for the user --> > -<!ENTITY restorepage.errorTitle "Well, this is embarrassing."> > +<!ENTITY restorepage.errorMessage "Sorry. We're having trouble getting your pages back."> Need a proper apostrophe ’, otherwise it will fail tests. ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.properties:5 (Diff revision 3) > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +# LOCALIZATION NOTE: %S will be replaced with the number of tabs to restore. You need to update the comment here https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

(In reply to Erica Wright [:ewright] from comment #4) > :dao Thanks for the review. About the copy - that was decided on by > :mheubusch and her team, they are working hard to get the voice to be on > brand for all of the error and info pages. I have no problem with establishing a consistent voice. My problem is that "View previous tabs and remove the checkmark from the tabs you don’t need to recover or think might be causing a problem and then restore" is a pretty long and unstructured sentence that's unnecessarily cumbersome to read. I don't think this has much to do with some particular tone we're trying to strike. > but as for changing > the way it is written or displayed I'd have to run it through a few people > first. For the sake of not blocking the illustration further, could spin off the text changes into a separate bug?

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review182704 ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:10 (Diff revision 3) > <!ENTITY restorepage.tabtitle "Restore Session"> > > <!-- LOCALIZATION NOTE: The title is intended to be apologetic and disarming, expressing dismay > and regret that we are unable to restore the session for the user --> > -<!ENTITY restorepage.errorTitle "Well, this is embarrassing."> > -<!ENTITY restorepage.problemDesc "&brandShortName; is having trouble recovering your windows and tabs. This is usually caused by a recently opened web page."> > +<!ENTITY restorepage.errorMessage "Sorry. We're having trouble getting your pages back."> > +<!ENTITY restorepage.errorDesc "We are having trouble restoring your last browsing session. Sometimes a page or two won’t load."> Another problem with the text changes: "Sometimes a page or two won’t load" is inaccurate and misleading. about:sessionrestore is displayed on startup when the previous sessions /crashed/.

We have the new copy now, and it's confirmed that we no longer need the "X tabs" counter, so I've removed that. Will add the copy in a separate patch once this is merged.

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review183642 Not completely sure if copy changes should be in this patch at this point. ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:18 (Diff revision 5) > <!ENTITY restorepage.startNew "Starting an entirely new browsing session"> > +<!ENTITY restorepage.hideTabs "Hide Previous Tabs"> > +<!ENTITY restorepage.showTabs "View Previous Tabs"> > > -<!ENTITY restorepage.tryagainButton "Restore"> > +<!ENTITY restorepage.restoreSession "Restore Session"> > <!ENTITY restorepage.restore.access "R"> Need to rename this (restorepage.restoreSession.accesskey) ::: browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd:20 (Diff revision 5) > +<!ENTITY restorepage.showTabs "View Previous Tabs"> > > -<!ENTITY restorepage.tryagainButton "Restore"> > +<!ENTITY restorepage.restoreSession "Restore Session"> > <!ENTITY restorepage.restore.access "R"> > -<!ENTITY restorepage.closeButton "Close"> > +<!ENTITY restorepage.newSession "Start New Session"> > <!ENTITY restorepage.close.access "C"> I guess you need an access key for this label? Please use .accesskey, not .access (i.e. restorepage.newSession.accesskey)

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review185468 ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:32 (Diff revision 6) > > - <body dir="&locale.dir;"> > + <body dir="&locale.dir;" class="illustrated"> > > <div class="container restore-chosen"> > - > + <div class="description-wrapper"> > + <div class="text-wrapper"> Can you cut this down to just one wrapper and rename it to "main-content"? ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:47 (Diff revision 6) > - <li>&restorepage.restoreSome;</li> > + <li>&restorepage.restoreSome;</li> > - <li>&restorepage.startNew;</li> > + <li>&restorepage.startNew;</li> > - </ul> > + </ul> > - </div> > + </div> > - </div> > + </div> > + <button id="showTabs" class="tabs-toggle"> I'm seeing some weird keyboard focus behavior when trying to tab through these elements. It seems like there's an invisble element taking focus. ::: browser/themes/shared/aboutSessionRestore.css:8 (Diff revision 6) > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > %endif > > +:root { > + --grey-40: #b1b1b3; --gray-40 appears to be unused ::: browser/themes/shared/aboutSessionRestore.css:34 (Diff revision 6) > + > +.tabs-toggle { > + cursor: pointer; > + color: var(--blue-50); > + -moz-context-properties: fill; > + fill: var(--blue-50); You can use fill: currentColor; and set color directly to #0a84ff without the variable. ::: browser/themes/shared/aboutSessionRestore.css:52 (Diff revision 6) > +} > + > +.tabs-toggle img { > + position: absolute; > + width: 20px; > + margin-left: 25px; margin-inline-start ::: browser/themes/shared/aboutSessionRestore.css:72 (Diff revision 6) > + max-height: none; > + min-height: 12em; > +} > + > +.button-container { > + text-align: right; text-align: inline-end;

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review185720 ::: browser/base/jar.mn:42 (Diff revision 7) > content/browser/abouthome/sync@2x.png (content/abouthome/sync@2x.png) > content/browser/abouthome/settings@2x.png (content/abouthome/settings@2x.png) > content/browser/abouthome/restore@2x.png (content/abouthome/restore@2x.png) > content/browser/abouthome/restore-large@2x.png (content/abouthome/restore-large@2x.png) > > + content/browser/illustrations/error-session-restore.svg (content/illustrations/error-session-restore.svg) Can you move this to browser/themes/? ::: browser/themes/shared/aboutSessionRestore.css:39 (Diff revision 7) > +#tabsToggle:hover { > + background-color: transparent; > +} > + > +#tabsToggle.show-tabs > span:first-child, > +#tabsToggle:not(.show-tabs) > span { Please use ids instead of relying on the order of the spans. ::: browser/themes/shared/aboutSessionRestore.css:59 (Diff revision 7) > + > +.arrow-up { > + transform: scaleY(-1); > +} > + > +.tree-container { Make this .tree-container:not(.expanded) ::: browser/themes/shared/aboutSessionRestore.css:63 (Diff revision 7) > + > +.tree-container { > + display: none; > +} > + > +.tree-container.expanded { ... and remove this rule

:dao it seems some css rules have changed since initially posting this. One thing I haven't been able to fix is the font size of the content in the `xul:tree` element. If you have any suggestions for that, or if you think it's okay let me know. It seems to have a font size of 11 now, despite all the rules telling it to have 15, and nothing I do will change it...

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review187894 ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:47 (Diff revision 8) > + <button id="tabsToggle" class="show-tabs"> > + <span id="showTabs"> > + &restorepage.showTabs; <img src="chrome://global/skin/icons/arrow-dropdown-16.svg"/> > + </span> > + <span id="hideTabs"> > + &restorepage.hideTabs; <img class="arrow-up" src="chrome://global/skin/icons/arrow-dropdown-16.svg"/> Can you move arrow-dropdown-16.svg to the stylesheet as a background-image? Looks good otherwise!

(In reply to Dão Gottwald [::dao] from comment #20) > Comment on attachment 8902397 [details] > Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, > > https://reviewboard.mozilla.org/r/173966/#review187894 > > ::: browser/components/sessionstore/content/aboutSessionRestore.xhtml:47 > (Diff revision 8) > > + <button id="tabsToggle" class="show-tabs"> > > + <span id="showTabs"> > > + &restorepage.showTabs; <img src="chrome://global/skin/icons/arrow-dropdown-16.svg"/> > > + </span> > > + <span id="hideTabs"> > > + &restorepage.hideTabs; <img class="arrow-up" src="chrome://global/skin/icons/arrow-dropdown-16.svg"/> > > Can you move arrow-dropdown-16.svg to the stylesheet as a background-image? > > Looks good otherwise! It's moved to a background image, because of the inversion it had to be added as an :after pseudo element. So long as that's okay with you I'll add checkin-needed to this bug.

Autoland can't push this until all open issues in MozReview are marked as resolved.

hg error in cmd: hg update --clean -r 5799b3ef8745: abort: unknown revision '5799b3ef8745'!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ff34580cb279 Update illustration for about:sessionrestore. ui-r=shorlander, r=dao

Backed out for failing many session store related tests due to exception, e.g. Marionette's test_refresh_firefox.py TestFirefoxRefresh.testReset: https://hg.mozilla.org/integration/autoland/rev/50d414550fa4fe84656254dbdcac5e811a7fe6d9 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ff34580cb279672bec10857c4ac5dc254a483119&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133429371&repo=autoland [task 2017-09-26T20:52:04.131Z] 20:52:04 INFO - TEST-START | browser/base/content/test/urlbar/browser_urlbar_blanking.js [task 2017-09-26T20:52:06.978Z] 20:52:06 INFO - TEST-INFO | started process screentopng [task 2017-09-26T20:52:08.450Z] 20:52:08 INFO - TEST-INFO | screentopng: exit 0 [task 2017-09-26T20:52:08.457Z] 20:52:08 INFO - Buffered messages logged at 20:52:04 [task 2017-09-26T20:52:08.457Z] 20:52:08 INFO - Entering test bound [task 2017-09-26T20:52:08.468Z] 20:52:08 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_blanking.js | The URL bar should be empty if we load a plain about:blank page. - [task 2017-09-26T20:52:08.470Z] 20:52:08 INFO - Buffered messages logged at 20:52:05 [task 2017-09-26T20:52:08.472Z] 20:52:08 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_blanking.js | The URL bar should be empty if we load a plain about:home page. - [task 2017-09-26T20:52:08.476Z] 20:52:08 INFO - Buffered messages logged at 20:52:06 [task 2017-09-26T20:52:08.479Z] 20:52:08 INFO - Console message: [JavaScript Warning: "Loading failed for the <script> with source “resource://onboarding/lib/UITour-lib.jsâ€." {file: "about:home" line: 1}] [task 2017-09-26T20:52:08.487Z] 20:52:08 INFO - TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_blanking.js | The URL bar should be empty if we load a plain about:privatebrowsing page. - [task 2017-09-26T20:52:08.491Z] 20:52:08 INFO - Buffered messages finished [task 2017-09-26T20:52:08.498Z] 20:52:08 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_urlbar_blanking.js | uncaught exception - TypeError: toggleTabs is null at window.onload@chrome://browser/content/aboutSessionRestore.js:27:3 [task 2017-09-26T20:52:08.501Z] 20:52:08 INFO - EventHandlerNonNull*@chrome://browser/content/aboutSessionRestore.js:19:1 [task 2017-09-26T20:52:08.507Z] 20:52:08 INFO - [task 2017-09-26T20:52:08.510Z] 20:52:08 INFO - Stack trace: [task 2017-09-26T20:52:08.516Z] 20:52:08 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652 [task 2017-09-26T20:52:08.522Z] 20:52:08 INFO - GECKO(1860) | JavaScript error: chrome://browser/content/aboutSessionRestore.js, line 27: TypeError: toggleTabs is null [task 2017-09-26T20:52:08.525Z] 20:52:08 INFO - Console message: [JavaScript Error: "TypeError: toggleTabs is null" {file: "chrome://browser/content/aboutSessionRestore.js" line: 27}] [task 2017-09-26T20:52:08.535Z] 20:52:08 INFO - window.onload@chrome://browser/content/aboutSessionRestore.js:27:3 [task 2017-09-26T20:52:08.537Z] 20:52:08 INFO - EventHandlerNonNull*@chrome://browser/content/aboutSessionRestore.js:19:1 [task 2017-09-26T20:52:08.542Z] 20:52:08 INFO

turns out this script is also used by `browser/components/migration/content/aboutWelcomeBack.xhtml`, which was when the error was occurring. fixed now, thank you :aryx.

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review189788 ::: browser/components/sessionstore/content/aboutSessionRestore.js:21 (Diff revision 10) > > // Page initialization > > window.onload = function() { > + if (document.body.classList.contains("illustrated")) { > + let toggleTabs = document.getElementById("tabsToggle"); This will fail again if we should add an illustration to about:welcomeback. Can you just null-check toggleTabs, or check window.location?

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 7cd29a64b86f -d 53da1770ecd1: rebasing 423187:7cd29a64b86f "Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, r=dao" (tip) merging browser/components/sessionstore/content/aboutSessionRestore.js merging browser/components/sessionstore/content/aboutSessionRestore.xhtml merging browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd merging browser/themes/shared/aboutSessionRestore.css merging browser/themes/shared/jar.inc.mn warning: conflicts while merging browser/components/sessionstore/content/aboutSessionRestore.xhtml! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cf0280811b62 Update illustration for about:sessionrestore. ui-r=shorlander, r=dao

Had to back this out for various failures like https://treeherder.mozilla.org/logviewer.html#?job_id=133907776&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=133907778&repo=autoland

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/859483e77e44 Backed out changeset cf0280811b62 for sessionstore related bc failures a=backout

try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1f800af1ab2fd03b72b2983fa8d27e71e40f82&selectedJob=134110286 I had to edit a few tests to make them work with the new restore page -:dao can you review again?

:dao could you look over this once more as I changed some things to fix the failing tests?

Comment on attachment 8902397 [details] Bug 1394457 - Update illustration for about:sessionrestore. ui-r=shorlander, https://reviewboard.mozilla.org/r/173966/#review191824 ::: browser/components/sessionstore/test/browser_590563.js:39 (Diff revision 13) > } > > -function middleClickTest(win) { > +async function middleClickTest(win) { > let browser = win.gBrowser.selectedBrowser; > + let tabsToggle = browser.contentDocument.getElementById("tabsToggle"); > + EventUtils.synthesizeMouse(tabsToggle, 10, 10, { button: 0 }, browser.contentWindow); Can you use synthesizeMouseAtCenter? ::: browser/components/sessionstore/test/browser_590563.js:41 (Diff revision 13) > -function middleClickTest(win) { > +async function middleClickTest(win) { > let browser = win.gBrowser.selectedBrowser; > + let tabsToggle = browser.contentDocument.getElementById("tabsToggle"); > + EventUtils.synthesizeMouse(tabsToggle, 10, 10, { button: 0 }, browser.contentWindow); > + let treeContainer = browser.contentDocument.querySelector(".tree-container") > + await ContentTaskUtils.waitForCondition(() => win.getComputedStyle(treeContainer).visibility == "visible"); Can you use BrowserTestUtils.waitForCondition? It looks you don't really need ContentTaskUtils.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d642aedb7ad6 Update illustration for about:sessionrestore. ui-r=shorlander, r=dao

Not going to try to uplift this to 57 -- let's have this ride the 58 release train.

Verified fixed using the latest Nightly (2017-10-10) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12