Ionic-framework: Popover positioning needs improvement

[softhorizons]

Short description of the problem:

Presenting a Popover with an event specified for positioning leads to silly positioning in most cases. Currently, the Popover is shown above or below the event target, even if that leads to huge portions of the Popover being off-screen. This can lead to downright ridiculous small (down to 1-line) sections of the Popover showing at the bottom of the screen. And, if the event target is a button that happens to be near the middle or bottom of a long ion-list, designer doesn't even have control over that position. Landscape orientation is another situation that almost always leads to outrageous results.

And, we don't want to make users scroll unless absolutely necessary, just for the sake of strictly positioning the Popover above/below the event target.

What behavior are you expecting?

Currently, the code currently tries to position the Popover below the event. If it doesn't fit entirely below, it tries to fit it entirely above the event. Failing that, the Popover goes below the event. This is a toy algorithm, doesn't even begin to deal reasonably with a majority of situations.

Suggestion 1 (preferred): after above/below fail, try full height to the left & right of the event, then fallback to centered on entire screen (as if no event target were specified).
Suggestion 2: after above/below fail, center to screen as if no event target were specified.

Note: Related issue 6797 [popover is incorrectly placed the first time is opened] hinders working around this and probably interferes with implementing suggested fixes. The issue seems to be disregarded because you were unable to reproduce, but it's a very real problem. The key step missing to reproduce is that the issue occurs only if use a templateUrl -- not template. Suggest you revisit.

Steps to reproduce:

1. See code. Use non-trivial Popup. Try setting the event target at different screen positions, screen orientations.

  PresentPhotoEdit(ev) {
    let popover = this.popoverCtrl.create(CameraPopover);
    popover.present( {ev: ev} ); 
  }

Which Ionic Version? 2.x beta11

Run ionic info from terminal/cmd prompt: (paste output below)
Cordova CLI: 6.3.1
Gulp version: CLI version 3.9.1
Gulp local: Local version 3.9.1
Ionic Framework Version: 2.0.0-beta.11
Ionic CLI Version: 2.0.0-beta.37
Ionic App Lib Version: 2.0.0-beta.20
OS: Windows 7 SP1
Node Version: v4.4.5

[manucorporat]

@softhorizons hi! can you provide some screenshots?

[softhorizons]

See below.

The first one is properly centered on screen (I omitted event target).

The other 3 are relative to event target (the grayed out photo). All of these needlessly force user to scroll to see entire Popup contents.

Notice the one above is extreme, has just a white fragment of the Popup box + pointer triangle at bottom of screen.

[daveshirman]

Hi I know this doesn't solve the problem, but when i was facing this sort of issue, I used the ActionSheet instead. At least it is consistently posiitoned + rendered. Just trying to help.

[softhorizons]

Thanks for the kind suggestion, Dave. I need more formatting flexibility than ActionSheets provide, so I worked around this by using a Popover _without an event target_, and further switching from templateUrl to template to work around the bug which messes up first-time positioning. That gives me Popovers centered on the screen, which is good enough but not 100%-ideal since it'd be a better UX with popups next to the event target like they're supposed to be instead of floating off in space. (The top screenshot of the workaround happens to ideally positioned, but that's just luck. It doesn't work out so well on a bigger screen, or in portrait orientation.)

I've moved on, but this is a beta and I'm trying to be a good citizen. Trying to give back by reporting issues even after I mostly worked around them. In the case of a design issue like this, I believe it's especially important to report problems at an early stage while the product's intended behavior is still easily changed without breaking lots of user code.

[daveshirman]

No worries. Good luck.

On 22 Aug 2016 4:34 p.m., "softhorizons" [email protected] wrote:

Thanks for the kind suggestion, Dave. I need more formatting flexibility
than ActionSheets provide, so I worked around this by using a Popover _without
an event target_, and further switching from templateUrl to template to
work around the bug which messes up first-time positioning. That gives me
Popovers centered on the screen, which is good enough but not 100%-ideal
since it'd be a better UX with popups next to the event target like they're
supposed to be instead of floating off in space. (The top screenshot of the
workaround happens to ideally positioned, but that's just luck. It doesn't
work out so well on a bigger screen, or in portrait orientation.)

I've moved on, but this is a beta and I'm trying to be a good citizen.
Trying to give back by reporting issues even after I mostly worked around
them. In the case of a design issue like this, I believe it's especially
important to report problems at an early stage while the product's intended
behavior is still easily changed without breaking lots of user code.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/driftyco/ionic/issues/7803#issuecomment-241452048,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANnlXuSWuY5Xl4VM8WLnjzDTEBt97Gkuks5qicFrgaJpZM4JpIms
.

