ensure JS.show/hide/toggle work as before, fix focus (#3692)

* ensure JS.show/hide/toggle work as before, fix focus

Fixes #3691.
Fixes #3675.
Fixes #3563.
Relates to: #3657
Relates to: f06877a

* try to clarify comments

assets/js/phoenix_live_view/js.js

@@ -94,22 +94,36 @@ let JS = {
   },
 
   exec_focus(e, eventType, phxEvent, view, sourceEl, el){
-    window.requestAnimationFrame(() => ARIA.attemptFocus(el))
+    ARIA.attemptFocus(el)
+    // in case the JS.focus command is in a JS.show/hide/toggle chain, for show we need
+    // to wait for JS.show to have updated the element's display property (see exec_toggle)
+    // but that run in nested animation frames, therefore we need to use them here as well
+    window.requestAnimationFrame(() => {
+      window.requestAnimationFrame(() => ARIA.attemptFocus(el))
+    })
   },
 
   exec_focus_first(e, eventType, phxEvent, view, sourceEl, el){
-    window.requestAnimationFrame(() => ARIA.focusFirstInteractive(el) || ARIA.focusFirst(el))
+    ARIA.focusFirstInteractive(el) || ARIA.focusFirst(el)
+    // if you wonder about the nested animation frames, see exec_focus
+    window.requestAnimationFrame(() => {
+      window.requestAnimationFrame(() => ARIA.focusFirstInteractive(el) || ARIA.focusFirst(el))
+    })
   },
 
   exec_push_focus(e, eventType, phxEvent, view, sourceEl, el){
-    window.requestAnimationFrame(() => focusStack.push(el || sourceEl))
+    focusStack.push(el || sourceEl)
   },
 
   exec_pop_focus(_e, _eventType, _phxEvent, _view, _sourceEl, _el){
-    window.requestAnimationFrame(() => {
-      const el = focusStack.pop()
-      if(el){ el.focus() }
-    })
+    const el = focusStack.pop()
+    if(el){
+      el.focus()
+      // if you wonder about the nested animation frames, see exec_focus
+      window.requestAnimationFrame(() => {
+        window.requestAnimationFrame(() => el.focus())
+      })
+    }
   },
 
   exec_add_class(e, eventType, phxEvent, view, sourceEl, el, {names, transition, time, blocking}){
@@ -195,11 +209,19 @@ let JS = {
         if(eventType === "remove"){ return }
         let onStart = () => {
           this.addOrRemoveClasses(el, inStartClasses, outClasses.concat(outStartClasses).concat(outEndClasses))
-          let stickyDisplay = display || this.defaultDisplay(el)
-          DOM.putSticky(el, "toggle", currentEl => currentEl.style.display = stickyDisplay)
+          const stickyDisplay = display || this.defaultDisplay(el)
           window.requestAnimationFrame(() => {
+            // first add the starting + active class, THEN make the element visible
+            // otherwise if we toggled the visibility earlier css animations
+            // would flicker, as the element becomes visible before the active animation
+            // class is set (see https://github.com/phoenixframework/phoenix_live_view/issues/3456)
             this.addOrRemoveClasses(el, inClasses, [])
-            window.requestAnimationFrame(() => this.addOrRemoveClasses(el, inEndClasses, inStartClasses))
+            // addOrRemoveClasses uses a requestAnimationFrame itself, therefore we need to move the putSticky
+            // into the next requestAnimationFrame...
+            window.requestAnimationFrame(() => {
+              DOM.putSticky(el, "toggle", currentEl => currentEl.style.display = stickyDisplay)
+              this.addOrRemoveClasses(el, inEndClasses, inStartClasses)
+            })
           })
         }
         let onEnd = () => {

assets/test/js_test.js

@@ -17,6 +17,7 @@ describe("JS", () => {
   beforeEach(() => {
     global.document.body.innerHTML = ""
     jest.useFakeTimers()
+    setStartSystemTime()
   })
 
   afterEach(() => {
@@ -196,11 +197,10 @@ describe("JS", () => {
       done()
     })
 
-    test("with in and out classes", (done) => {
-      jest.useFakeTimers()
+    test("with in and out classes", async () => {
       let view = setupView(`
       <div id="modal">modal</div>
-      <div id="click" phx-click='[["toggle", {"to": "#modal", "ins": [["fade-in"],[],[]], "outs": [["fade-out"],[],[]]}]]'></div>
+      <div id="click" phx-click='[["toggle", {"to": "#modal", "ins": [["fade-in"],["fade-in-start"],["fade-in-end"]], "outs": [["fade-out"],["fade-out-start"],["fade-out-end"]]}]]'></div>
       `)
       let modal = simulateVisibility(document.querySelector("#modal"))
       let click = document.querySelector("#click")
@@ -217,26 +217,62 @@ describe("JS", () => {
       expect(modal.classList.contains("fade-out")).toBe(false)
       expect(modal.classList.contains("fade-in")).toBe(false)
 
-      // toggle in
+      // toggle out
       JS.exec(event, "click", click.getAttribute("phx-click"), view, click)
-      jest.advanceTimersByTime(100)
+      expect(hideStartCalled).toBe(true)
+      // first tick: waiting for start classes to be set
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-out-start")).toBe(true)
+      expect(modal.classList.contains("fade-out")).toBe(false)
+      // second tick: waiting for out classes to be set
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-out-start")).toBe(true)
       expect(modal.classList.contains("fade-out")).toBe(true)
-      expect(modal.classList.contains("fade-in")).toBe(false)
+      // third tick: waiting for outEndClasses
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-out-start")).toBe(false)
+      expect(modal.classList.contains("fade-out")).toBe(true)
+      expect(modal.classList.contains("fade-out-end")).toBe(true)
+      // wait for onEnd
       jest.runAllTimers()
+      advanceTimersToNextFrame()
+      // fifth tick: display: none
+      advanceTimersToNextFrame()
+      expect(hideEndCalled).toBe(true)
+      expect(modal.style.display).toEqual("none")
+      // sixth tick, removed end classes
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-out-start")).toBe(false)
+      expect(modal.classList.contains("fade-out")).toBe(false)
+      expect(modal.classList.contains("fade-out-end")).toBe(false)
 
-      // toggle out
+      // toggle in
       JS.exec(event, "click", click.getAttribute("phx-click"), view, click)
-      jest.advanceTimersByTime(100)
-      expect(modal.classList.contains("fade-out")).toBe(false)
+      expect(showStartCalled).toBe(true)
+      // first tick: waiting for start classes to be set
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-in-start")).toBe(true)
+      expect(modal.classList.contains("fade-in")).toBe(false)
+      expect(modal.style.display).toEqual("none")
+      // second tick: waiting for in classes to be set
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-in-start")).toBe(true)
       expect(modal.classList.contains("fade-in")).toBe(true)
+      expect(modal.classList.contains("fade-in-end")).toBe(false)
+      expect(modal.style.display).toEqual("block")
+      // third tick: waiting for inEndClasses
+      advanceTimersToNextFrame()
+      expect(modal.classList.contains("fade-in-start")).toBe(false)
+      expect(modal.classList.contains("fade-in")).toBe(true)
+      expect(modal.classList.contains("fade-in-end")).toBe(true)
+      // wait for onEnd
       jest.runAllTimers()
-
+      advanceTimersToNextFrame()
       expect(showEndCalled).toBe(true)
-      expect(hideEndCalled).toBe(true)
-      expect(showStartCalled).toBe(true)
-      expect(hideStartCalled).toBe(true)
-
-      done()
+      // sixth tick, removed end classes
+      expect(modal.classList.contains("fade-in-start")).toBe(false)
+      expect(modal.classList.contains("fade-in")).toBe(false)
+      expect(modal.classList.contains("fade-in-end")).toBe(false)
     })
   })
 

jest.config.js

@@ -144,7 +144,9 @@ module.exports = {
   ],
 
   // A list of paths to modules that run some code to configure or set up the testing framework before each test
-  // setupFilesAfterEnv: [],
+  setupFilesAfterEnv: [
+    "<rootDir>/setupTestsAfterEnv.js"
+  ],
 
   // The number of seconds after which a test is considered as slow and reported as such in the results.
   // slowTestThreshold: 5,

setupTestsAfterEnv.js

@@ -0,0 +1,33 @@
+// https://github.com/jestjs/jest/pull/14598#issuecomment-1748047560
+// TODO: remove this once jest.advanceTimersToNextFrame() is available
+// ensure you are using "modern" fake timers
+// 1. before doing anything, grab the start time `setStartSystemTime()`
+// 2. step through frames by using `advanceTimersToNextFrame()`
+
+let startTime = null
+
+/** Record the initial (mocked) system start time 
+ * 
+ * This is no longer needed once `jest.advanceTimersToNextFrame()` is available
+ * https://github.com/jestjs/jest/pull/14598
+*/
+global.setStartSystemTime = () => {
+  startTime = Date.now()
+}
+
+/** Step forward a single animation frame
+ * 
+ * This is no longer needed once `jest.advanceTimersToNextFrame()` is available
+ * https://github.com/jestjs/jest/pull/14598
+ */
+global.advanceTimersToNextFrame = () => {
+  if(startTime == null){
+    throw new Error("Must call `setStartSystemTime` before using `advanceTimersToNextFrame()`")
+  }
+
+  // Stealing logic from sinon fake timers
+  // https://github.com/sinonjs/fake-timers/blob/fc312b9ce96a4ea2c7e60bb0cccd2c604b75cdbd/src/fake-timers-src.js#L1102-L1105
+  const timePassedInFrame = (Date.now() - startTime) % 16
+  const timeToNextFrame = 16 - timePassedInFrame
+  jest.advanceTimersByTime(timeToNextFrame)
+}