Merge branch 'feature-indicators-invisible-in-collapsed-connection-panel-des-1162'

3 files changed, 78 insertions, 74 deletions

diff --git a/gui/src/renderer/components/main-view/ConnectionPanel.tsx b/gui/src/renderer/components/main-view/ConnectionPanel.tsx
index 29c4139703..1d5c549254 100644
--- a/gui/src/renderer/components/main-view/ConnectionPanel.tsx
+++ b/gui/src/renderer/components/main-view/ConnectionPanel.tsx
@@ -83,7 +83,7 @@ export default function ConnectionPanel() {
     tunnelState.featureIndicators !== undefined &&
     tunnelState.featureIndicators.length > 0;
 
-  useEffect(collapse, [tunnelState, collapse]);
+  useEffect(collapse, [tunnelState.state, collapse]);
 
   return (
     <StyledConnectionPanel $expanded={expanded}>
diff --git a/gui/src/renderer/components/main-view/FeatureIndicators.tsx b/gui/src/renderer/components/main-view/FeatureIndicators.tsx
index 81495e46eb..a0a8091a5e 100644
--- a/gui/src/renderer/components/main-view/FeatureIndicators.tsx
+++ b/gui/src/renderer/components/main-view/FeatureIndicators.tsx
@@ -1,4 +1,4 @@
-import { useLayoutEffect, useRef } from 'react';
+import { useEffect, useRef } from 'react';
 import { sprintf } from 'sprintf-js';
 import styled from 'styled-components';
 
@@ -55,12 +55,13 @@ const StyledFeatureIndicatorLabel = styled.span<{ $expanded: boolean }>(tinyText
   visibility: props.$expanded ? 'visible' : 'hidden',
 }));
 
