1080934 - about:welcomeback needs radio buttons

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Broken out from bug 1063702 - about:welcomeback should have radio buttons so the user generally doesn't see or need to choose the tabs/windows to restore.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

This is what I think we want :)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0007-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

now with working!

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8502958 [details] [diff] [review]
0007-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

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

That'll teach me for being helpful ;)

::: browser/components/migration/content/aboutWelcomeBack.xhtml
@@ +54,5 @@
>          <div id="errorTrailerDesc">
> +          <div id="radioContainer">
> +            <div>
> +              <input id="radioRestoreAll" type="radio" name="restore" checked="checked"
> +                     onclick="updateTabListVisibility()"/>

Nit: It would be nice to not be adding any more inline scripts like this. It adds to the changes need in order to have bug 923920. But I wouldn't let it block landing this before the merge - your call.

::: browser/themes/shared/aboutWelcomeBack.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #errorPageContainer {
>    background-image: url("chrome://global/skin/icons/information-64.png");
> +  height:auto;

Nit: Space after ":"

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: about-welcomeback-all.png

This shows 3 variations of about:welcomeback.

* The first shows what it looks like on a "wide" screen - note how the buttons are aligned to the left and right edges.

* The second shows what it looks like as the screen narrows.  If it narrows too far, we do get some wrapping, but that's probably OK

* The last one shows what it looks like when the tree to select tabs/windows is visible - note how the first 2 images don't have the extra space for the tree below the buttons (as it is hidden) whereas in this one the button has moved down to make space for the tree.

verdi, is this what you had in mind?

Comment 5

[Verdi [:verdi] Best to slack me]

Attachment: radiobuttons.jpg

