Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong positioning of popover after scrolling #299

Closed
lostip opened this issue Aug 29, 2014 · 15 comments
Closed

Wrong positioning of popover after scrolling #299

lostip opened this issue Aug 29, 2014 · 15 comments
Labels
Milestone

Comments

@lostip
Copy link
Contributor

lostip commented Aug 29, 2014

When calling editorScope.showPopover(element), scroll position is not considered for positioning the popover.
Please see attached screenshots to see an example where no scrolling has occurred, and one where it has (this is a custom tool, but I'm using the provided popover).
Also, here's a diff with a possible fix, where the scrollTop is subtracted from the calculated top position in the reflowPopover function:

            scope.reflowPopover = function(_el){
                if(scope.displayElements.text[0].offsetHeight - 51 > _el[0].offsetTop){
-                   scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight + 'px');
+                   scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight - scope.displayElements.scrollWindow[0].scrollTop + 'px');
                    scope.displayElements.popover.removeClass('top').addClass('bottom');
                }else{
-                   scope.displayElements.popover.css('top', _el[0].offsetTop - 54 + 'px');
+                   scope.displayElements.popover.css('top', _el[0].offsetTop - 54 - scope.displayElements.scrollWindow[0].scrollTop + 'px');
                    scope.displayElements.popover.removeClass('bottom').addClass('top');
                }
                var _maxLeft = scope.displayElements.text[0].offsetWidth - scope.displayElements.popover[0].offsetWidth;

no-scroll
with-scroll

@SimeonC SimeonC added the bug label Aug 31, 2014
@SimeonC
Copy link
Collaborator

SimeonC commented Aug 31, 2014

Thanks, I'll look into it when I can.

@geyang
Copy link
Collaborator

geyang commented Aug 31, 2014

I have encountered this problem before, what it turned out to be in my case is that my overflow-y is added to the editor element.

The proper way to add overflow-y is to style the ta-scroll-window element instead. Because the popover element is a child of the ta-scroll-window, the popover position is maintained w.r.t. the text.

Hope this helps.
screenshot 2014-08-31 18 18 35

@SimeonC
Copy link
Collaborator

SimeonC commented Sep 16, 2014

@lostip Can you confirm that the solution posted by @episodeyang fixes your problem or not please?

@SimeonC
Copy link
Collaborator

SimeonC commented Oct 23, 2014

Closing due to inactivity.

@SimeonC SimeonC closed this as completed Oct 23, 2014
@lostip
Copy link
Contributor Author

lostip commented Nov 13, 2014

Apologies for the late response.
@episodeyang's fix is not what I need, because adding the overflow to the ta-scroll-window means the popover won't 'overflow' the editor, which means I'm stuck with a really small popover to make it fit inside the editor.
So, @SimeonC, I guess if you plan to support that the included popover can really 'pop' over anything, then this needs to be fixed.
If not, it needs to be documented.

@SimeonC
Copy link
Collaborator

SimeonC commented Nov 13, 2014

Ok I think I see now. I might be able to fix this better than just CSS by altering where the popover HTML is placed.

I haven't really used the editor in a short format so this isn't a point I've considered before.

EDIT: I'm gonna schedule this for the next version as 1.3.0 is too overdue as it is.

@SimeonC SimeonC reopened this Nov 13, 2014
@SimeonC SimeonC added this to the 1.3.1 milestone Nov 13, 2014
@SimeonC SimeonC modified the milestones: 1.3.1, 1.3.X Jan 26, 2015
@SimeonC
Copy link
Collaborator

SimeonC commented Mar 1, 2015

@lostip It's taken me a while to get back around to this - adding in the fix you mentioned earlier seems to work OK, but I don't have any CSS to test this with. I can commit based off your diff if you can confirm that that would fix your issue, but I would like to be able to test it myself.

@lostip
Copy link
Contributor Author

lostip commented Mar 3, 2015

@SimeonC let me put together a plunkr or something like that so you can test this better.
I'll try to have tomorrow or the day after.
Thanks for looking into this!