[jgw96]

Hello all! If you switch the popover to use inline templates does it solve the issue for you? Thanks!

[softhorizons]

Yes and no. All the above screenshots were taken of a setup where I used inline templates.

1 - No: inline templates do _not_ fix the positioning issue if an event target _is_ supplied (screenshots 2 - 4). Looking at the Popover source code, inlining couldn't possibly fix this.
2 - Yes: inline templates _do_ fix positioning if _no_ event target is supplied (screenshot 1). Mind you, some of us have workflow issues with inline templates, so not an ideal workaround.

[brandyscarney]

So we haven't disregarded issue #6797. We are actively working on integrating AoT compiling which will fix the need for inline templates as a workaround and in turn fix that issue.

In response to your issue, this is the first time it has been brought to our attention so thank you for the details to reproduce. This use case was missed when initially adding the component so I'll make sure to add it to our tests. Please let us know if you have any further feedback. Thanks!

[zakton5]

Related to popover positioning, it might be a nice idea to add another popover option for preferred position. For example, if I know that I'm going to want my popover to appear above the $event element, then i could specify position: 'top' in the popover options.

[gianpaj]

FYI: for others overlooking the docs.

I had this very simple Popover, but it's wasn't opening on the top right where the click event happened (like in the doc example).

@Component({
  template: `
    <ion-list>
      <button ion-item (click)="user.logout()">Logout</button>
      <button ion-item (click)="openPage()">Delete Account</button>
      <button ion-item (click)="close()">Cancel</button>
    </ion-list>
  `
})
export class PopoverMenu {
  constructor(public viewCtrl: ViewController,
              private nav: NavController,
              public user: UserService) {}

  close() {
    this.viewCtrl.dismiss();
  }

  openPage() {
    this.nav.push(DeleteAccountPage);
  }
}

@Component({
  selector: 'my-profile-page',
  templateUrl: 'index.html'
})
export class MyProfilePage implements OnInit {
  user: any;

  constructor(private userService: UserService,
              private popoverCtrl: PopoverController,
              private nav: NavController) {
  }

  ngOnInit() {
    this.user = this.userService.getUser();
    console.log(this.user);
  }

  openOptions(ev) { // <-- the fix
  openOptions() { // <-- the bug
    let popover = this.popoverCtrl.create(PopoverMenu);
    popover.present({ ev: ev }); // <-- the fix
    popover.present(); // <-- the bug
  }
}

<ion-header>
  <ion-navbar>
    <button ion-button menuToggle>
      <ion-icon name="menu"></ion-icon>
    </button>
    <ion-buttons end>
      <!-- --- BELOW IS THE FIX -->
      <button ion-button icon-only (click)="openOptions($event)">
      <!-- --- BELOW IS WRONG -->
      <button ion-button icon-only (click)="openOptions()">
        <ion-icon name="more"></ion-icon>
      </button>
    </ion-buttons>
  </ion-navbar>
</ion-header>

Now 🎉 :

[jgw96]

Hello everyone! As I cannot reproduce this issue anymore with the latest release of Ionic I am going to close this issue for now. Thanks!

[gianpaj]

I can still reproduce this with:

$ ionic info

Your system information:

Cordova CLI: 7.0.0
Ionic CLI Version: 2.2.2
Ionic App Lib Version: 2.2.1
ios-deploy version: 1.9.1
ios-sim version: 5.0.8
OS: macOS Sierra
Node Version: v6.10.2
Xcode version: Xcode 8.3.1 Build version 8E1000a

[cleitonjar]

Any news?

[cleitonjar]

Any news?

[MartinMuzatko]

There is a problem with nested loops. Then, the popover does not manage to find the origin anymore and gets shown at the upper left corner.

[MartinMuzatko]

After investing countless hours, I found that the renderer re-renders the looped data before inserting the popover. Leading to the originating event.target to not exist anymore when calculating the bounding box.

[ionitron-bot[bot]]

This issue has been automatically identified as an Ionic 3 issue. We recently moved Ionic 3 to its own repository. I am moving this issue to the repository for Ionic 3. Please track this issue over there.

If I've made a mistake, and if this issue is still relevant to Ionic 4, please let the Ionic Framework team know!

Thank you for using Ionic!

[ionitron-bot[bot]]

Issue moved to: https://github.com/ionic-team/ionic-v3/issues/126