Anchor comments directly to the scroll position and optimise comment layout

[Cwiiis]

• Resolves: CommentSection layout is very expensive while scrolling #10748
• Target version: master

Summary

It is desirable for comments to scroll in sync with the document instead of lagging behind the scroll position. The first commit in this branch anchors comments directly to the scroll position and the second commit fixes various performance issues doing this exposes.

Checklist

• I have run make prettier-write and formatted the code.
• All commits have Change-Id
• I have run tests with make check
• I have issued make run and manually verified that everything looks okay
• Documentation (manuals or wiki) has been updated or is not required

[Cwiiis]

Some failures here, likely due to the show/hide caching - I was wondering if that would hold up :) Will dig into it.

[Cwiiis]

I'm not fully convinced these failures are directly caused by this patch - they seem to be happening because sometimes after this patch when you hit save, you end up with a conflict dialog asking if you want to overwrite changes that another user made - possibly there's some bad ordering of events and we're getting the acknowledgement back from the server before we've set a property that indicates we were the ones to have made the change... Still digging. It doesn't seem to be a cache invalidation issue at least(!)

[Cwiiis]

Failures should be fixed, it was caching of show/hide as I initially thought - didn't quite get to the root of it, but made sure it behaved identically to how it did before and the failures disappeared locally.

[Cwiiis]

Ok, so still some of the same failures, I'll dig further to see if I can replicate them locally... Though the last run I had was successful, so not entirely sure what's happening now - intermittent maybe?

[Cwiiis]

ah, just noticed it's specifically the calc annotation_spec.js that's failing - I didn't try that locally so it's probably legit again. There's specific behaviour for annotations in calc, looking at this now.

[Cwiiis]

So I think the broken tests, it's the test themselves that are bad - they're modifying the DOM and so obviously breaking caching. I think these tests need to be rewritten to not do this, of course randomly modifying the DOM is going to break assumptions within the app... I'll try to figure out what they're trying to do and fix the tests.

[Cwiiis]

Should be good now. Rather than trying to fix all those tests and calc behaviour, I just made the choice not to cache show/hide for calc - Only one comment can be visible and they're hidden during scrolling, so it doesn't really buy us anything vs the quite large amount of work to fix this properly.

[gokaysatir]

Thanks new update mechanism feels much better:)
There are some things, not sure if they are related to your PR:

• When i paste a long content into comment (while creating a new comment) and click on save, it doesn't save the comment and i need to click twice. This sometimes adds 2 instances of the same comment.
• In Impress, long content of a comment has a gap above it.

If they are not related to your code, we can get this in, in RED code i think.

Other than that, i checked the code and behaviour quickly, thank you.

[gokaysatir]

I think "hidden" cannot be true if we are here. I guess we could initiate hidden as "false" and remove these assignments. Not that important but may be confusing in the future while editing the code.

[Cwiiis]

Argh whoops, it's not this that's wrong (I don't think), but the check at the top of the function - makes me wonder how all the tests passed - I guess because they don't make multiple changes and at first the value isn't cached (intentionally, to mimic the old behaviour of the comment being in an indeterminate state on creation).

[gokaysatir]

Now i checked master branch. Both issues seem to be related to this PR. Can you have a look at them when you have time?

[Cwiiis]

Now i checked master branch. Both issues seem to be related to this PR. Can you have a look at them when you have time?

Absolutely, thanks for the thorough testing 🙂

[Cwiiis]

@gokaysatir With the mistake in CommentSection.show fixed, I can't reproduce any of the problems you list - would you mind giving it another quick check and if you still have issues, let me know very detailed steps to reproduce? Thanks :)

[gokaysatir]

We have 2 things here:

• I can't update a comment because of the "someone else is updating" popup.
• There are JS errors on the console. Not sure if they were there before. Though seems unrelated to your commits, i couldn't be sure.

Can you check if you can reproduce the issues? I tried to switch to master and back to your PR to be sure. If you can't reproduce, then i'll need to refresh browser cache and dist directory.

Gap in Impress comments seems to persist, but i couldn't check correctly because of the popup.

[Cwiiis]

Hopefully I've fixed these issues - the problem was the interaction with editing and show/hide caching. In the end it made sense to also fix isEdit being slow by caching that value. Annotation tests now pass locally.

I think long-term, we need to have a bit of an audit of the codebase - there's too much direct DOM modification and reading peppered through the code and we'd likely fix a lot of performance issues by consolidating all DOM access into a few context-specific utility functions that could also cache the last state that was set. For example, in the CommentSection case specifically, nodeModify and nodeReply are frequently accessed and modified directly, but we could consolidate these into just a couple of functions, e.g. setModifyVisible, setReplyVisible and corresponding properties to access that state. This would improve performance and make the code more concise and readable.

[Cwiiis]

Seems I can't win with this patch! I can reproduce the invalidations failure locally, investigating now...

Edit: Actually, this is an unrelated intermittent failure.

[gokaysatir]

Thanks for the improvements :) I reviewed this before. And now issues are gone. I think we can get this in.