Method of Mobile Popup placeTo works

[goldim]

Fixed #9423 . Please check it carefully with attention. Some of code difficult to understand. For example I don't get next code (what reason):

// Move outside of viewport
this.placeTo(-1000, -1000);

I haven't noticed any bad side effects removing it. It generated extra updatePosition. In docs said there is 3rd case of using popup - it is attaching to certain an absolute point but forgotten to process this case during updatePosition and it is was just centered with first case of using (see docs about class Popup). And another thing: I remember that replaceTo was called and don't reset it. Should it be reset in some case?

[derrell]

What problem were you trying to fix with this PR? Was there some problem caused by moving the popup offscreen by default with

// Move outside of viewport
this.placeTo(-1000, -1000);

? It's been working, I believe, for a very long time...

[goldim]

@derrell this issue - https://github.com/qooxdoo/qooxdoo/issues/9423

[derrell]

Yeah, ok. If there's no place that exits the event loop during execution of the popup code, then maybe reset it with:

setTimeout(() => this.__placed = false, 0);

[goldim]

@derrell I declared the member in the beginning of a member section as you asked. Also renamed it and added in destructor of class. I don't get why I have to reset it via time. I try to explain there are 3 cases of positioning:

1. constructor - you just don't pass the second argument - an anchor
2. method setAnchor btw this method calculates position of widget-anchor and calls placeTo
3. directly via placeTo

There is no method resetAnchor and only way to get rid of anchor - to pass undefined/null value in setAnchor but this method will save the position. I thought maybe it would be right to position to center bc the anchor is removed.

[derrell]

There's no BC issue, and if one really wanted to reset it, it looks like placing it offscreen, e.g. with popup.placeTo(-1000, -1000), would do it. I suppose you could reset __isPlaced near line 211:

        if (isOutsideViewPort) {
          this.__isPlaced = false;
          this._positionToCenter();
        } else {

although I'm not sure it's really necessary.

[goldim]

@derrell I will try it and check it out.

[goldim]

@derrell I can not reproduce offscreen case neither in original code not mine. Also about setting margins to zero px or null: There is case when firstly popup is moved to center and margins are left and if I want to placeTo then the popup will be shifted - It is unexpected behaviour. I think resetting them is good choice. What do you think? Btw I found more dangerous bug (infinite recursive calls) debugging code of Popup but about it I will make another PR.

[goldim]

@derrell I think the method qx.util.placement.Placement and its options keeps inside of viewport and situation where it is outside is imposible. About margins: it is possible that someone can set margins for popup via theming for example? In this case resetting margin before placeTo is bad choice.

[derrell]

@goldim I don't think resetting things is worth the trouble. Popups are not issued in a fast loop; rather, they are presented only occasionally. I think if someone wanted to show a previously shown popup and restore its centered behavior, they could simply regenerate a new popup, discarding the old one. I don't want to discourage you from searching for a way to reset the position, even adding a new method to do it, but it does not appear to be necessary for BC, nor IMO, is it necessary for usability.

[goldim]

@cboulanger @level420 Could you approve or reject this PR please?