Closed Bug 797209 Opened 9 years ago Closed 8 years ago

flyout panel can appear anchored outside of the sidebar

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(firefox18-)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox18 - ---

People

(Reporter: mixedpuppy, Assigned: markh)

References

Details

Attachments

(2 files, 2 obsolete files)

the flyout panel can be opened at a vertical offset.  We need to ensure the offset is in a range that matches the sidebar, it can bet set higher or lower than the sidebar.
Whiteboard: [Fx17]
(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> the flyout panel can be opened at a vertical offset.  We need to ensure the
> offset is in a range that matches the sidebar, it can bet set higher or
> lower than the sidebar.

Who's going to take this bug? All tracked bugs should have an assignee.
Assignee: nobody → mixedpuppy
If this turns out to be non-trivial, I think we can let this slip Fx17 without any real issues.
(In reply to Mark Hammond (:markh) from comment #2)
> If this turns out to be non-trivial, I think we can let this slip Fx17
> without any real issues.

Actually, I take that back seeing it can be demonstrated today:

* Scroll the sidebar downwards such that an item that could cause a flyout is only partially showing (ie, so the majority of the item has scrolled off the top of the sidebar).

* Hover over the fraction of the item that remains in view.

* The flyout opens anchored to where the top of the item would be - ie, it is inside the toolbar area.
Attached patch flyout positioning patch (obsolete) — Splinter Review
Attachment #669331 - Flags: review?(mhammond)
Attachment #669331 - Flags: review?(jaws)
This patch introduces a slightly new behavior with the arrow panel, which is to locate the arrow on the y axis in a position where it points to the yoffset we want.  Flip had to be turned off in order to prevent lower level code moving our panel around on us, so we also have to constrain the location of the panel so it does not overflow the window.
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Created attachment 669331 [details] [diff] [review]
> flyout positioning patch

This patch is very complex and I think it would be quite scary to try to take directly to beta. I'd much rather see a patch built on top of the patch for bug 793157 (which also can't be uplifted due to changing the panel API).

I think the bug about getting panels to show on top of the toolbar is acceptable for the first version of this feature.
Depends on: 793157
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(In reply to Jared Wein [:jaws] from comment #6)

> I think the bug about getting panels to show on top of the toolbar is
> acceptable for the first version of this feature.

It is actually a little worse than I described above.  It is possible to have the anchor arrow a long way from the anchor itself, and also to have the panel a long way off the top of the app.  To repro:

* Move FF and resize the sidebar's internal horizontal splitter such that items that can generate flyouts is near the bottom of the screen.

* Hover over one of the items near the bottom of the screen.

The panel "flips" as it can't fit at the bottom - but the anchor arrow remains at the top of the panel - it should be at the bottom - so the arrow is pointing a long way from where it should be.

* Now move the mouse such that items above the current one show the flyout, and keep going until you reach the top-most item that can cause a flyout.

The panel moves upwards as you expect - but it remains "flipped" and with the anchor arrow at the top where it should be at the bottom.  When you get to the top-most item in the sidebar, the top if the panel is *much* higher than the top of the Firefox border and the anchor arrow is still at the top - pointing outside the Firefox borders.

That said though, I'm inclined to agree with the concern about the size of this patch going directly to beta.  The fact that none of us or our dog-fooders have noticed this means I think that having the bug stay in the beta builds might be OK.

Another solution that is slightly ugly would be to have this hovering close and reopen the panel instead of "fluidly" moving - this side-steps the entire issue.
(In reply to Jared Wein [:jaws] from comment #6)
> This patch is very complex and I think it would be quite scary to try to
> take directly to beta. I'd much rather see a patch built on top of the patch
> for bug 793157 (which also can't be uplifted due to changing the panel API).

I agree about the risk of this patch. I think we can probably uplift bug 793157's patch to beta if we need to, though.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8)
> I agree about the risk of this patch. I think we can probably uplift bug
> 793157's patch to beta if we need to, though.

FWIW and unfortunately, bug 793157's patch doesn't help solve this.
Comment on attachment 669331 [details] [diff] [review]
flyout positioning patch

Cancelling my review request now since I'm too busy with Firefox17-targeted social api work. We could take a patch like this on mozilla-central, but I wouldn't like to see us uplift it to fx17.

I think we should be ok shipping v1 of social api with this bug. The risk/reward of taking a patch that reimplements flyout positioning doesn't seem to be in our favor for such a short time frame.

Note that taking a patch like this on mozilla-central right now would also cause merge conflicts for any fx17-tracked changes too.

Please request review again when we have determined that fx17 social api work is complete.
Attachment #669331 - Flags: review?(jaws)
Whiteboard: [Fx17]
Removing dependency based on comment #9.
No longer depends on: 793157
Summary: flyout panel offset → flyout panel can appear anchored outside of the sidebar
It looks like if both bug 793157 and bug 798226 were fixed we would not need this patch.
(In reply to Mark Hammond (:markh) from comment #12)
> It looks like if both bug 793157 and bug 798226 were fixed we would not need
> this patch.

we would still have the flip happen which does not account for the location of the arrow, it results in the arrow at the wrong location.  The patch modifies the location of the arrow so it is always matching to the yoffset.

I dont see this landing for 17, maybe we can deal with something for 18.
I would very much like to see this land for 17 but understand if it can't for time/risk constraints. However, I would strongly vote for this landing no later than Firefox 18 for Beta. It's fairly trivial to trigger this bug and it's pretty evident when it does happen. Some of my flyouts even appear partially off-screen. The UX can be quite jarring.

Landing this for 18b1 gives us 6 weeks to back-out if a regression is found. I'm not keen on the idea of this being the state of Social for 8 more weeks (from today) if it doesn't make 18 Beta.
Attached patch flyout positioning patch (obsolete) — Splinter Review
updated patch.  this patch only affects the flyout panel in social.  Mark, if you could apply and test this on windows that would be great.
Attachment #669331 - Attachment is obsolete: true
Attachment #669331 - Flags: review?(mhammond)
Attachment #688342 - Flags: feedback?(mhammond)
Attachment #688342 - Flags: feedback?(gavin.sharp)
IMO bug 798226 does not handle our use case.  We have a very large anchor and may have tall panels that we want to fit 100% on the screen.  This means moving the arrow itself to point to the offset rather than depending on the the flip code to handle this correctly.  The problem becomes more apparent on small screens (e.g. my 11inch mac air).  

I'll attach an image to illustrate, but you can see the behavior I am talking about if you try this with the sidebar that is on facebooks website (hide the socialapi sidebar) and move the splitter to the bottom so you have ticker stories close to the bottom of the window.
Attached image largeflyout.png
Image shows a large flyout that, on a smaller screen would not fit under normal means (anchored at top or bottom of panel).  So the panel is positioned to fit in the screen, then the arrow itself is moved to position.
Attachment #688342 - Flags: feedback?(enndeakin)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> IMO bug 798226 does not handle our use case.

Bug 812943 is for this use-case, which sadly isn't moving anywhere.  I've asked Neil for additional feedback here.
Per discussion over email, this is a visual glitch that isn't catastrophic and would require non-trivial front-end workarounds, so we're not going to track this for 18. Let's get it fixed on trunk via bug 812943, though.
Comment on attachment 688342 [details] [diff] [review]
flyout positioning patch

Have we gotten a definitive answer about whether bug 812943 will save us from this?
Attachment #688342 - Flags: feedback?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> Comment on attachment 688342 [details] [diff] [review]
> flyout positioning patch
> 
> Have we gotten a definitive answer about whether bug 812943 will save us
> from this?

Doesn't seem so.  The code in this patch could probably be adapted to be a part of the panel itself.  I didn't originally go that route because, at the time, I didn't want to affect toolkit code, I choose to only affect the flyout for our sidebar.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> Have we gotten a definitive answer about whether bug 812943 will save us
> from this?

I believe the combination of bug 798226, bug 812943 and bug 822165 will certainly solve it.  

Bug 812943 is the only one without reasonable progress, but that one is unlikely to be hit once the others are fixed.

Those others will give us correct "flip" behaviour.  This will only leave a problem when the panel can not fit when flipped in either direction (which is precisely bug 812943).  In practice, this will mean there will only be a problem when the height of the panel is > 1/2 the height of the screen and the anchor location is near the vertical middle of the screen.  I've never seen a flyout be that size on my desktop, but I guess, eg, laptops with a small height could still hit it - and even then, IIUC the anchor arrow will be correct but the height of the panel will be reduced to accommodate it.

IOW, those other 2 bugs get us a *long* way towards having the problem fixed and bug 812943 will cover the additional and rare edge-cases.
(In reply to Mark Hammond (:markh) from comment #22)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #20)
> > Have we gotten a definitive answer about whether bug 812943 will save us
> > from this?

> IOW, those other 2 bugs get us a *long* way towards having the problem fixed
> and bug 812943 will cover the additional and rare edge-cases.

It's not so rare, once I apply Bug 805684 I have constant positioning issues.  Trying to fix those lead in the direction of the patch in this bug.  I'm happy to wait on the bugs I'll add as dependencies here, and then revisit bug 805684, but we need to get this done soon.
Blocks: 805684
Depends on: 812943, 798226, 824963
Attachment #688342 - Flags: feedback?(mhammond)
Attachment #688342 - Flags: feedback?(enndeakin)
This trivial patch enables "sliding" for the social panel.  It depends on bug 812943 which is currently on inbound.
Assignee: mixedpuppy → mhammond
Attachment #688342 - Attachment is obsolete: true
Attachment #748605 - Flags: review?(mixedpuppy)
Attachment #748605 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/mozilla-central/rev/02d3d8a70215
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.