Closed Bug 784879 Opened 7 years ago Closed 7 years ago

use collapsed instead of hidden for status panels

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx17])

Attachments

(1 file, 3 obsolete files)

using hidden is changing the content so it is not showing up correctly.  using collapsed allows the content to retain its correct appearance.
Attached patch use collapsed (obsolete) — Splinter Review
Attachment #654446 - Flags: review?(jaws)
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?
(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.
Duplicate of this bug: 785085
Attached patch use collapsed (obsolete) — Splinter Review
keeps panel hidden for startup, content will need to draw on the socialFrameShow event.
Attachment #654446 - Attachment is obsolete: true
Attachment #654446 - Flags: review?(jaws)
Attachment #654688 - Flags: review?(jaws)
Attachment #654688 - Flags: review?(gavin.sharp)
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).
Attached patch use collapsed (obsolete) — Splinter Review
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.
Attachment #654688 - Attachment is obsolete: true
Attachment #654688 - Flags: review?(jaws)
Attachment #654688 - Flags: review?(gavin.sharp)
Attachment #654702 - Flags: review?(gavin.sharp)
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.
Attachment #654702 - Flags: review?(gavin.sharp) → review+
Attached patch use collapsedSplinter Review
updated from comments
Attachment #654702 - Attachment is obsolete: true
Attachment #654710 - Flags: review?(gavin.sharp)
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?
(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.
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 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.
Attachment #654710 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa807b020b71
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/aa807b020b71
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Depends on: 1478576
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.