792677 - social flyout panel doesn't adjust for dynamic content changes

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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] [:mhammond]]

Attachment: 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] [:mhammond]]

Attachment: 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)]

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] [:mhammond]]

Attachment: With requested tweaks, plus tests

With requested tweaks, plus tests

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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 6

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c49a095aa9

Comment 7

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/31c49a095aa9

Comment 8

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23ec9dcd89c5