Implements simple scroll progress tracking for safe html5 renderer#13983
Conversation
github-actions
Bot
added
DEV: renderers
Build Artifacts
|
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Some thoughts on cleanup - I must admit, the duration based progress feels like something we can do further cleanup, outside of the scope of this PR too - I'll think on what that might look like!
There was a problem hiding this comment.
Could probably conditionalize this on this.forceDurationBasedProgress - if it's not set, we can just call recordProgress at the end of handleScroll?
|
I am guessing the merge conflict here is just because of |
akolson
force-pushed
the
simple-scoll-progress
branch
from
bdec078 to
68654b4
Compare
|
@rtibbles I think we should be good with this? |
| }, | ||
| }, | ||
| async created() { | ||
| async mounted() { |
There was a problem hiding this comment.
This is now deferring everything until mounted, when the only thing we need to defer until mounting is the tab indexes and the scroll listeners?
Thinking about it more, we're actually giving ourselves a potential race condition here by just waiting for next tick, because the point at which the ref will exist is not dependent on mounting, but rather when loading has been set to false.
I would revert this change from created to mounted, and move all of the things that are waiting on the next tick to another method:
async safeHtmlDomReadyHandler() {
if (!this.loading) {
await this.$nextTick();
this.applyTabIndexes();
window.addEventListener('resize', this.applyTabIndexes);
this.setupScrollListener();
}
},
then in the mounted hook, you would do this:
mounted() {
this.safeHtmlDomReadyHandler();
this.$watch('loading',this.safeHtmlDomReadyHandler);
},
This gives us the safety of the next tick, to make sure the effect of loading becoming false is propagated to the DOM, and if, for whatever reason it reloads, the watch statement will reapply these things for us.
There was a problem hiding this comment.
Yes. I did encounter race conditions due to the above while testing with the created and mounted hooks. This seems reasonable direction. Thanks!
|
I made one more comment about the timing of manipulating/accessing the DOM (some of which was extant from the prior implementation) which would be good to get resolved. It also looks like you have two stray commits from @AllanOXDi and one from @dependabot in this PR? Seems like the rebase has gone slightly awry. |
Yes it appears the release and develop were out of sync! Any ideas on how this can be resolved @rtibbles? Besides that, I think your comment has been successfully resolved. |
|
I think doing |
|
Aside from the extraneous commits, code wise looks good to me - although I note there are failing tests - maybe need to be updated after the tweaks to the loading pattern? |
|
yeah noticed this too! on it--thanks |
akolson
force-pushed
the
simple-scoll-progress
branch
2 times, most recently
from
c75ab46 to
1d8aa10
Compare
|
That should resolve it for the misplaced commits I think, @rtibbles? |
|
I am still seeing some commits that have @AllanOXDi and @marcellamaki's face on? But now they've been coauthored by you as well? Should just be able to drop these commits entirely, because they're duplicates of ones that are already in the branch. |
akolson
force-pushed
the
simple-scoll-progress
branch
from
1d33724 to
548d8f0
Compare
github-actions
Bot
added
DEV: dev-ops
akolson
force-pushed
the
simple-scoll-progress
branch
from
548d8f0 to
22d0a49
Compare
|
@rtibbles dropped the commits, should be fine I think now! |
|
A finely crafted commit history, I love to see it! |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Code wise, this is looking excellent now - I just need to do a manual test locally before we hand over to QA for final checks.
|
Local QA checks out - does exactly what it says on the tin. A more thorough check by the QA team definitely appreciated though! |
|
Progress tracking through scrolling the page seems to be working correctly in all supported browsers in Windows and Ubuntu, and on the physical Android device. I've updated the QA channel to include one long article to help with testing. |
🚀3 participants
Summary
This PR implements scroll-based progress tracking for the SafeHTML5 viewer to more accurately measure user engagement with content. It also updates the scroll-completion threshold to account for real-world browser behavior, where scrolling often stops just short of the theoretical maximum (1 in our case)
References
Closes #13828
Reviewer guidance
kolibri plugin enable kolibri.plugings.safe_html5_viewer