Closed
Bug 779923
Opened 12 years ago
Closed 12 years ago
implement activity flyout panel
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
(4 files, 13 obsolete files)
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
The attached patch implements a flyout panel for the sidebar using the existing popup panel with an added browser element. The first of the two images show a bug when the panel is first opened, this happens with panels anchored to on a side, we would need to figure out the fix for this. The panel is green because the content I load for testing sets the content background to green when it has access to navigator.mozSocial.
Assignee | ||
Updated•12 years ago
|
Attachment #648406 -
Flags: feedback?(jaws)
Attachment #648406 -
Flags: feedback?(gavin.sharp)
Assignee | ||
Comment 5•12 years ago
|
||
another note, trying to shift the panel closer to the anchor causes the loss of the arrow, I'm assuming this is another generic panel bug.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 6•12 years ago
|
||
prototype for feedback
Attachment #648406 -
Attachment is obsolete: true
Attachment #648406 -
Flags: feedback?(jaws)
Attachment #648406 -
Flags: feedback?(gavin.sharp)
Attachment #648542 -
Flags: feedback?(mhammond)
Attachment #648542 -
Flags: feedback?(jaws)
Attachment #648542 -
Flags: feedback?(gavin.sharp)
Assignee | ||
Comment 7•12 years ago
|
||
figured out how to fix the first open problem, has to do with side and position attributes on the panel, which are defaulted for the typical panel use.
Attachment #648408 -
Attachment is obsolete: true
Attachment #648542 -
Attachment is obsolete: true
Attachment #648542 -
Flags: feedback?(mhammond)
Attachment #648542 -
Flags: feedback?(jaws)
Attachment #648542 -
Flags: feedback?(gavin.sharp)
Attachment #648822 -
Flags: feedback?(jaws)
Attachment #648822 -
Flags: feedback?(gavin.sharp)
Assignee | ||
Comment 8•12 years ago
|
||
This is a simple enough patch to get together, I'm marking this for 17
Whiteboard: [Fx17]
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mixedpuppy
Comment 9•12 years ago
|
||
Comment on attachment 648822 [details] [diff] [review] simple flyout patch Review of attachment 648822 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.xul @@ +280,5 @@ > </deck> > </panel> > + <panel id="social-flyout-panel" > + class="social-arrow-panel" > + side="right" This doesn't look like it will work for RTL. ::: browser/themes/pinstripe/browser.css @@ +3511,5 @@ > margin-right: -1px; > -moz-transform: scaleX(-1); > } > > +.social-arrow-panel .panel-arrow[side="right"] { The side="left" and side="right" here can be simplified if there was a side="horiz-end" type of value. Then you could do: .social-panel-arrow[side="horiz-end"] { list-style-image: url("..."); -moz-margin-end: -1px; } .social-panel-arrow[side="horiz-end"]:-moz-locale-dir(rtl) { transform: scaleX(-1); } ::: toolkit/components/social/MozSocialAPI.jsm @@ +181,5 @@ > + let doc = notifBrowser.contentDocument; > + // "notif" is an implementation detail that we should get rid of > + // eventually > + let body = doc.getElementById("notif") || (doc.body && doc.body.firstChild); > + if (!body) { We should be using the measurements from doc.getElementById("notif").firstChild. So this code should be: let body = doc.getElementById("notif") || doc.body; if (!body || !body.firstChild) { return; } See the patch for bug 777176 for some of the tweaks that I made to this function to get it to work correctly with the panel sizing.
Attachment #648822 -
Flags: feedback?(jaws) → feedback+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #9) > > </panel> > > + <panel id="social-flyout-panel" > > + class="social-arrow-panel" > > + side="right" > > This doesn't look like it will work for RTL. > > +.social-arrow-panel .panel-arrow[side="right"] { > > The side="left" and side="right" here can be simplified if there was a > side="horiz-end" type of value. The "side" attribute is a part of the panel xbl and must be set for right/left to appear correctly on the first popup. > We should be using the measurements from > doc.getElementById("notif").firstChild. So this code should be: > let body = doc.getElementById("notif") || doc.body; > if (!body || !body.firstChild) { > return; > } > > See the patch for bug 777176 for some of the tweaks that I made to this > function to get it to work correctly with the panel sizing. yeah, iirc I saw that after I did the patch, we need to perhaps move that to a utility function somewhere.
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #648822 -
Attachment is obsolete: true
Attachment #648822 -
Flags: feedback?(gavin.sharp)
Attachment #651919 -
Flags: feedback?(felipc)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #651919 -
Attachment is obsolete: true
Attachment #651919 -
Flags: feedback?(felipc)
Attachment #652591 -
Flags: review?(jaws)
Attachment #652591 -
Flags: review?(felipc)
Assignee | ||
Comment 13•12 years ago
|
||
update event names for show/hide
Attachment #652591 -
Attachment is obsolete: true
Attachment #652591 -
Flags: review?(jaws)
Attachment #652591 -
Flags: review?(felipc)
Attachment #652957 -
Flags: review?(jaws)
Attachment #652957 -
Flags: review?(felipc)
Comment 14•12 years ago
|
||
Comment on attachment 652957 [details] [diff] [review] updated patch Review of attachment 652957 [details] [diff] [review]: ----------------------------------------------------------------- I see changes for pinstripe/browser.css and winstripe/browser.css, should there be changes to gnomestripe? ::: browser/base/content/browser.xul @@ +272,5 @@ > <box id="social-notification-box" flex="1"></box> > </panel> > + <panel id="social-flyout-panel" > + class="social-arrow-panel" > + side="right" Where does side get changed to "right"? Could it ever be "top" or "bottom"? ::: browser/base/content/test/browser_social_flyout.js @@ +28,5 @@ > + case "got-sidebar-message": > + port.postMessage({topic: "test-flyout-open"}); > + break; > + case "got-flyout-visibility": > + ok(e.data.result == "hidden", "flyout visibility "+e.data.result); is(e.data.result, "hidden", "flyout visibility should be hidden"); ::: browser/base/content/test/social_flyout.html @@ +12,5 @@ > + }, false); > + window.addEventListener("socialFrameHide", function(e) { > + var port = navigator.mozSocial.getWorker().port; > + port.postMessage({topic: "flyout-visibility", result: "hidden"}); > + nit: trailing whitespace ::: browser/themes/pinstripe/browser.css @@ +3512,2 @@ > list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png"); > margin-right: -1px; I'm not sure why separate arrow panel images are needed instead of using the already existing arrow images. Either way, we don't need to duplicate some of these rules. This can be: > .social-arrow-panel .panel-arrow[side="left"], > .social-arrow-panel .panel-arrow[side="right"] { > list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png"); > -moz-margin-end: -1px; > } > > .social-arrow-panel .panel-arrow[side="left"] { > transform: scaleX(-1); > } @@ +3513,5 @@ > margin-right: -1px; > -moz-transform: scaleX(-1); > } > > +.social-arrow-panel .panel-arrow[side="right"] { Since you're changing these, please convert them to use the child selector (>). ::: toolkit/components/social/MozSocialAPI.jsm @@ +205,5 @@ > + // create and initialize the panel for this window > + let iframe = chromeWindow.document.createElement("iframe"); > + iframe.setAttribute("type", "content"); > + iframe.setAttribute("flex", "1"); > + iframe.setAttribute("mozframetype", "content"); If you are using a XUL:iframe, then you won't need to set the mozframetype attribute. That is only needed for HTML:iframes. @@ +210,5 @@ > + panel.appendChild(iframe); > + > + panel.addEventListener("popupshowing", function onpopupshowing() { > + let docShell = iframe.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell); > + docShell.isActive = true; This QI isn't needed if you use a XUL:iframe (https://developer.mozilla.org/en-US/docs/XUL/iframe#p-docShell). @@ +248,5 @@ > + if (!body || !body.firstChild) { > + return; > + } > + > + let [height, width] = [body.firstChild.offsetHeight || 300, 330]; For chat, I think we'll want to limit the height of the chat window to 300, so please add a check like so after this: > height = height > 300 ? 300 : height;
Attachment #652957 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #14) > Comment on attachment 652957 [details] [diff] [review] > @@ +248,5 @@ > > + if (!body || !body.firstChild) { > > + return; > > + } > > + > > + let [height, width] = [body.firstChild.offsetHeight || 300, 330]; > > For chat, I think we'll want to limit the height of the chat window to 300, > so please add a check like so after this: > > height = height > 300 ? 300 : height; The flyout panel is not chat.
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Assignee | ||
Comment 19•12 years ago
|
||
updated from feedback + removed custom panel css for osx.
Attachment #652957 -
Attachment is obsolete: true
Attachment #652957 -
Flags: review?(felipc)
Attachment #653534 -
Flags: review?(jaws)
Attachment #653534 -
Flags: review?(gavin.sharp)
Comment 20•12 years ago
|
||
Comment on attachment 653534 [details] [diff] [review] flyout patch with tests >- list-style-image: url("chrome://browser/skin/social/panelarrow-up.png"); >- list-style-image: url("chrome://browser/skin/social/panelarrow-down.png"); >- list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png"); >- list-style-image: url("chrome://browser/skin/social/panelarrow-horiz.png"); You need to completely remove these files from the theme.
Assignee | ||
Comment 21•12 years ago
|
||
removed arrow images
Attachment #653534 -
Attachment is obsolete: true
Attachment #653534 -
Flags: review?(jaws)
Attachment #653534 -
Flags: review?(gavin.sharp)
Attachment #653537 -
Flags: review?(jaws)
Attachment #653537 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 22•12 years ago
|
||
sigh...remember jar as well.
Attachment #653537 -
Attachment is obsolete: true
Attachment #653537 -
Flags: review?(jaws)
Attachment #653537 -
Flags: review?(gavin.sharp)
Attachment #653543 -
Flags: review?(jaws)
Attachment #653543 -
Flags: review?(gavin.sharp)
Comment 23•12 years ago
|
||
Comment on attachment 653543 [details] [diff] [review] flyout patch with tests Review of attachment 653543 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_social_flyout.js @@ +28,5 @@ > + case "got-sidebar-message": > + port.postMessage({topic: "test-flyout-open"}); > + break; > + case "got-flyout-visibility": > + ok(e.data.result == "hidden", "flyout visibility is hidden"); is(e.data.result, "hidden", "flyout visibility should be 'hidden'"); @@ +31,5 @@ > + case "got-flyout-visibility": > + ok(e.data.result == "hidden", "flyout visibility is hidden"); > + next(); > + case "got-flyout-message": > + ok(true, "got flyout message"); This should check e.data.result, which would show if the |window| was valid or not. ::: toolkit/components/social/MozSocialAPI.jsm @@ +264,5 @@ > + notifBrowser.style.height = height + "px"; > + } > + > + let lastOrigin = notifBrowser.getAttribute("origin"); > + if (lastOrigin != provider.origin) { How do you foresee the origin attribute getting out of date?
Comment 24•12 years ago
|
||
Comment on attachment 653543 [details] [diff] [review] flyout patch with tests Review of attachment 653543 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/social/MozSocialAPI.jsm @@ +199,5 @@ > + if (!fullURL) > + return; > + > + // assuming that we are using a deck, and the last browser element is reserved > + // for flyouts this comment seems outdated, doesn't reflect the code could you split the first-time panel creation in a separate function, and leave this one only about setting the URL and displaying the panel? and then in this function you could do: let notifBrowser = panel.firstChild ? panel.firstChild : createFlyoutFrame(..)
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #23) > Comment on attachment 653543 [details] [diff] [review] > ::: toolkit/components/social/MozSocialAPI.jsm > @@ +264,5 @@ > > + notifBrowser.style.height = height + "px"; > > + } > > + > > + let lastOrigin = notifBrowser.getAttribute("origin"); > > + if (lastOrigin != provider.origin) { > > How do you foresee the origin attribute getting out of date? changing the provider will at a minimum unset origin.
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #653543 -
Attachment is obsolete: true
Attachment #653543 -
Flags: review?(jaws)
Attachment #653543 -
Flags: review?(gavin.sharp)
Attachment #653973 -
Flags: review?(jaws)
Attachment #653973 -
Flags: review?(felipc)
Comment 27•12 years ago
|
||
If the provider changes, we should just remove all of the iframes that were constructed as part of the previous provider. Either way, I think we should remove multiprovider focused parts of this patch until multiprovider support has landed.
Assignee | ||
Comment 28•12 years ago
|
||
this patch changes direction a bit, moving the bulk of the panel code into browser-social.js
Attachment #654291 -
Flags: review?(jaws)
Comment 29•12 years ago
|
||
Comment on attachment 654291 [details] [diff] [review] flyout-v2 Review of attachment 654291 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +232,5 @@ > + if (!panel.firstChild) > + return > + panel.removeEventListener("popupshown", this.onShown.bind(this)); > + panel.removeEventListener("popuphiding", this.onHidden.bind(this)); > + window.removeEventListener("unload", this.destroyFrame.bind(this)); .bind() will return a new instance of the function, so this removeEventListener won't actually remove the listener. One way to fix this is to store a reference to the bound function and use that reference for removing the event listener. @@ +233,5 @@ > + return > + panel.removeEventListener("popupshown", this.onShown.bind(this)); > + panel.removeEventListener("popuphiding", this.onHidden.bind(this)); > + window.removeEventListener("unload", this.destroyFrame.bind(this)); > + panel.removeChild(panel.firstChild); We only remove the firstChild, but if createFrame is called multiple times then many children can exist. If we want to limit this to only one child of the panel, then we should enforce that limit within createFrame as well as destroyFrame. Also, I think createFrame and destroyFrame should have underscore prefixes since they shouldn't be called externally. @@ +268,5 @@ > + panel.hidden = false; > + let iframe = panel.firstChild; > + > + > + let src = iframe.getAttribute("src"); nit: only one blank line above and also remove trailing whitespace in this patch. ::: browser/base/content/test/social_sidebar.html @@ +9,5 @@ > var topic = e.data.topic; > switch (topic) { > + case "test-flyout-open": > + navigator.mozSocial.openPanel("social_flyout.html", function(flyoutWin) { > + port.postMessage({topic: "flyout-opened", result: flyoutWin ? "ok" : "failed"}); Where is "flyout-opened" used? I don't see it anywhere else in this patch. ::: browser/themes/winstripe/browser.css @@ +3443,5 @@ > #social-statusarea-username:hover { > text-decoration: underline; > } > > +.social-arrow-panel { Lets get rid of the social-arrow-panel class. The height and width are explicitly set in sizeSocialPanelToContent and the other styles that used .social-arrow-panel have been removed in this patch.
Attachment #654291 -
Flags: review?(jaws) → review-
Comment 30•12 years ago
|
||
Comment on attachment 653973 [details] [diff] [review] updated from comments Clearing review on the older patch as Shane has requested to use the newer version going forward.
Attachment #653973 -
Attachment is obsolete: true
Attachment #653973 -
Flags: review?(jaws)
Attachment #653973 -
Flags: review?(felipc)
Assignee | ||
Comment 31•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #29) > Comment on attachment 654291 [details] [diff] [review] > flyout-v2 > @@ +233,5 @@ > > + return > > + panel.removeEventListener("popupshown", this.onShown.bind(this)); > > + panel.removeEventListener("popuphiding", this.onHidden.bind(this)); > > + window.removeEventListener("unload", this.destroyFrame.bind(this)); > > + panel.removeChild(panel.firstChild); > > We only remove the firstChild, but if createFrame is called multiple times > then many children can exist. If we want to limit this to only one child of > the panel, then we should enforce that limit within createFrame as well as > destroyFrame. I'll limit it to one in createFrame. > Also, I think createFrame and destroyFrame should have underscore prefixes > since they shouldn't be called externally. destroyFrame would be called in the case you disable the feature (I thought I had put that in the patch)
Comment 32•12 years ago
|
||
Yeah, it's in the patch, but the naming of the function makes it seem quite internal.
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #654291 -
Attachment is obsolete: true
Attachment #654427 -
Flags: review?(jaws)
Comment 34•12 years ago
|
||
Comment on attachment 654427 [details] [diff] [review] flyout-v2 Review of attachment 654427 [details] [diff] [review]: ----------------------------------------------------------------- r+ from me with the following issues addressed. ::: browser/base/content/browser-social.js @@ +17,5 @@ > }, > > // Called on window unload > uninit: function SocialUI_uninit() { > + SocialFlyout.unload(); Do we need to be removing DOM elements on window unload? Seems like that is taken care of already when the window is being unloaded itself. @@ +262,5 @@ > + this._createFrame(); > + panel.hidden = false; > + let iframe = panel.firstChild; > + > + carry over from previous review, please remove this extra blank line and trailing whitespace in this patch. @@ +272,5 @@ > + if (aCallback) > + aCallback(iframe.contentWindow); > + }, true); > + iframe.setAttribute("src", aURL); > + } I think the callback should still be called if the src == aURL. ::: browser/base/content/browser.xul @@ +267,5 @@ > #endif > </hbox> > </panel> > > + <panel id="social-notification-panel" class="social-arrow-panel" type="arrow" hidden="true" noautofocus="true"> We no longer need the class="social-arrow-panel" on these two panels. ::: toolkit/components/social/MozSocialAPI.jsm @@ +193,5 @@ > } > return fullURL; > } > > +function openPanel(chromeWindow, provider, url, offset, callback) { Does this need to be a separate function? I think the body of this function can be moved inside of MozSocialAPI.openPanel.
Attachment #654427 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 35•12 years ago
|
||
updated with last comments
Attachment #654427 -
Attachment is obsolete: true
Attachment #654866 -
Flags: review?(jaws)
Updated•12 years ago
|
Attachment #654866 -
Attachment description: flyout-v2 → flyout-v2.1
Updated•12 years ago
|
Attachment #654866 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 36•12 years ago
|
||
IMO This can land and we can update css as appropriate for the panels on osx separately.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 38•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/772c3eed0cfb
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Comment 39•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/772c3eed0cfb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•