790489 - "Restore previous session" in about:home can flash visible on load

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The "Restore previous session" button in about:home defaults to visible, and is hidden when not needed. This can lead to that button flashing visible when about:home loads, and the button isn't meant to be visible. 

It should default to hidden and optionally be shown to avoid this.

Comment 1

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

We should determine what is needed most often and do that. Maybe session restore is available more often than not. I think step 1 of this bug would be to land a telemetry probe that reports if session restore is an option for each page load of about:home.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I don't think that should matter. If the machine is slow enough to cause any flash (hide -> show -> hide), its *very* noticeable. Alternatively, even a much longer delay before its shown (hide -> show) would be far less disruptive, because its far less visual noise.

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 4

[Frank Yan (:fryn)]

Comment on attachment 664386 [details] [diff] [review]
v1

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

r+ with comment addressed!

::: browser/base/content/abouthome/aboutHome.css
@@ +241,1 @@
>  #restorePreviousSessionSeparator {

Could you move this block to the location of the comment below?
This way, we won't have to worry as much about future changes to selector specificity messing this up, and it arguably makes the logic easier to follow.
Of course, I wrote this originally, so not your fault. :)

@@ +268,5 @@
>  }
>  
>  body[narrow] #restorePreviousSession {
>    font-size: 80%;
>  }

Put it here, please!

Comment 5

[Frank Yan (:fryn)]

Jared, thanks for the review baton pass. :)

Raymond, my review comment was misleading.
I actually mean to take the block that is:
 
+#launcher[session] > #restorePreviousSessionSeparator,
+#launcher[session] > #restorePreviousSession {
+  display: block;
+}

and move it after:

 
 body[narrow] #restorePreviousSession {
   font-size: 80%;
 }

Thanks for writing the patch! :)

Comment 6

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

Comment on attachment 664386 [details] [diff] [review]
v1

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

I don't think this patch does enough. It does move the CSS around in a way that looks like it will work, but http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.xhtml#57 shows that the |session| attribute is on the element by default.

Based on Blair's feedback, this attribute should only be placed on the element if it has been confirmed that session restore is available.

Please update http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2497 and http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2716 to reflect this reversal.

Also, I'm not sure why there are two places in our codebase that remove this attribute. Frank, do you know why?

Comment 7

[Frank Yan (:fryn)]

(In reply to Jared Wein [:jaws] from comment #6)
> Comment on attachment 664386 [details] [diff] [review]
> v1
> 
> Review of attachment 664386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this patch does enough. It does move the CSS around in a way
> that looks like it will work, but
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/
> aboutHome.xhtml#57 shows that the |session| attribute is on the element by
> default.
> 
> Based on Blair's feedback, this attribute should only be placed on the
> element if it has been confirmed that session restore is available.

Oh right. Good catch. Raymond, please change the JS, so that behaves as Jared explained.

> Please update
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#2497 and
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#2716 to reflect this reversal.
> 
> Also, I'm not sure why there are two places in our codebase that remove this
> attribute. Frank, do you know why?

Yes. The first instance removes the session attribute when it is discovered session restore is not available. The second instance removes the session attribute when the session restore button is clicked.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2

> +#launcher[session] > #restorePreviousSessionSeparator,
> +#launcher[session] > #restorePreviousSession {
> +  display: block;
> +}
> 
> and move it after:
> 
>  
>  body[narrow] #restorePreviousSession {
>    font-size: 80%;
>  }

Moved


> > I don't think this patch does enough. It does move the CSS around in a way
> > that looks like it will work, but
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/
> > aboutHome.xhtml#57 shows that the |session| attribute is on the element by
> > default.
> > 
> > Based on Blair's feedback, this attribute should only be placed on the
> > element if it has been confirmed that session restore is available.
> 
> Oh right. Good catch. Raymond, please change the JS, so that behaves as
> Jared explained.
> 
> > Please update
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> > js#2497 and
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> > js#2716 to reflect this reversal.
> > 
> > Also, I'm not sure why there are two places in our codebase that remove this
> > attribute. Frank, do you know why?
> 
> Yes. The first instance removes the session attribute when it is discovered
> session restore is not available. The second instance removes the session
> attribute when the session restore button is clicked.

Removed the |session| attribute on the element and changed the first instance to set the session attribute when session restore is available.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 664784 [details] [diff] [review]
v2

>-.launchButton[hidden],
>-#launcher:not([session]) > #restorePreviousSessionSeparator,
>-#launcher:not([session]) > #restorePreviousSession {
>+.launchButton[hidden] {
>   display: none;
> }

> #restorePreviousSessionSeparator {
>+  display: none;
> }

> #restorePreviousSession {
>+  display: none;
> }

>+#launcher[session] > #restorePreviousSessionSeparator,
>+#launcher[session] > #restorePreviousSession {
>+  display: block;
>+}

What's the purpose of shuffling the code around like this? It shouldn't behave any different from the original code.

Comment 10

[Dão Gottwald [:dao]]

In other words, the aboutHome.xhtml and browser.js changes should be enough to fix this bug.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v3

Just updated aboutHome.xhtml and browser.js

Comment 12

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

Comment on attachment 665047 [details] [diff] [review]
v3

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

Thanks for the feedback Dao and the quick turn-around Raymond.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d8848ffd07

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0d8848ffd07