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

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: [qa?])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

5 years ago
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.
Assignee: nobody → mhammond
Attachment #662400 - Flags: review?(jaws)
Attachment #662400 - Flags: review?(felipc)
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
Attachment #662400 - Flags: review?(felipc) → feedback+
(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.
(Assignee)

Comment 4

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

Comment 5

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

Comment 9

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

Comment 10

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

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/deda9b2561ab
(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.
Depends on: 793157
I filed bug 793157 for this and attached a patch.
https://hg.mozilla.org/mozilla-central/rev/deda9b2561ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Assignee)

Updated

5 years ago
No longer depends on: 793157
Attachment #662400 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc2f2eccce86
status-firefox17: --- → fixed
Is there a way we can verify this is fixed? Perhaps with Shane's socialapi-demo?
Whiteboard: [qa?]
You need to log in before you can comment on or make changes to this bug.