Closed
Bug 784879
Opened 12 years ago
Closed 12 years ago
use collapsed instead of hidden for status panels
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Whiteboard: [Fx17])
Attachments
(1 file, 3 obsolete files)
2.62 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
using hidden is changing the content so it is not showing up correctly. using collapsed allows the content to retain its correct appearance.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #654446 -
Flags: review?(jaws)
Comment 2•12 years ago
|
||
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?
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
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).
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
updated from comments
Attachment #654702 -
Attachment is obsolete: true
Attachment #654710 -
Flags: review?(gavin.sharp)
Comment 10•12 years ago
|
||
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?
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=77badc2b71dc
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa807b020b71
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Updated•12 years ago
|
Keywords: checkin-needed
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa807b020b71
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•