Closed Bug 790489 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: General, defect)

14 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: Unfocused, Assigned: raymondlee)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=css][lang=js][Snappy])

Attachments

(1 file, 2 obsolete files)

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.
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.
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.
Whiteboard: [good first bug][mentor=jaws][lang=css][lang=js]
Version: unspecified → 14 Branch
Attached patch v1 (obsolete) — Splinter Review
Attachment #664386 - Flags: review?(jaws)
Attachment #664386 - Attachment is patch: true
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #664386 - Flags: review?(jaws) → review?(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!
Attachment #664386 - Flags: review?(fryn) → review+
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 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?
Attachment #664386 - Flags: review+ → feedback+
Whiteboard: [good first bug][mentor=jaws][lang=css][lang=js] → [good first bug][mentor=jaws][lang=css][lang=js][Snappy]
(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.
Attached patch v2 (obsolete) — Splinter Review
> +#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.
Attachment #664784 - Flags: review?(jaws)
Attachment #664386 - Attachment is obsolete: true
Attachment #664784 - Flags: review?(jaws) → review+
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.
Attachment #664784 - Flags: feedback-
In other words, the aboutHome.xhtml and browser.js changes should be enough to fix this bug.
Attached patch v3Splinter Review
Just updated aboutHome.xhtml and browser.js
Attachment #664784 - Attachment is obsolete: true
Attachment #665047 - Flags: review?(jaws)
Attachment #665047 - Attachment is patch: true
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.
Attachment #665047 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/d0d8848ffd07
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.