Last Comment Bug 792677 - social flyout panel doesn't adjust for dynamic content changes
: social flyout panel doesn't adjust for dynamic content changes
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-19 22:52 PDT by Mark Hammond [:markh]
Modified: 2012-10-16 16:30 PDT (History)
4 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
use a mutation observer to dynamically resize and tweaks to the sizing itself (3.09 KB, patch)
2012-09-20 21:19 PDT, Mark Hammond [:markh]
no flags Details | Diff | Splinter Review
Use offsetHeight instead of scrollHeight (3.09 KB, patch)
2012-09-20 22:24 PDT, Mark Hammond [:markh]
jaws: feedback+
Details | Diff | Splinter Review
With requested tweaks, plus tests (5.36 KB, patch)
2012-09-24 22:31 PDT, Mark Hammond [:markh]
jaws: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mark Hammond [:markh] 2012-09-19 22:52:28 PDT
One of our providers uses the exact same URL for every flyout, and magically adjusts the content to suit whatever it is they want to show.

In effect, this means the social flyout is resized once for the initial load, but is never resized after that, meaning the panel is often an inappropriate size for the content being shown.

We can probably use mutation observers for this - https://github.com/mozilla/socialapi-dev/blob/develop/content/toolbarStatusWidget.js#L119 might be useful...
Comment 1 Mark Hammond [:markh] 2012-09-20 21:19:32 PDT
Created attachment 663273 [details] [diff] [review]
use a mutation observer to dynamically resize and tweaks to the sizing itself

Hopefully is fairly self-explanatory.  I did tweak the resizing itself as it has picked up a little cruft from the old addon we did for this that isn't necessary now.  Note that for some providers, the height of the notification panels may be too small, but AFAIK this is an issue on the provider side.
Comment 2 Mark Hammond [:markh] 2012-09-20 22:24:49 PDT
Created attachment 663289 [details] [diff] [review]
Use offsetHeight instead of scrollHeight

After chatting to Jaws on IRC, this new patch reverts to using offsetHeight instead of scrollHeight
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-09-24 10:26:18 PDT
Comment on attachment 663289 [details] [diff] [review]
Use offsetHeight instead of scrollHeight

Review of attachment 663289 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +199,5 @@
> +  // BUT - for at least one provider, the scrollWidth/offsetWidth/css width
> +  // isn't set appropriately, so the 330 is "fixed" for now...
> +  iframe.style.width = "330px";
> +  let height = body.offsetHeight || 300;
> +  iframe.style.height = height + "px";

offsetHeight won't include the marginTop or marginBottom. Can you add those in here too?

let cs = doc.defaultView.getComputedStyle(body);
let computedHeight = parseInt(cs.marginTop) + body.offsetHeight + parseInt(cs.marginBottom);
let height = computedHeight || 300;

@@ +217,5 @@
> +      mo.disconnect();
> +      mo = null;
> +    }
> +  }, false);
> +  // and resize it now.

nit: this comment isn't useful
Comment 4 Mark Hammond [:markh] 2012-09-24 22:31:29 PDT
Created attachment 664361 [details] [diff] [review]
With requested tweaks, plus tests

With requested tweaks, plus tests
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2012-09-25 13:57:38 PDT
Comment on attachment 664361 [details] [diff] [review]
With requested tweaks, plus tests

Review of attachment 664361 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +208,5 @@
> +  iframe.style.width = "330px";
> +  // offsetHeight doesn't include margins, so account for that.
> +  let cs = doc.defaultView.getComputedStyle(body);
> +  let computedHeight = parseInt(cs.marginTop) + body.offsetHeight + parseInt(cs.marginBottom);
> +  let height = computedHeight || 300;  

nit: trailing whitespace.

@@ +218,5 @@
> +  let mo = new iframe.contentWindow.MutationObserver(function(mutations) {
> +    sizeSocialPanelToContent(iframe);
> +  });
> +  // configuration of the observer - we want everything that could
> +  // cause the size to change.

// Observe anything that causes the size to change.

(The "configuration of the observer" part is implied.)

@@ +219,5 @@
> +    sizeSocialPanelToContent(iframe);
> +  });
> +  // configuration of the observer - we want everything that could
> +  // cause the size to change.
> +  let config = {attributes: true, childList: true, characterData: true, subtree: true};

nit: keep these in alphabetical order.

::: browser/base/content/test/browser_social_flyout.js
@@ +51,5 @@
> +    let port = Social.provider.getWorkerPort();
> +    ok(port, "provider has a port");
> +    port.onmessage = function (e) {
> +      let topic = e.data.topic;
> +      dump("MESSAGE: " + topic + "\n");

info("MESSAGE: " + topic + "\n");

@@ +75,2 @@
>    }
> +

nit: no need for a blank line here.

::: browser/base/content/test/social_flyout.html
@@ +14,5 @@
>          var port = navigator.mozSocial.getWorker().port;
>          port.postMessage({topic: "flyout-visibility", result: "hidden"});
>        }, false);
> +      window.addEventListener("socialTest-MakeWider", function(e) {
> +        document.body.setAttribute("style", "width: 500px;");

Please add the following after this:
document.body.offsetWidth; // force a layout flush

Could you dispatch an event here and have browser_social_flyout.js listen for that event? This should let us remove the executeSoon call.
Comment 7 Mounir Lamouri (:mounir) 2012-09-26 04:04:34 PDT
https://hg.mozilla.org/mozilla-central/rev/31c49a095aa9
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-02 18:20:13 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/23ec9dcd89c5

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