[Bug 42799] New: Toolbar stickiness implementation is flaky
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Bug ID: 42799 Summary: Toolbar stickiness implementation is flaky Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Staff interface Assignee: jonathan.druart@gmail.com Reporter: jonathan.druart@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com Depends on: 36694 Highlighted by the Cypress upgrade (bug 42772): the sticky element is always visible and IntersectionObserver rely on the visibility of the element. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36694 [Bug 36694] Remove HC Sticky library in favor of CSS -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 200128 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200128&action=edit Bug 42799: Fix sticky behavior by using a sentinel node Highlighted by the Cypress upgrade (bug 42772): the sticky element is always visible and IntersectionObserver rely on the visibility of the element. Cypress test Toolbar_spec.ts use scrollTo("bottom") to test the behaviour of the sticky toolbar. We test that the "floating" class is correctly added to the element. To do that we rely on IntersectionObserver that will check the visibility of the element. But the sticky element is always visible. I actually do not recreate the problem when using the UI but the current code may be broken in some browsers (or new/future versions). To fix this problem we insert a new "sentinel" element into the DOM and rely on its visibility to toggle the "floating" class. Test plan: Confirm that the sticky toolbar behaviour is the same after this patch is applied, ie. that the "floating" class is added when we scroll down. On top of bug 42772 confirm that the following command passes (run it several times): yarn cypress run --spec t/cypress/integration/Toolbar_spec.ts QA: Note that the apply_sticky function takes only one node in parameter, that's why it has been renamed nodes => node -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- It actually still fails, less often but still :-( -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 200129 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200129&action=edit Bug 42799 : Replace IntersectionObserver with scroll listener for sticky behaviour Assisted-by: Mistral Vibe <vibe@mistral.ai> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com, | |paul.derscheid@lmscloud.de -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42772 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42772 [Bug 42772] Upgrade Cypress to 15.16.0 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #4 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 200134 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200134&action=edit Bug 42799: Replace IntersectionObserver with scroll listener for sticky behaviour Assisted-by: Mistral Vibe <vibe@mistral.ai> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200129|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200128|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #5 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 200136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200136&action=edit Bug 42799: Fix sticky behavior by using a sentinel node Highlighted by the Cypress upgrade (bug 42772): the sticky element is always visible and IntersectionObserver rely on the visibility of the element. Cypress test Toolbar_spec.ts use scrollTo("bottom") to test the behaviour of the sticky toolbar. We test that the "floating" class is correctly added to the element. To do that we rely on IntersectionObserver that will check the visibility of the element. But the sticky element is always visible. I actually do not recreate the problem when using the UI but the current code may be broken in some browsers (or new/future versions). To fix this problem we insert a new "sentinel" element into the DOM and rely on its visibility to toggle the "floating" class. Test plan: Confirm that the sticky toolbar behaviour is the same after this patch is applied, ie. that the "floating" class is added when we scroll down. On top of bug 42772 confirm that the following command passes (run it several times): yarn cypress run --spec t/cypress/integration/Toolbar_spec.ts QA: Note that the apply_sticky function takes only one node in parameter, that's why it has been renamed nodes => node Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #6 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 200137 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200137&action=edit Bug 42799 : Replace IntersectionObserver with scroll listener for sticky behaviour Assisted-by: Mistral Vibe <vibe@mistral.ai> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200134|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200136|0 |1 is obsolete| | Attachment #200137|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201644&action=edit Bug 42799: Fix sticky behavior by using a sentinel node Highlighted by the Cypress upgrade (bug 42772): the sticky element is always visible and IntersectionObserver rely on the visibility of the element. Cypress test Toolbar_spec.ts use scrollTo("bottom") to test the behaviour of the sticky toolbar. We test that the "floating" class is correctly added to the element. To do that we rely on IntersectionObserver that will check the visibility of the element. But the sticky element is always visible. I actually do not recreate the problem when using the UI but the current code may be broken in some browsers (or new/future versions). To fix this problem we insert a new "sentinel" element into the DOM and rely on its visibility to toggle the "floating" class. Test plan: Confirm that the sticky toolbar behaviour is the same after this patch is applied, ie. that the "floating" class is added when we scroll down. On top of bug 42772 confirm that the following command passes (run it several times): yarn cypress run --spec t/cypress/integration/Toolbar_spec.ts QA: Note that the apply_sticky function takes only one node in parameter, that's why it has been renamed nodes => node Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201645 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201645&action=edit Bug 42799: Replace IntersectionObserver with scroll listener for sticky behaviour Assisted-by: Mistral Vibe <vibe@mistral.ai> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42799 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201646 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201646&action=edit Bug 42799: (QA follow-up) Close scroll listener leak in apply_sticky apply_sticky is invoked from a watch() in Toolbar.vue every time the toolbar element (re)mounts. The scroll listener it registers was never removed, so long-lived SPA modules (ERM, ILL, Acquisitions, Preservation, SIP2) that mount/unmount this Toolbar as the user navigates between records leaked another window-level scroll listener on every remount. An IntersectionObserver + sentinel node was tried instead, since it's the standard technique and doesn't poll on every scroll frame, but verified against KTD/Cypress it fails t/cypress/integration/Toolbar_spec.ts consistently: the observer's callback fires once on attach and never fires again after cy.scrollTo("bottom"), even though the sentinel is confirmed (via instrumentation) to have scrolled fully out of the viewport. The scroll-listener approach passes the same spec reliably across repeated runs. So the scroll listener stays; apply_sticky now returns a disconnect() callback and Toolbar.vue calls it before re-applying and on unmount, closing the leak without touching detection behaviour. Test plan: 1. Rebuild assets (yarn build) and confirm: yarn cypress run --spec t/cypress/integration/Toolbar_spec.ts passes consistently across several runs. 2. Manually confirm sticky toolbars in ERM, Acquisitions (vendors), ILL, Preservation and SIP2 still float correctly on scroll, and that navigating between several records doesn't accumulate extra "scroll" listeners on window (DevTools -> getEventListeners(window)). Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon@bugs.koha-community.org