The default bug view has changed. See this FAQ.

social flyout panel doesn't adjust for dynamic content changes

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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...
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → mhammond
Attachment #663273 - Flags: feedback?(gavin.sharp)
Attachment #663273 - Flags: feedback?(felipc)
(Assignee)

Comment 2

5 years ago
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
Attachment #663273 - Attachment is obsolete: true
Attachment #663273 - Flags: feedback?(gavin.sharp)
Attachment #663273 - Flags: feedback?(felipc)
Attachment #663289 - Flags: feedback?(jaws)
Attachment #663289 - Flags: feedback?(gavin.sharp)
Attachment #663289 - Flags: feedback?(felipc)
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
Attachment #663289 - Flags: feedback?(jaws)
Attachment #663289 - Flags: feedback?(gavin.sharp)
Attachment #663289 - Flags: feedback?(felipc)
Attachment #663289 - Flags: feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 664361 [details] [diff] [review]
With requested tweaks, plus tests

With requested tweaks, plus tests
Attachment #663289 - Attachment is obsolete: true
Attachment #664361 - Flags: review?(jaws)
Whiteboard: [fx17]
tracking-firefox17: --- → +
Whiteboard: [fx17]
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.
Attachment #664361 - Flags: review?(jaws) → review+
Version: unspecified → Trunk
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c49a095aa9
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/31c49a095aa9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Attachment #664361 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/23ec9dcd89c5
status-firefox17: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.