792295 - social flyout panel doesn't adjust to new yOffset while open.

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

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] [:mhammond]]

Attachment: 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 for replies!)]

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: [e-mail]]]

(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] [:mhammond]]

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] [:mhammond]]

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]

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)]

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]

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] [:mhammond]]

(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] [:mhammond]]

(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)]

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 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/deda9b2561ab

Comment 13

[Neil Deakin]

(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]

I filed bug 793157 for this and attached a patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/deda9b2561ab

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc2f2eccce86

Comment 17

[u279076]

Is there a way we can verify this is fixed? Perhaps with Shane's socialapi-demo?