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

Error in positioning of annotations #243

Closed
richajain opened this issue Jul 21, 2013 · 3 comments
Closed

Error in positioning of annotations #243

richajain opened this issue Jul 21, 2013 · 3 comments

Comments

@richajain
Copy link

The annotations are not placed over the quotes hovered, rather they are being placed at some posiiton below the actual text. This may be caused due to the change d45a366 . I think offset() is relative to the document and position() is relative to offset. So using position() seems wrong here.
http://api.jquery.com/offset/
http://api.jquery.com/position/

@tilgovi
Copy link
Member

tilgovi commented Jul 22, 2013

It's not as simple as offset() vs position(). The former is correct under the assumption that the wrapper is the offset parent of the annotation. The latter is correct under the assumption that the offset parent of the annotation is positioned at (0, 0).

In many uses of Annotator, both of these are true. When the body, or some other parent of the wrapper div, is not at (0, 0), the current version breaks. The old version worked. This is a proper regression.

However, in hypothesis/h, we don't use the wrapper div because I find it to be nasty and unnecessary to manipulate the DOM that way. That was what motivated the change.

Here's a plunk which demonstrates: http://plnkr.co/edit/FVmBmchrO3zmcYTTDtTJ

The proper solution is to check the offset parent. If the offset parent of the target is the wrapper, then the .offset() of that should be taken. Otherwise, the .offset() of the offset parent of the wrapper should be taken as the origin point.

@tilgovi
Copy link
Member

tilgovi commented Jul 22, 2013

There's an additional issue you can see in the plunk, which I have not finished finding a solution for, which is when the target is itself the wrapper. You can see it in the plunk by clicking somewhere around the edges of the list such that the entire wrapper is highlighted in red as the target.

@tilgovi
Copy link
Member

tilgovi commented Jul 22, 2013

Plunk updated to reflect possible solution. Pull request forthcoming.

gnuhub pushed a commit to gnuhub/mediawiki_extensions that referenced this issue Apr 19, 2014
Project: mediawiki/extensions/Annotator  cbc0ac1f406b276690a28c64c4752e2098b8b6b6

Added the destroy class

The id parameter is being passed and annotations are deleted and return
a 204 http status code. The annotator-full.js has been rebuilt from upstream
after the bug openannotation/annotator#243 is fixed and now there
is no positioning error.

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

No branches or pull requests

2 participants