877402 - panel sizing tests did not work correctly

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The recent change in bug 799014 didn't take into account the size of the arrowbox when sizing the panel.  Our intention is to size the iframe based on the size of content within it.  The tests didn't check the iframe size so we didn't catch that the change would size the iframe too small.  This patch fixes the iframe sizing as well as the test.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: panel size patch

This sets size using the iframe, which is easier than figuring out all the panel margins, arrow size, etc.  It also works well for panels like share, which have additional ui elements in the panel.

The test has to use the same calculations as the sizeto function in order to validate the iframe is at the correct size.

Comment 2

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

Comment on attachment 755651 [details] [diff] [review]
panel size patch

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

::: browser/base/content/test/social/browser_social_flyout.js
@@ +63,1 @@
>            // The width of the flyout should be 250px

this 250 should be 400 now, right?

@@ +64,5 @@
>            let iframe = panel.firstChild;
> +          let body = iframe.contentDocument.body;
> +          let cs = iframe.contentWindow.getComputedStyle(body);
> +          // same calculation as in sizeSocialPanelToContent
> +          let computedWidth = parseInt(cs.marginLeft) + body.offsetWidth + parseInt(cs.marginRight);

I don't think there is much value in duplicating the logic here, and having the test use 2 calculated values with a message that says "500px" (or "400px") is confusing.

How about just dropping all the computed* values and just using hard-coded 250 and 400 literals?

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: panel size patch

more deterministic tests

Comment 4

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=52accb6cf18a

Comment 5

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73253d9cee7e

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 755715 [details] [diff] [review]
panel size patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 799014
User impact if declined: social panels are sized too small so content does not appear correctly in panels
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low, only affects social arrow panels
String or IDL/UUID changes made by this patch: none

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73253d9cee7e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

It's not clear to me how to get this to apply to Aurora.

Comment 9

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

Wait, bug 799014 is 24-targeted. Do we actually need this on Aurora? Is this really 23:affected?

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> Wait, bug 799014 is 24-targeted. Do we actually need this on Aurora? Is this
> really 23:affected?

we didn't, my mistake, I had thought bug 799014 made it on 23.