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

Tribute menu wrong position when body is scrolled on contenteditable elements #10

Closed
dmail opened this issue Jun 24, 2016 · 11 comments
Closed
Labels

Comments

@dmail
Copy link
Contributor

dmail commented Jun 24, 2016

When the window is scrolled it's correctly taken into account by getTextAreaOrInputUnderlinePosition. However its contenteditable sibling getContentEditableCaretPosition does not consider window scroll

A quick fix I used was to add this block of at the bottom of getContentEditableCaretPosition

var doc = document.documentElement
var windowLeft = (window.pageXOffset || doc.scrollLeft) - (doc.clientLeft || 0)
var windowTop = (window.pageYOffset || doc.scrollTop)  - (doc.clientTop || 0)

coordinates.top += windowTop
coordinates.left += windowLeft

A better fix would take into account the scroll of every parent having a scroll, not only the window. (For instance if there is an overflow: auto on an ancestor element)

@mrsweaters
Copy link
Collaborator

@dmail I can see where this would be problematic. What we need to do is create a scenario that reproduces this. Can you add an example of the issue to the example/index.html file?

@dmail
Copy link
Contributor Author

dmail commented Jun 27, 2016

After testing it appears that taking into account the scroll of ancestor elements is required when tributeMenu & inputElement have different scrollable ancestor.

This is not the case in example/index.html because the menuContainer options is set document.getElementById('content') to append the tributeMenu in the same scrollable ancestor.

In conclusion

  • The suggested fix is enough used in combination with the menuContainer options.
  • If tribute was calculating all ancestors scroll the need for menuContainer would vanish but may have side effects whoose I'm unaware of.

@mrsweaters
Copy link
Collaborator

@dmail Thank you! You brought up a good point, it seems like menuContainer and your suggested fix is the best way to go here. I'll look over your PR and see if there is any other possible approach we can take.

@dmail
Copy link
Contributor Author

dmail commented Jun 27, 2016

Yeah the approch with buttons and javascript to force the scroll have strong chances to break as the index.html file changes but was the faster/cleaner way to illustrate my point at the moment.

Btw you are welcome I like what you did and your enclination for vanilla js so thank you too ;)

@mrsweaters
Copy link
Collaborator

@dmail From what I can tell, it looks like the current code positions correctly in your pull request, but when I scroll then I lose my position until I start typing again, at which point it repositions. Maybe we need to bind to the onscroll event for both the window and the target element and when those change, reposition the menu?

@mrsweaters mrsweaters added the bug label Jun 30, 2016
@dmail
Copy link
Contributor Author

dmail commented Jul 1, 2016

Do we agree that there is two issue ?

Issue 1

step to reproduce

  • I'm in the contenteditable element
  • Menu is not opened
  • body is scrolled
  • I type in the field

Bug description

Menu is badly positionned

Suggested fix

Can be fixed as suggested in the first comment

Issue 2

step to reproduce

  • I'm in contenteditable element or traditional form element
  • Menu is opened
  • I scroll the body or the container

Bug description

menu does not stick to the input and becomes badly positionned, menu is correctly repositionned as soon as you type in the field.

Suggested fix

Can be fixed as you suggest, by listening for the scroll event of the body (or menuContainer when options is set) and update menu position for each scroll event.

@joaolavoier
Copy link

joaolavoier commented Jul 20, 2016

When this issue will be fixed? Can i help to solve this issue?

@dmail
Copy link
Contributor Author

dmail commented Jul 20, 2016

I don't have much time atm but I can tell you without testing that adding the code below

(this.menuContainer || window).addEventListener('scroll', function() {
    if (this.tribute.isActive) {
        this.tribute.showMenuFor(this.tribute.current.element)
    }
}.bind(this), false)

after line#256 should fix the scroll issue.

@mrsweaters do you think you can add these two fix soon ?

@mrsweaters
Copy link
Collaborator

@dmail @joaolavoier Sorry guys, been crazy busy the past few days. I should have some time to test this out today.

@mrsweaters
Copy link
Collaborator

@dmail @joaolavoier Please test this pull request: #18

@tcstory
Copy link

tcstory commented Apr 1, 2019

I had meet the similar problem, in the source code it use window.pageYOffset to calculate the value of windowTop, but it's wrong when you set menuContainer to another div insteaf of the body

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

No branches or pull requests

4 participants