-const StyledBaseEllipsis = styled.span(tinyText, {
+const StyledBaseEllipsis = styled.span<{ $display: boolean }>(tinyText, (props) => ({
   position: 'absolute',
-  bottom: 0,
+  top: `${LINE_HEIGHT + GAP}px`,
   color: colors.white,
   padding: '2px 8px 2px 16px',
-});
+  display: props.$display ? 'inline' : 'none',
+}));
 
 const StyledEllipsisSpacer = styled(StyledBaseEllipsis)({
   right: 0,
@@ -94,6 +95,7 @@ export default function FeatureIndicators(props: FeatureIndicatorsProps) {
 
   const featureIndicatorsVisible =
     tunnelState.state === 'connected' || tunnelState.state === 'connecting';
+
   const featureIndicators = useRef(
     featureIndicatorsVisible ? tunnelState.featureIndicators ?? [] : [],
   );
@@ -104,53 +106,58 @@ export default function FeatureIndicators(props: FeatureIndicatorsProps) {
 
   const ellipsis = messages.gettext('%(amount)d more...');
 
-  useLayoutEffect(() => {
-    if (
-      !props.expanded &&
-      featureIndicatorsContainerRef.current &&
-      ellipsisSpacerRef.current &&
-      ellipsisRef.current
-    ) {
-      // Get all feature indicator elements.
-      const indicatorElements = Array.from(
-        featureIndicatorsContainerRef.current.getElementsByTagName('span'),
-      );
-      let lastVisibleIndex = 0;
-      let hasHidden = false;
-      indicatorElements.forEach((indicatorElement, i) => {
-        if (
-          !indicatorShouldBeHidden(
-            featureIndicatorsContainerRef.current!,
-            indicatorElement,
-            ellipsisSpacerRef.current!,
-          )
-        ) {
-          // If an indicator should be visible we set its visibility and increment the variable
-          // containing the last visible index.
-          indicatorElement.style.visibility = 'visible';
-          lastVisibleIndex = i;
-        } else {
-          // If it should be visible we store that there exists hidden indicators.
-          hasHidden = true;
-        }
-      });
+  useEffect(() => {
+    // We need to defer the visibility logic one painting cycle to make sure the elements are
+    // rendered and available.
+    setTimeout(() => {
+      if (
+        !props.expanded &&
+        featureIndicatorsContainerRef.current &&
+        ellipsisSpacerRef.current &&
+        ellipsisRef.current
+      ) {
+        // Get all feature indicator elements.
+        const indicatorElements = Array.from(
+          featureIndicatorsContainerRef.current.getElementsByTagName('span'),
+        );
 
-      if (hasHidden) {
-        const lastVisibleIndicatorRect =
-          indicatorElements[lastVisibleIndex].getBoundingClientRect();
-        const containerRect = featureIndicatorsContainerRef.current.getBoundingClientRect();
+        let lastVisibleIndex = 0;
+        let hasHidden = false;
+        indicatorElements.forEach((indicatorElement, i) => {
+          if (
+            !indicatorShouldBeHidden(
+              featureIndicatorsContainerRef.current!,
+              indicatorElement,
+              ellipsisSpacerRef.current!,
+            )
+          ) {
+            // If an indicator should be visible we set its visibility and increment the variable
+            // containing the last visible index.
+            indicatorElement.style.visibility = 'visible';
+            lastVisibleIndex = i;
+          } else {
+            // If it should be visible we store that there exists hidden indicators.
+            hasHidden = true;
+          }
+        });
 
-        // Place the ellipsis at the end of the last visible indicator.
-        const left = lastVisibleIndicatorRect.right - containerRect.left;
-        ellipsisRef.current.style.left = `${left}px`;
-        ellipsisRef.current.style.visibility = 'visible';
+        if (hasHidden) {
+          const lastVisibleIndicatorRect =
+            indicatorElements[lastVisibleIndex].getBoundingClientRect();
+          const containerRect = featureIndicatorsContainerRef.current.getBoundingClientRect();
 
-        // Add the ellipsis text to the ellipsis.
-        ellipsisRef.current.textContent = sprintf(ellipsis, {
-          amount: indicatorElements.length - (lastVisibleIndex + 1),
-        });
+          // Place the ellipsis at the end of the last visible indicator.
+          const left = lastVisibleIndicatorRect.right - containerRect.left;
+          ellipsisRef.current.style.left = `${left}px`;
+          ellipsisRef.current.style.visibility = 'visible';
+
+          // Add the ellipsis text to the ellipsis.
+          ellipsisRef.current.textContent = sprintf(ellipsis, {
+            amount: indicatorElements.length - (lastVisibleIndex + 1),
+          });
+        }
       }
-    }
+    }, 0);
   });
 
   return (
@@ -172,18 +179,14 @@ export default function FeatureIndicators(props: FeatureIndicatorsProps) {
               </StyledFeatureIndicatorLabel>
             ))}
           </StyledFeatureIndicatorsWrapper>
-          {!props.expanded && (
-            <>
-              <StyledEllipsis ref={ellipsisRef} />
-              <StyledEllipsisSpacer ref={ellipsisSpacerRef}>
-                {
-                  // Mock amount for the spacer ellipsis. This needs to be wider than the real
-                  // ellipsis will ever be.
-                  sprintf(ellipsis, { amount: 222 })
-                }
-              </StyledEllipsisSpacer>
-            </>
-          )}
+          <StyledEllipsis $display={!props.expanded} ref={ellipsisRef} />
+          <StyledEllipsisSpacer $display={!props.expanded} ref={ellipsisSpacerRef}>
+            {
+              // Mock amount for the spacer ellipsis. This needs to be wider than the real
+              // ellipsis will ever be.
+              sprintf(ellipsis, { amount: 222 })
+            }
+          </StyledEllipsisSpacer>
         </StyledFeatureIndicators>
       </StyledFeatureIndicatorsContainer>
     </StyledAccordion>
@@ -197,19 +200,14 @@ function indicatorShouldBeHidden(
 ): boolean {
   const indicatorRect = indicator.getBoundingClientRect();
   const ellipsisSpacerRect = ellipsisSpacer.getBoundingClientRect();
+  const containerRect = container.getBoundingClientRect();
 
-  // If 2 or less lines are required to display the indicators all should be visible. This is
-  // calculated based on the scroll height.
-  if (container.scrollHeight <= 2 * LINE_HEIGHT + GAP) {
-    return false;
-  }
+  // Calculate which line the indicator is positioned on.
+  const lineIndex = Math.round((indicatorRect.top - containerRect.top) / (LINE_HEIGHT + GAP));
 
-  // An indicator should be hidden if it's placed farther down than the spacer ellipsis, or if it
-  // overlaps it.
-  return (
-    indicatorRect.top >= ellipsisSpacerRect.bottom ||
-    (indicatorRect.top === ellipsisSpacerRect.top && indicatorRect.right >= ellipsisSpacerRect.left)
-  );
+  // an indicator should remain hidden if it's on the third line or later, or if it is on the
+  // second line and overlap with the ellipsis.
+  return lineIndex > 1 || (lineIndex === 1 && indicatorRect.right >= ellipsisSpacerRect.left);
 }
 
 function getFeatureIndicatorLabel(indicator: FeatureIndicator) {
diff --git a/gui/test/e2e/mocked/feature-indicators.spec.ts b/gui/test/e2e/mocked/feature-indicators.spec.ts
index 0cb0b1b1ec..1ba216bd52 100644
--- a/gui/test/e2e/mocked/feature-indicators.spec.ts
+++ b/gui/test/e2e/mocked/feature-indicators.spec.ts
@@ -58,6 +58,7 @@ test('App should show no feature indicators', async () => {
   await expect(ellipsis).not.toBeVisible();
 
   await expectFeatureIndicators(page, []);
+  await page.getByTestId('connection-panel-chevron').click();
 });
 
 test('App should show feature indicators', async () => {
@@ -85,13 +86,14 @@ test('App should show feature indicators', async () => {
     },
   });
 
+  // Make sure panel is collapsed before checking indicator visibility.
+  const ellipsis = page.getByText(/^\d more.../);
+  await expect(ellipsis).toBeVisible();
+
   await expectConnected(page);
   await expectFeatureIndicators(page, ["DAITA", "Quantum resistance"], false);
   await expectHiddenFeatureIndicator(page, "Mssfix");
 
-  const ellipsis = page.getByText(/^\d more.../);
-  await expect(ellipsis).toBeVisible();
-
   await page.getByTestId('connection-panel-chevron').click();
   await expect(ellipsis).not.toBeVisible();
 
@@ -113,6 +115,10 @@ async function expectHiddenFeatureIndicator(page: Page, hiddenIndicator: string)
   const indicators = page.getByTestId('feature-indicator');
   const indicator = indicators.getByText(hiddenIndicator, { exact: true });
 
+  // Make sure at least one is visible to not run the "not visible" check before they become
+  // visible.
+  await expect(indicators.first()).toBeVisible();
+
   await expect(indicator).toHaveCount(1);
   await expect(indicator).not.toBeVisible();
 }