viewer: don't repeatedly show overlapping annotations #521
Conversation
|
Good find. I wonder how much effort it would be to change the highlighter to coalesce spans. |
|
I've actually ended up looking at the span-creation stuff anyway (for performance when annotating a range of every cell in a 5000-row table – perhaps pathological but that's where I find myself) so maybe I'll take a look… |
|
🍰 thanks |
The knowledge of .data('annotation') seems a bit more reasonable than
the knowledge that the plugin calls .map() on its highlights property.
This seems to have been introduced when translating from coffeescript @mousedown to JavaScript.
1fc7024 changed this test from `which == 1` to `which > 1` with the explanation: > Also incorporating a change in the button check, from > > e.which != 1 > > to > > e.which > 1 > > which seems to be necessary for IE8 support (see openannotation#436). (I'm offline so can't check openannotation#436, but) this seems wrong: the only button we care about is the primary button, used to select text, and the commit and the message disagree anyway!
|
@BigBlueHat this is orthogonal to #523 unfortunately. I've an alternative patch which doesn't break other mouseover handlers in the document, plus some other fixes I noticed along the way – just a mo… |
wjt
force-pushed
the
many-overlapping-annotations
branch
from
d3bac5d to
f69bb50
Compare
|
@wjt bump |
|
Sorry, should have been explicit that I had pushed the “alternative patch”! Calling |
|
There shouldn't be any interior markup unless it's added outside annotator
after the highlights have been added because the highlighter wraps the text
nodes.
|
See GH issue openannotation#520. If you have a chunk of text with hundreds of annotations at the same spot, the markup looks like this: ```html <span class="annotation-hl"> <span class="annotator-hl"> ... Foo ... </span> </span> ``` Mousing over the text fires this event once per `<span>`; so `.load()` would be called once per `<span>`, each time with the full set of annotations. `.load()` is surprisingly expensive -- `Widget.checkOrientation` seems costly -- and so in my test case with 150 annotations on the same block of text, the browser would freeze for ~10 seconds. (I tried changing the highlighter to use a single `<span>` for all annotations covering a piece of text, but it's surprisingly fiddly to get right if you have annotations which intersect but are not identical.)
wjt
force-pushed
the
many-overlapping-annotations
branch
from
28583f6 to
b5211bf
Compare
|
Fair enough – I guess that if something is meddling with the contents of the highlighter's elements after they are created, there may be bigger problems. Here's a version which checks |
There was a problem hiding this comment.
I don't think this part is necessary since the event is delegated.
viewer: don't repeatedly show overlapping annotations
|
Thanks! Sorry that took me so long. |
|
No prob, thanks for merging! |
3 participants
See GH issue #520.
This is problematic because it will break mouseover events on the
highlight's parent, but it does make the page not freeze when you mouse
in and out of a chunk of text with hundreds of annotations. See
http://willthompson.co.uk/misc/annotator-freeze-on-mouseout/ for an
example without this patch.
One alternative would be to use a single span.annotation-hl with
multiple annotation-ids where currently they would be nested. ie,
transform this:
into this:
Another alternative would be to stick with the current structure but
make the .done() callback inside _onHighlightMouseover smarter about not
repeating itself.