798212 - flyout may be misaligned horizontally after resizing

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

The flyout will sometimes such that the right-border moves.  As the anchor is on the right hand side, it may become mis-aligned with the anchor

Comment 1

[Asa Dotzler [:asa]]

Mark, when does this happen and how should I reproduce it?

Comment 2

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

(In reply to Asa Dotzler [:asa] from comment #1)
> Mark, when does this happen and how should I reproduce it?

Hover slowly over the news feed waiting for each item to show the flyout (ie, don't click anywhere - just allow the flyout to open on hover).  Eventually you will find a news feed item that is narrower than the "usual" size - when it is displayed the problem can be seen (the anchor arrow will be too far to the left).  You may need to scroll to find such an item.

Conversely, close the flyout, then start the above process by starting your hover over the one that caused the problem above.  Then hover over a different one such that the panel resizes wider.  In this case the anchor arrow will be too far to the right and the panel will be partially obscuring the news feed.

Comment 3

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

Adding [Fx17] as I'm fairly confident that once you see it, you ain't gunna like it :)

Comment 4

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

Attachment: Move the panel after resize

Attached is a patch that works around the problem by recording the position of the right-hand-side of the panel before we adjust the size, then moving the panel after the resize such that the right-hand-side ends up in the same position.  This is only done for panels with the anchor on the right.

If we take this patch, I'll open a new bug to remove this code once the underlying problem is resolved and add a reference to that new bug in a comment.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: v2

I applied the patch to see how it would work, and I got a very visual jumping around of the panel, so I wondered if using panel.sizeTo and panel.moveTo would fix it.  That didn't, but moving the panel before setting the iframe size does get rid of the visual jumpiness.

The previous css based calculations also didn't consider border or padding in content, using getBoundingClientRect allows us to get an accurate box for the body.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 668745 [details] [diff] [review]
Move the panel after resize

consider the changes I suggest in the other patch

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: V2

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: V2

Comment 9

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

Comment on attachment 668855 [details] [diff] [review]
V2

I think we should try and get this patch landed and into Aurora.  Note that the change to PANEL_WIN_HEIGHT might cause some flyouts to be too short today, but we can expect changes from the providers such that the calculated size will be correct making the smaller minimum appropriate.

Comment 10

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

Comment on attachment 668855 [details] [diff] [review]
V2

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

Reason for not granting r+ is to figure out why the height/width calculations got changed.

::: browser/base/content/browser-social.js
@@ +210,5 @@
>      return;
>    }
> +  let contentRect = doc.body.getBoundingClientRect();
> +
> +  let height = Math.max(contentRect.height, PANEL_MIN_HEIGHT);

Not sure why the height calculation got changed from using marginTop + offsetHeight + marginBottom. This calculation was also changed for the width. Can these changes be reverted?

@@ +211,5 @@
>    }
> +  let contentRect = doc.body.getBoundingClientRect();
> +
> +  let height = Math.max(contentRect.height, PANEL_MIN_HEIGHT);
> +  let hDiff = height - iframe.boxObject.height;

hDiff is unused and as discussed on IRC should be removed.

Comment 11

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

Attachment: updated to stick with getComputedStyle and margin sizes

A patch closer to the original, but the move happens before the size, which mixedpuppy reports causes less "shuddering" in his testing.

Comment 12

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

Comment on attachment 669022 [details] [diff] [review]
updated to stick with getComputedStyle and margin sizes

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

r=me with the one issue below addressed.

::: browser/base/content/browser-social.js
@@ +217,3 @@
>    let computedWidth = parseInt(cs.marginLeft) + body.offsetWidth + parseInt(cs.marginRight);
>    let width = Math.max(computedWidth, PANEL_MIN_WIDTH);
> +  let wDiff = width - iframe.boxObject.width;

This comment, https://bugzilla.mozilla.org/show_bug.cgi?id=792295#c6, states that boxObject.width is deprecated and suggests using getBoundingClientRect in its place.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/24cf40690042

Comment 14

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

Attachment: Patch with review comments, as landed.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The social flyout panel may be misaligned horizontally.
Testing completed (on m-c, etc.): Tested the alignment problem is resolved.
Risk to taking this patch (and alternatives if risky): Risks are limited to social functionality.
String or UUID changes made by this patch: None

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/24cf40690042

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a4b3f04e27e