-
-
Notifications
You must be signed in to change notification settings - Fork 956
Aggregate embedded viewer progress in the SafeHtml article viewer #15037
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,14 @@ | |
| role="region" | ||
| :aria-label="$tr('articleContent')" | ||
| > | ||
| <SafeHTML :html="html" /> | ||
| <SafeHTML | ||
| :html="html" | ||
| @startTracking="handleViewerStartTracking" | ||
| @stopTracking="handleViewerStopTracking" | ||
| @updateProgress="handleViewerUpdateProgress" | ||
| @addProgress="handleViewerAddProgress" | ||
| @finished="handleViewerFinished" | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
|
|
@@ -52,12 +59,41 @@ | |
| html: null, | ||
| scrollBasedProgress: 0, | ||
| debouncedHandleScroll: null, | ||
| // Track embedded viewers for progress aggregation | ||
| // Using object instead of Map for Vue 2.7 reactivity | ||
| // Structure: { viewerId: { progress: number } } | ||
| embeddedViewers: {}, | ||
| // Guard to prevent emitting 'finished' multiple times | ||
| hasEmittedFinished: false, | ||
| }; | ||
| }, | ||
| computed: { | ||
| entry() { | ||
| return (this.options && this.options.entry) || 'index.html'; | ||
| }, | ||
| // Count of registered embedded viewers | ||
| viewerCount() { | ||
| return Object.keys(this.embeddedViewers).length; | ||
| }, | ||
| // Aggregated progress using dynamic weighting | ||
| // If no embedded viewers: progress = scrollBasedProgress | ||
| // If viewers exist: progress = (scrollBasedProgress + avgViewerProgress) / 2 | ||
| aggregatedProgress() { | ||
| if (this.viewerCount === 0) { | ||
| return this.scrollBasedProgress; | ||
| } | ||
|
|
||
| let totalViewerProgress = 0; | ||
| for (const viewer of Object.values(this.embeddedViewers)) { | ||
| totalViewerProgress += viewer.progress; | ||
| } | ||
| const avgViewerProgress = totalViewerProgress / this.viewerCount; | ||
|
|
||
| // Dynamic weighting: 50% scroll, 50% viewers | ||
| // A live average, so a viewer registering mid-session lowers it; the backend | ||
| // keeps the maximum, so learner-facing progress never regresses. | ||
| return (this.scrollBasedProgress + avgViewerProgress) / 2; | ||
| }, | ||
| }, | ||
| async created() { | ||
| const storageUrl = this.defaultFile.storage_url; | ||
|
|
@@ -100,18 +136,66 @@ | |
| } | ||
| }); | ||
| }, | ||
|
|
||
| // Handle startTracking from embedded viewers | ||
| handleViewerStartTracking(viewerId) { | ||
| if (viewerId && !this.embeddedViewers[viewerId]) { | ||
| this.$set(this.embeddedViewers, viewerId, { progress: 0 }); | ||
| } | ||
| }, | ||
|
|
||
| // Handle stopTracking from embedded viewers | ||
| handleViewerStopTracking(viewerId) { | ||
| if (viewerId) { | ||
| this.$delete(this.embeddedViewers, viewerId); | ||
| } | ||
| }, | ||
|
|
||
| // Handle updateProgress from embedded viewers | ||
| handleViewerUpdateProgress(progress, viewerId) { | ||
| if (viewerId && this.embeddedViewers[viewerId]) { | ||
| this.$set(this.embeddedViewers, viewerId, { | ||
| ...this.embeddedViewers[viewerId], | ||
| progress: Math.min(1, Math.max(0, progress)), | ||
| }); | ||
| } | ||
| }, | ||
|
|
||
| // Handle addProgress from embedded viewers | ||
| handleViewerAddProgress(delta, viewerId) { | ||
| if (viewerId) { | ||
| const viewer = this.embeddedViewers[viewerId]; | ||
| if (viewer) { | ||
| const newProgress = Math.min(1, Math.max(0, viewer.progress + delta)); | ||
| this.$set(this.embeddedViewers, viewerId, { | ||
| ...viewer, | ||
| progress: newProgress, | ||
| }); | ||
| } | ||
| } | ||
| }, | ||
|
|
||
| // Handle finished from embedded viewers | ||
| handleViewerFinished(viewerId) { | ||
| if (viewerId && this.embeddedViewers[viewerId]) { | ||
| this.$set(this.embeddedViewers, viewerId, { progress: 1 }); | ||
| } | ||
| }, | ||
|
|
||
| recordProgress() { | ||
| let progress; | ||
| if (this.forceDurationBasedProgress) { | ||
| progress = this.durationBasedProgress; | ||
| } else { | ||
| // Use scroll events to track progress | ||
| progress = this.scrollBasedProgress; | ||
| // Use aggregated progress from scroll + embedded viewers | ||
| progress = this.aggregatedProgress; | ||
| } | ||
| this.$emit('updateProgress', progress); | ||
|
|
||
| if (progress >= 1) { | ||
| // Scroll-to-bottom completes the article; embedded viewers | ||
| // registered at that point must also be at 1. | ||
| if (progress >= 1 && !this.hasEmittedFinished) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Resolved — addressed in the current code. suggestion: |
||
| this.$emit('finished'); | ||
| this.hasEmittedFinished = true; | ||
| } | ||
| this.pollProgress(); | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| import { render, screen } from '@testing-library/vue'; | ||
| import { createTranslator } from 'kolibri/utils/i18n'; | ||
| // eslint-disable-next-line import-x/named | ||
| import useContentViewer, { useContentViewerMock } from 'kolibri/composables/useContentViewer'; | ||
| import SafeHtml5RendererIndex from '../SafeHtml5RendererIndex.vue'; | ||
|
|
||
| const { articleContent$ } = createTranslator( | ||
| SafeHtml5RendererIndex.name, | ||
| SafeHtml5RendererIndex.$trs, | ||
| ); | ||
|
|
||
| let mockEmitFromSafeHTML; | ||
|
|
||
| jest.mock('kolibri', () => ({ | ||
| canHandleElement: jest.fn(() => false), | ||
| })); | ||
|
|
||
| jest.mock('kolibri/composables/useContentViewer'); | ||
|
|
||
| // Mock SafeHTML to render html content and provide controllable event emission | ||
| // for testing how the parent aggregates progress from embedded viewers. | ||
| jest.mock('kolibri-common/components/SafeHTML', () => ({ | ||
| createSafeHTML: () => ({ | ||
| name: 'SafeHTML', | ||
| props: { html: String }, | ||
| created() { | ||
| mockEmitFromSafeHTML = (event, ...args) => this.$emit(event, ...args); | ||
| }, | ||
| render(h) { | ||
| return h('div', { domProps: { innerHTML: this.html || '' } }); | ||
| }, | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock('kolibri-common/components/SafeHTML/style.scss', () => ({})); | ||
| jest.mock('kolibri-zip', () => { | ||
| return jest.fn().mockImplementation(() => ({ | ||
| file: jest.fn().mockResolvedValue({ | ||
| toString: () => '<h1>Mocked HTML content</h1>', | ||
| }), | ||
| })); | ||
| }); | ||
|
|
||
| const renderComponent = () => { | ||
| return render(SafeHtml5RendererIndex); | ||
| }; | ||
|
|
||
| describe('SafeHtml5RendererIndex progress aggregation', () => { | ||
| beforeEach(() => { | ||
| useContentViewer.mockImplementation(() => | ||
| useContentViewerMock({ defaultFile: { storage_url: 'mock://test.html' } }), | ||
| ); | ||
| jest.useFakeTimers(); | ||
| mockEmitFromSafeHTML = null; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| }); | ||
|
|
||
| async function renderAndLoad() { | ||
| const result = renderComponent(); | ||
| await screen.findByRole('region', { name: articleContent$() }); | ||
| return result; | ||
| } | ||
|
|
||
| function getLastEmittedProgress(emitted) { | ||
| jest.advanceTimersByTime(5000); | ||
| const events = emitted().updateProgress; | ||
| return events[events.length - 1][0]; | ||
| } | ||
|
|
||
| // In jsdom, scrollHeight and clientHeight are both 0, so maxScroll = 0 | ||
| // and handleScroll sets scrollBasedProgress = 1 (content considered fully read). | ||
| function simulateFullScroll() { | ||
| const wrapper = document.querySelector('[data-testid="safe-html-wrapper"]'); | ||
| wrapper.dispatchEvent(new Event('scroll')); | ||
| jest.advanceTimersByTime(150); // flush debounce | ||
| } | ||
|
|
||
| it('reports scroll-based progress when no embedded viewers exist', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| expect(getLastEmittedProgress(emitted)).toBe(0); | ||
| }); | ||
|
|
||
| it('averages scroll and viewer progress with one viewer', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1'); | ||
| // scroll=0, viewer=0.5, aggregated=(0+0.5)/2=0.25 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0.25); | ||
| }); | ||
|
|
||
| it('averages scroll and average viewer progress with multiple viewers', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-2'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.25, 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.75, 'viewer-2'); | ||
| // scroll=0, viewer avg=(0.25+0.75)/2=0.5, aggregated=(0+0.5)/2=0.25 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0.25); | ||
| }); | ||
|
|
||
| it('accumulates delta progress via addProgress', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('addProgress', 0.25, 'viewer-1'); | ||
| mockEmitFromSafeHTML('addProgress', 0.25, 'viewer-1'); | ||
| // viewer=0.5, scroll=0, aggregated=(0+0.5)/2=0.25 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0.25); | ||
| }); | ||
|
|
||
| it('clamps negative progress to 0', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean if i am on a video player and i have watched 50% of it, and I restart, it doesn't subtract it? I don't think so.... I think it just means that if I somehow get negative total progress i can't go less than zero, like setting a lower bound. right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, basically, if there's a weird error and for some reason a renderer emits a progress < 0. |
||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', -0.5, 'viewer-1'); | ||
| // clamped to 0, aggregated=(0+0)/2=0 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0); | ||
| }); | ||
|
|
||
| it('clamps progress above 1 to 1', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 1.5, 'viewer-1'); | ||
| // clamped to 1, aggregated=(0+1)/2=0.5 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0.5); | ||
| }); | ||
|
|
||
| it('reverts to scroll-only progress after viewer unregisters', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one also feels a bit unclear to me. maybe it's because i can't quite keep track of the where the registering and unregistering is managed and I think i might be conflating it with rendering. I think it's within the child (responsible for unregistering itself) but I am having a bit of trouble putting the pieces together about what this means practically -- a scenario where this happens with a particular content node and what that means for the progress for the user and UX
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the only case where this would occur is again, if there was an unrecoverable error in the content viewer, and it implodes - so this makes sure that you aren't stuck not being able to complete the article because of a dodgy resource. |
||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.8, 'viewer-1'); | ||
| mockEmitFromSafeHTML('stopTracking', 'viewer-1'); | ||
| // no viewers left, progress=scrollBasedProgress=0 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0); | ||
| }); | ||
|
|
||
| it('does not re-register an already registered viewer', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1'); | ||
| // Re-registering should not reset progress | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| // Still one viewer at 0.5, aggregated=(0+0.5)/2=0.25 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0.25); | ||
| }); | ||
|
|
||
| it('ignores events without a viewerId', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', null); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, null); | ||
| mockEmitFromSafeHTML('addProgress', 0.5, null); | ||
| mockEmitFromSafeHTML('finished', null); | ||
| mockEmitFromSafeHTML('stopTracking', null); | ||
| // No viewers registered, progress=scroll=0 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0); | ||
| }); | ||
|
|
||
| it('ignores updates to unregistered viewers', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'unknown'); | ||
| mockEmitFromSafeHTML('addProgress', 0.5, 'unknown'); | ||
| mockEmitFromSafeHTML('finished', 'unknown'); | ||
| // No viewers, progress=scroll=0 | ||
| expect(getLastEmittedProgress(emitted)).toBe(0); | ||
| }); | ||
|
|
||
| it('does not emit finished when progress is below 1', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1'); | ||
| jest.advanceTimersByTime(5000); | ||
| expect(emitted().finished).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not emit finished when viewers are incomplete', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1'); | ||
| simulateFullScroll(); | ||
| // scroll=1, viewer=0.5, aggregated=(1+0.5)/2=0.75 and viewer not complete | ||
| expect(emitted().finished).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('emits finished when scroll is complete and no embedded viewers exist', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| simulateFullScroll(); | ||
| // scroll=1, no viewers, aggregated=1 | ||
| expect(emitted().finished).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('emits finished when scroll is complete and all viewers are finished', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-2'); | ||
| mockEmitFromSafeHTML('finished', 'viewer-1'); | ||
| mockEmitFromSafeHTML('finished', 'viewer-2'); | ||
| simulateFullScroll(); | ||
| // scroll=1, both viewers complete at progress=1 | ||
| // aggregated=(1+1)/2=1 | ||
| expect(emitted().finished).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('emits finished when a viewer reaches progress 1 without emitting finished', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 1, 'viewer-1'); | ||
| simulateFullScroll(); | ||
| expect(emitted().finished).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('emits finished on duration-based progress regardless of embedded viewers', async () => { | ||
| useContentViewer.mockImplementation(() => | ||
| useContentViewerMock({ | ||
| defaultFile: { storage_url: 'mock://test.html' }, | ||
| forceDurationBasedProgress: true, | ||
| durationBasedProgress: 1, | ||
| }), | ||
| ); | ||
| const { emitted } = await renderAndLoad(); | ||
| mockEmitFromSafeHTML('startTracking', 'viewer-1'); | ||
| mockEmitFromSafeHTML('updateProgress', 0.5, 'viewer-1'); | ||
| jest.advanceTimersByTime(5000); | ||
| expect(emitted().finished).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('emits finished only once across multiple recordProgress calls', async () => { | ||
| const { emitted } = await renderAndLoad(); | ||
| simulateFullScroll(); // triggers recordProgress via handleScroll | ||
| jest.advanceTimersByTime(5000); // triggers another recordProgress via poll | ||
| jest.advanceTimersByTime(5000); // and another | ||
| expect(emitted().finished).toHaveLength(1); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved — addressed in the current code.
suggestion: Because progress averages over the current viewer set, a viewer registering mid-session lowers the average and pushes the emitted
updateProgressvalue down (e.g. reader ~1 with viewer-1, then viewer-2 lazy-loads at 0 → drops to ~0.5). The backend keeps the max so learner-facing progress won't regress, but the emitted stream is non-monotonic. If embeds can mount lazily after scroll, this will happen in practice — worth seeding late registrants or noting the intent.