Last Comment Bug 784879 - use collapsed instead of hidden for status panels
: use collapsed instead of hidden for status panels
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
Mentors:
: 785085 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 17:18 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-08-25 08:45 PDT (History)
4 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use collapsed (2.67 KB, patch)
2012-08-22 17:23 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
use collapsed (1.01 KB, patch)
2012-08-23 10:55 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
use collapsed (2.58 KB, patch)
2012-08-23 11:11 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
Details | Diff | Splinter Review
use collapsed (2.62 KB, patch)
2012-08-23 11:25 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-08-22 17:18:57 PDT
using hidden is changing the content so it is not showing up correctly.  using collapsed allows the content to retain its correct appearance.
Comment 1 Shane Caraveo (:mixedpuppy) 2012-08-22 17:23:49 PDT
Created attachment 654446 [details] [diff] [review]
use collapsed
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-22 18:10:33 PDT
The panel needs to stay initially hidden so as to not impact browser startup time.

Using collapsed for the iframes inside the panel makes sense, though. Is that alone sufficient to address the problems you're seeing?
Comment 3 Shane Caraveo (:mixedpuppy) 2012-08-22 18:33:46 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> The panel needs to stay initially hidden so as to not impact browser startup
> time.
> 
> Using collapsed for the iframes inside the panel makes sense, though. Is
> that alone sufficient to address the problems you're seeing?

no, if the panel is hidden when we preload the the status panels the content doesn't render correctly.  but we can require providers to reset their sizes when they get the socialFrameShow event, then we can have panel hidden on startup.
Comment 4 Shane Caraveo (:mixedpuppy) 2012-08-23 09:58:09 PDT
*** Bug 785085 has been marked as a duplicate of this bug. ***
Comment 5 Shane Caraveo (:mixedpuppy) 2012-08-23 10:55:20 PDT
Created attachment 654688 [details] [diff] [review]
use collapsed

keeps panel hidden for startup, content will need to draw on the socialFrameShow event.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-23 10:57:41 PDT
We can unhide the panel before we preload the panels. We just can't have it unhidden by default (e.g. when social is entirely disabled).
Comment 7 Shane Caraveo (:mixedpuppy) 2012-08-23 11:11:26 PDT
Created attachment 654702 [details] [diff] [review]
use collapsed

ok, this unhides the panel when updateButton is called, prior to setting the status iframes.  This works well, avoids the need for content to redraw the first time.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-23 11:14:53 PDT
Comment on attachment 654702 [details] [diff] [review]
use collapsed

Please define |let panel = document.getElementById("social-notification-panel");| rather than using notifBox.parentNode - make it much clearer what element you're referring to (and makes it easy to grep for references to social-notification-panel if we're refactoring things later).

r=me with that.
Comment 9 Shane Caraveo (:mixedpuppy) 2012-08-23 11:25:48 PDT
Created attachment 654710 [details] [diff] [review]
use collapsed

updated from comments
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-23 11:29:41 PDT
Comment on attachment 654710 [details] [diff] [review]
use collapsed

Hmm, actually I wonder whether we should continue to use "hidden" for the iframes. It reduces the memory footprint for unused documents, and it shouldn't have any impact on behavior?
Comment 11 Shane Caraveo (:mixedpuppy) 2012-08-24 12:32:52 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #10)
> Comment on attachment 654710 [details] [diff] [review]
> use collapsed
> 
> Hmm, actually I wonder whether we should continue to use "hidden" for the
> iframes. It reduces the memory footprint for unused documents, and it
> shouldn't have any impact on behavior?

Using hidden reflows some of the content ui to zero height/width and requires content to deal with that when visible.  I think this will be a non-obvious issue for providers in general.  Let me know what you want to do.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-24 12:38:10 PDT
I don't think hiding and then unhiding an iframe should have any side effects. Can you describe the problem you were seeing in more detail?
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-24 15:41:20 PDT
Comment on attachment 654710 [details] [diff] [review]
use collapsed

This is fine if it fixes the immediate problem. I want to investigate more later to see if we can somehow get away with hiding entirely rather than just collapsing, but that might be difficult given how we keep the content preloaded.
Comment 14 Shane Caraveo (:mixedpuppy) 2012-08-24 17:27:12 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:45:51 PDT
https://hg.mozilla.org/mozilla-central/rev/aa807b020b71

Note You need to log in before you can comment on or make changes to this bug.