(In reply to Mark Hammond [:markh] from comment #4)
> 
> verdi, is this what you had in mind?

Hi Mark,
With the buttons aligned right and left there is too much space between them when wide and then not enough when narrow. Can you try something like 20px of padding between them instead (attached example)?

Comment 6

[Verdi [:verdi] Best to slack me]

(In reply to Verdi [:verdi] from comment #5)
> Created attachment 8505043 [details]
> radiobuttons.jpg
> 
> (In reply to Mark Hammond [:markh] from comment #4)
> > 
> > verdi, is this what you had in mind?
> 
> Hi Mark,
> With the buttons aligned right and left there is too much space between them
> when wide and then not enough when narrow. Can you try something like 20px
> of padding between them instead (attached example)?

On second thought, that won't work. The stings will be too long in some languages even when the window is wide. Let me give it more thought.

Comment 7

[Verdi [:verdi] Best to slack me]

Attachment: Stacked radio buttons

Ok after conferring with Philipp Sackl and Michael Maslaney, I think it would be better to stack the radio buttons. This will leave plenty of room for long strings. When the strings do wrap, the second line should align with the first and not the containing div. Also, the first radio button label should be the same distance below the horizontal rule as the text above is below the horizontal rule above it. And the radio buttons themselves should have some vertical space between them. I've attached a mockup with some sample long German strings.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

As requested, but there's probably better CSS I could be using.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: about-welcomback-stacked.png

Does this look OK?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8505873 [details] [diff] [review]
0003-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

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

Also need to address the issues in comment 3.

::: browser/components/migration/content/aboutWelcomeBack.xhtml
@@ +54,5 @@
>          <div id="errorTrailerDesc">
> +          <div>
> +            <div>
> +              <input id="radioRestoreAll" type="radio" name="restore" checked="checked"
> +                     onclick="updateTabListVisibility()"/>

Hm, this isn't getting a focus ring - so you can't tell where the focus is when navigating via keyboard. You'll need something like:

#radioRestoreAll:-moz-focusring,
#radioRestoreChoose:-moz-focusring {
  outline: 1px dotted black;
}

::: browser/components/sessionstore/content/aboutSessionRestore.js
@@ +87,5 @@
>  // User actions
> +function updateTabListVisibility() {
> +  let visible = document.getElementById("radioRestoreChoose").checked;
> +  let tabList = document.getElementById("tabList");
> +  tabList.style.display = visible ? "-moz-box" : "none";

I'd generally prefer to see this done via a property on the element (or one of it's parents), and a CSS selector matching that to hide/show the list.

::: browser/themes/shared/aboutWelcomeBack.css
@@ +16,5 @@
> +
> +.radioChooseLabel {
> +  display: block;
> +  margin-left: 25px;
> +  margin-top: -20px

This is bad :) You're only needing to manually position this because you've made the <label> a block. Just removing all this and letting it be inline (the default) should get you the behaviour you want.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

Thanks for the help Blair.  I've made all the suggested changes, including the ones I forgot from comment 3 :)  Not requesting feedback for this version - let's wait for Verdi to sign-off on the screen-shot first.

Comment 12

[Verdi [:verdi] Best to slack me]

Comment on attachment 8505875 [details]
about-welcomback-stacked.png

This is good. Thanks Mark.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8505875 [details]
about-welcomback-stacked.png

(Presumably you meant this on the patch file)

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8506627 [details] [diff] [review]
0003-Bug-1080934-add-radio-buttons-to-about-welcomeback.-.patch

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

::: browser/components/migration/content/aboutWelcomeBack.xhtml
@@ +53,5 @@
>          <!-- Short Description -->
>          <div id="errorTrailerDesc">
> +          <div>
> +            <div class="radioRestoreContainer">
> +              <input class="radioRestoreButton" id="radioRestoreAll" type="radio" 

Nit: THE HORROR OF TRAILING WHITESPACE. BEGONE, YOU FOUL DEMON! BEGONE!!

Comment 15

[Mark Hammond [:markh] [:mhammond]]

FOUL DEMON GONE!

https://hg.mozilla.org/integration/fx-team/rev/0b08eb8e264b

Comment 16

[Mark Hammond [:markh] [:mhammond]]

backed out in 5a7d636882fe for bc failures :(

Comment 17

[Mark Hammond [:markh] [:mhammond]]

(In reply to Blair McBride [:Unfocused] from comment #10)

> ::: browser/components/sessionstore/content/aboutSessionRestore.js
> @@ +87,5 @@
> >  // User actions
> > +function updateTabListVisibility() {
> > +  let visible = document.getElementById("radioRestoreChoose").checked;
> > +  let tabList = document.getElementById("tabList");
> > +  tabList.style.display = visible ? "-moz-box" : "none";
> 
> I'd generally prefer to see this done via a property on the element (or one
> of it's parents), and a CSS selector matching that to hide/show the list.

My most recent patch took this a little further - it includes:

function isTreeViewVisible() {
  let tabList = document.getElementById("tabList");
  return tabList.hasAttribute("available");
}

function initTreeView() {
  // If we aren't visible we initialize as we are made visible (and it's OK
  // to initialize multiple times)
  if (!isTreeViewVisible()) {
    return;
  }
...

So this attribute is used both by the CSS *and* by the JS code as a proxy for indicating if the treeView is visible or not.

So far, so good - except for the fact that about:sessionrestore *did not* define this attribute, so the code assumed the treeView was hidden when it wasn't, causing a test to fail (yay tests!)

A fix is:

+++ b/browser/components/sessionstore/content/aboutSessionRestore.xhtml
@@ -57,7 +57,7 @@
           <tree xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
                 id="tabList" flex="1" seltype="single" hidecolumnpicker="true"
                 onclick="onListClick(event);" onkeydown="onListKeyDown(event);"
-                _window_label="&restorepage.windowLabel;">
+                available="true" _window_label="&restorepage.windowLabel;">

An alternative would be to fall back to checking the computed style's display attribute, but I'm inclined to think the addition of the "available" attribute (even though it's not used by the css in aboutSessionRestore.xhtml, only aboutWelcomeBack) is better.

Blair, are you happy with that addition?

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, that'd work.

Alternative could be to change it so the code sets that attribute to false instead of removing it. Then isTreeviewVisible() just needs to check if getAttribute("available")!="false"

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Thanks!

(In reply to Blair McBride [:Unfocused] from comment #18)
> Yea, that'd work.
> 
> Alternative could be to change it so the code sets that attribute to false
> instead of removing it. Then isTreeviewVisible() just needs to check if
> getAttribute("available")!="false"

That kinda grates me the wrong way - I maybe should have gone for, eg, "hidden" (ie, negating the name of the attribute) in which case it would make more sense.  But that sounds pain for not alot of gain, so I re-pushed with the change in comment 17.

https://hg.mozilla.org/integration/fx-team/rev/5cc3708f165a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cc3708f165a

Comment 21

[Bogdan Maris, Desktop Test Engineering]

Found a few issues with e10s enabled, logged bugs on that bug 1098304 and bug 1098305. The implementation looks good from QA point of view, tested on different locales and LTR builds using latest Nightly across platforms. I will go and mark this verified as fixed based on my testing.