Last Comment Bug 792295 - social flyout panel doesn't adjust to new yOffset while open.
: social flyout panel doesn't adjust to new yOffset while open.
Status: RESOLVED FIXED
[qa?]
:
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-09-18 19:54 PDT by Mark Hammond [:markh]
Modified: 2012-10-16 16:07 PDT (History)
5 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Move the panel with a new panel request is made and it is already open (1.06 KB, patch)
2012-09-18 20:07 PDT, Mark Hammond [:markh]
jaws: review+
felipc: feedback+
gavin.sharp: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Mark Hammond [:markh] 2012-09-18 19:54:40 PDT
If a social flyout panel is open and the provider requests a new popup, the popup URL changes correctly but the yOffset does not - it remains in the same position as it is currently open at.

It all works correctly when the panel is closed and reopened at a new position, just not when already open.
Comment 1 Mark Hammond [:markh] 2012-09-18 20:07:27 PDT
Created attachment 662400 [details] [diff] [review]
Move the panel with a new panel request is made and it is already open

Update the panel position if it is already open.  Slightly more painful than it should be, but it does work.  No tests as this is very hard to write sane tests for and the behaviour is easily verified visually.
Comment 2 :Felipe Gomes (needinfo me!) 2012-09-19 13:50:41 PDT
Comment on attachment 662400 [details] [diff] [review]
Move the panel with a new panel request is made and it is already open

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

Looks ok. Ideally the panel code would handle this situation, but I'm not sure how complicated would be to fix that.

Can you make it work with getBoundingClientRect()? boxObject is supposed to not be used anymore
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-19 14:17:19 PDT
(In reply to :Felipe Gomes from comment #2)
> Looks ok. Ideally the panel code would handle this situation, but I'm not
> sure how complicated would be to fix that.

We should ask Neil! Really seems like we shouldn't just wallpaper over this in the front-end.
Comment 4 Mark Hammond [:markh] 2012-09-19 21:19:16 PDT
A case could probably be made that openPopup still works as normal if the popup is already opened (ie, it would just move the popup).  Best I can tell, this is prevented by http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#1378, which declines to "open" an already open one.
Comment 5 Mark Hammond [:markh] 2012-09-19 22:48:12 PDT
http://adblockplus.org/blog/getting-screen-coordinates-for-an-html-element seems to talk about this issue a little and the code looks like it will be more complex, but I'll have a go at this tomorrow...
Comment 6 Neil Deakin 2012-09-20 06:56:03 PDT
Only the x,y,width and height properties of boxObject are deprecated. (getBoundingClientRect should be used instead). boxObject.screenX and box.screenY are not deprecated.
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-09-20 10:13:04 PDT
Comment on attachment 662400 [details] [diff] [review]
Move the panel with a new panel request is made and it is already open

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

Can you file a bug for fixing the core issue and add a comment in the code with a reference to the respective bug?
Comment 8 Neil Deakin 2012-09-20 12:57:09 PDT
The intent here is that you want to move the popup but keep it anchored? I don't see from where the open method you changed in this patch is called from.
Comment 9 Mark Hammond [:markh] 2012-09-20 16:30:38 PDT
(In reply to Neil Deakin from comment #8)
> The intent here is that you want to move the popup but keep it anchored?

We want to keep it anchored to the same element, but with a different yOffset.

> I don't see from where the open method you changed in this patch is called
> from.

It is called by providers, via the API we inject into their sidebar, at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/MozSocialAPI.jsm#113.  The providers *only* supply the yOffset and we provide the anchor internally (the anchor is the sidebar itself)
Comment 10 Mark Hammond [:markh] 2012-09-20 21:26:17 PDT
(In reply to Jared Wein [:jaws] from comment #7)
> Can you file a bug for fixing the core issue and add a comment in the code
> with a reference to the respective bug?

TBH, I'm not sure what the "core issue" is - is it "openPopup() should not be a noop when already open but instead move the panel if necessary"?  Or maybe that a new method specifically for this use-case is needed?  Neil, what are your thoughts?

Jaws: Given the comment the box object and screenX etc is OK, are you OK with this being landed?
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-09-20 21:34:46 PDT
The core issue being comment #3 and #4. I think openPopup should move the popup to that new location regardless of it is open or not. movePopup should move relative the current location, but openPopup should be using absolute location (i hope that makes sense).

Yeah, I'm fine with landing it now.
Comment 13 Neil Deakin 2012-09-21 04:28:36 PDT
(In reply to Mark Hammond (:markh) from comment #10)
> TBH, I'm not sure what the "core issue" is - is it "openPopup() should not
> be a noop when already open but instead move the panel if necessary"?  Or
> maybe that a new method specifically for this use-case is needed?

The latter.
Comment 14 Neil Deakin 2012-09-21 06:41:35 PDT
I filed bug 793157 for this and attached a patch.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-09-21 19:57:50 PDT
https://hg.mozilla.org/mozilla-central/rev/deda9b2561ab
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-02 18:19:31 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc2f2eccce86
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-16 16:07:13 PDT
Is there a way we can verify this is fixed? Perhaps with Shane's socialapi-demo?

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