Closed Bug 877402 Opened 8 years ago Closed 8 years ago

panel sizing tests did not work correctly

Categories

(Firefox Graveyard :: SocialAPI, defect)

24 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox24 fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox24 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch panel size patch (obsolete) — Splinter Review
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.
Assignee: nobody → mixedpuppy
Attachment #755651 - Flags: review?(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?
Attachment #755651 - Flags: review?(mhammond) → review+
Attached patch panel size patchSplinter Review
more deterministic tests
Attachment #755651 - Attachment is obsolete: true
Attachment #755715 - Flags: review+
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
Attachment #755715 - Flags: approval-mozilla-aurora?
Attachment #755715 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/73253d9cee7e
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
It's not clear to me how to get this to apply to Aurora.
Wait, bug 799014 is 24-targeted. Do we actually need this on Aurora? Is this really 23:affected?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #755715 - Flags: approval-mozilla-aurora+
Version: 23 Branch → 24 Branch
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.