Closed Bug 1080934 Opened 5 years ago Closed 5 years ago

about:welcomeback needs radio buttons

Categories

(Firefox :: General, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.1

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
This is what I think we want :)
Attachment #8502909 - Flags: feedback?(bmcbride)
now with working!
Attachment #8502909 - Attachment is obsolete: true
Attachment #8502909 - Flags: feedback?(bmcbride)
Attachment #8502958 - Flags: feedback?(bmcbride)
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 ":"
Attachment #8502958 - Flags: feedback?(bmcbride) → feedback+
Attached image about-welcomeback-all.png (obsolete) —
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?
Attachment #8504389 - Flags: ui-review?(mverdi)
Attached image radiobuttons.jpg (obsolete) —
(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)?
(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.
Attached image 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.
Attachment #8505043 - Attachment is obsolete: true
As requested, but there's probably better CSS I could be using.
Attachment #8502958 - Attachment is obsolete: true
Attachment #8505873 - Flags: feedback?(bmcbride)
Does this look OK?
Attachment #8504389 - Attachment is obsolete: true
Attachment #8504389 - Flags: ui-review?(mverdi)
Attachment #8505875 - Flags: ui-review?(mverdi)
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.
Attachment #8505873 - Flags: feedback?(bmcbride) → feedback+
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.
Attachment #8505873 - Attachment is obsolete: true
Comment on attachment 8505875 [details]
about-welcomback-stacked.png

This is good. Thanks Mark.
Attachment #8505875 - Flags: ui-review?(mverdi) → ui-review+
Attachment #8505875 - Flags: review?(bmcbride)
Comment on attachment 8505875 [details]
about-welcomback-stacked.png

(Presumably you meant this on the patch file)
Attachment #8505875 - Flags: review?(bmcbride)
Attachment #8506627 - Flags: review?(bmcbride)
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!!
Attachment #8506627 - Flags: review?(bmcbride) → review+
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Iteration: --- → 36.1
backed out in 5a7d636882fe for bc failures :(
(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?
Flags: needinfo?(bmcbride)
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"
Flags: needinfo?(bmcbride)
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
https://hg.mozilla.org/mozilla-central/rev/5cc3708f165a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.