Last Comment Bug 798212 - flyout may be misaligned horizontally after resizing
: flyout may be misaligned horizontally after resizing
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-04 21:57 PDT by Mark Hammond [:markh]
Modified: 2012-10-15 09:58 PDT (History)
6 users (show)
gavin.sharp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Move the panel after resize (4.61 KB, patch)
2012-10-06 00:48 PDT, Mark Hammond [:markh]
mixedpuppy: feedback+
Details | Diff | Review
v2 (8.62 KB, patch)
2012-10-06 15:16 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
V2 (6.44 KB, patch)
2012-10-06 17:16 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
V2 (6.44 KB, patch)
2012-10-06 17:18 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jaws: feedback+
Details | Diff | Review
updated to stick with getComputedStyle and margin sizes (4.86 KB, patch)
2012-10-07 22:51 PDT, Mark Hammond [:markh]
jaws: review+
Details | Diff | Review
Patch with review comments, as landed. (4.97 KB, patch)
2012-10-07 23:18 PDT, Mark Hammond [:markh]
markh: review+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mark Hammond [:markh] 2012-10-04 21:57:08 PDT
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] 2012-10-05 10:41:37 PDT
Mark, when does this happen and how should I reproduce it?
Comment 2 Mark Hammond [:markh] 2012-10-05 18:03:58 PDT
(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] 2012-10-05 18:30:19 PDT
Adding [Fx17] as I'm fairly confident that once you see it, you ain't gunna like it :)
Comment 4 Mark Hammond [:markh] 2012-10-06 00:48:20 PDT
Created attachment 668745 [details] [diff] [review]
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) [on leave 5/16-7/16] 2012-10-06 15:16:50 PDT
Created attachment 668836 [details] [diff] [review]
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) [on leave 5/16-7/16] 2012-10-06 15:17:42 PDT
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) [on leave 5/16-7/16] 2012-10-06 17:16:59 PDT
Created attachment 668854 [details] [diff] [review]
V2
Comment 8 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-10-06 17:18:15 PDT
Created attachment 668855 [details] [diff] [review]
V2
Comment 9 Mark Hammond [:markh] 2012-10-07 15:22:18 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-07 22:19:31 PDT
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] 2012-10-07 22:51:50 PDT
Created attachment 669022 [details] [diff] [review]
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-07 23:00:21 PDT
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 14 Mark Hammond [:markh] 2012-10-07 23:18:58 PDT
Created attachment 669031 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-10-08 01:29:43 PDT
https://hg.mozilla.org/mozilla-central/rev/24cf40690042
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-08 02:14:57 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a4b3f04e27e

Note You need to log in before you can comment on or make changes to this bug.