flyout may be misaligned horizontally after resizing

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified
Firefox 18
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Depends on: 798226

Comment 1

5 years ago
Mark, when does this happen and how should I reproduce it?
(Assignee)

Comment 2

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

Comment 3

5 years ago
Adding [Fx17] as I'm fairly confident that once you see it, you ain't gunna like it :)
Whiteboard: [Fx17]
(Assignee)

Comment 4

5 years ago
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.
Assignee: nobody → mhammond
Attachment #668745 - Flags: feedback?(mixedpuppy)
Attachment #668745 - Flags: feedback?(jaws)
Attachment #668745 - Flags: feedback?(felipc)
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 on attachment 668745 [details] [diff] [review]
Move the panel after resize

consider the changes I suggest in the other patch
Attachment #668745 - Flags: feedback?(mixedpuppy) → feedback+
Created attachment 668854 [details] [diff] [review]
V2
Created attachment 668855 [details] [diff] [review]
V2
Attachment #668836 - Attachment is obsolete: true
Attachment #668854 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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.
Attachment #668855 - Flags: review?(jaws)
Attachment #668855 - Flags: review?(gavin.sharp)
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.
Attachment #668855 - Flags: review?(jaws) → feedback+
Attachment #668745 - Flags: feedback?(jaws)
(Assignee)

Comment 11

5 years ago
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.
Attachment #668745 - Attachment is obsolete: true
Attachment #668855 - Attachment is obsolete: true
Attachment #668745 - Flags: feedback?(felipc)
Attachment #668855 - Flags: review?(gavin.sharp)
Attachment #669022 - Flags: review?(jaws)
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.
Attachment #669022 - Flags: review?(jaws) → review+
Summary: flyout may be misaligned. → flyout may be misaligned horizontally after resizing
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cf40690042
(Assignee)

Comment 14

5 years ago
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
Attachment #669022 - Attachment is obsolete: true
Attachment #669031 - Flags: review+
Attachment #669031 - Flags: approval-mozilla-aurora?
Attachment #669031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/24cf40690042
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
No longer depends on: 798226
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a4b3f04e27e
status-firefox17: --- → fixed
tracking-firefox17: --- → +
You need to log in before you can comment on or make changes to this bug.