@SimeonC
Copy link
Collaborator

SimeonC commented Mar 13, 2015

This should be patched in 1.3.8, I think. So if when you get around to testing this do it on that tag please.

@pbassut
Copy link
Contributor

pbassut commented Aug 4, 2015

@lostip Could you setup a plnkr so we can update this issue?

Thanks a lot.

@iwontcode
Copy link

Issue reproduced at Plnkr
This is how the reflowPopover function looks like:

scope.reflowPopover = function(_el) {
    if (_el[0].closest) {
        var scrollLength = _el[0].closest('.ta-bind').scrollTop;
    } else {
        var scrollLength = $(_el[0]).closest('.ta-bind')[0].scrollTop;
    }

    /* istanbul ignore if: catches only if near bottom of editor */
    if (scope.displayElements.text[0].offsetHeight - 51 > _el[0].offsetTop) {
        //scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight + scope.displayElements.scrollWindow[0].scrollTop - scrollLength + 'px'); 
        scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight + scope.displayElements.scrollWindow[0].scrollTop - scope.displayElements.scrollWindow[0].scrollTop + 'px');
        scope.displayElements.popover.removeClass('top').addClass('bottom');
    } else {
        //scope.displayElements.popover.css('top', _el[0].offsetTop - 54 + scope.displayElements.scrollWindow[0].scrollTop - scrollLength + 'px');
        scope.displayElements.popover.css('top', _el[0].offsetTop - 54 + scope.displayElements.scrollWindow[0].scrollTop - scope.displayElements.scrollWindow[0].scrollTop + 'px');
        scope.displayElements.popover.removeClass('bottom').addClass('top');
    }
    var _maxLeft = scope.displayElements.text[0].offsetWidth - scope.displayElements.popover[0].offsetWidth;
    var _targetLeft = _el[0].offsetLeft + (_el[0].offsetWidth / 2.0) - (scope.displayElements.popover[0].offsetWidth / 2.0);
    scope.displayElements.popover.css('left', Math.max(0, Math.min(_maxLeft, _targetLeft)) + 'px');
    scope.displayElements.popoverArrow.css('margin-left', (Math.min(_targetLeft, (Math.max(0, _targetLeft - _maxLeft))) - 11) + 'px');
};

So basically I had to negate the addition of scope.displayElements.scrollWindow[0].scrollTop with itself (which amounts to zero px). Which means that the code before @lostip code was committed was just fine.

I maybe wrong about it, but currently that is what is working for me.

JoelParke added a commit to JoelParke/textAngular that referenced this issue Oct 2, 2016
…e positioning of the popover on scrolling
JoelParke added a commit that referenced this issue Oct 2, 2016
fix(main): Corrected and improved the issue #299 around the positioni…
@JoelParke
Copy link
Collaborator

Corrected this with PR #1349. Closing - please test...

@jaimeschettini
Copy link

Hello, I'm having the same problem here and seems that PR #1349 hasn't solved it. I also tested in this plucker which I forked from the one created by @hiretheheir and just changed the version of textAngular to 1.5.12.

@LeonadoRivaldo
Copy link

hi, i have the same problem and i find a math problem inside the reflowPopover function, só i ll leave here the code that work for me as a suggestion to fix the problem.

if(spaceAboveImage < 51) {
	//original line
	//scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight + scope.displayElements.scrollWindow[0].scrollTop + 'px');
	//correction
	scope.displayElements.popover.css('top', _el[0].offsetTop + _el[0].offsetHeight + 'px');
	scope.displayElements.popover.removeClass('top').addClass('bottom');
} else {
	//origirnal line
	//scope.displayElements.popover.css('top', _el[0].offsetTop - 54 + scope.displayElements.scrollWindow[0].scrollTop + 'px');
	//correction
	scope.displayElements.popover.css('top', _el[0].offsetTop - 54 + 'px');
	scope.displayElements.popover.removeClass('bottom').addClass('top');
}

@pranav-pkasthana
Copy link

for me setting the below option inside div worked
popover-append-to-body="false"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants