More E2E tests for Component Browser (#8559)

More cases in E2E tests checking all ways of using component browser. The tests found one actual bug: sometimes we displayed only a few entries after opening CB because the scroller size was not refreshed (we assumed it will be available in the next tick). Refactored Component Browser so it does not use nextTick anymore.
2024-11-23 08:08:34 +03:00 · 2024-01-03 10:12:38 +01:00 · 2024-01-03 10:12:38 +01:00 · 9c17b260dc
commit 9c17b260dc
parent f878549b78
6 changed files with 234 additions and 60 deletions
--- a/app/gui2/e2e/componentBrowser.spec.ts
+++ b/app/gui2/e2e/componentBrowser.spec.ts
@ -1,23 +1,136 @@
 import { expect, test } from '@playwright/test'
+import assert from 'assert'
 import * as actions from './actions'
 import * as customExpect from './customExpect'
 import * as locate from './locate'

-test('component browser shows entries, and creates a new node', async ({ page }) => {
+test('Different ways of opening Component Browser', async ({ page }) => {
  await actions.goToGraph(page)
-  await locate.graphEditor(page).click()
+  const nodeCount = await locate.graphNode(page).count()
+
+  async function expectAndCancelBrowser(expectedInput: string) {
+    await customExpect.toExist(locate.componentBrowser(page))
+    await customExpect.toExist(locate.componentBrowserEntry(page))
+    await expect(locate.componentBrowserInput(page).locator('input')).toHaveValue(expectedInput)
+    await page.keyboard.press('Escape')
+    await expect(locate.componentBrowser(page)).not.toBeVisible()
+    await expect(locate.graphNode(page)).toHaveCount(nodeCount)
+  }
+
+  // Without source node
+
+  // (+) button
+  await locate.addNewNodeButton(page).click()
+  await expectAndCancelBrowser('')
+  // Enter key
  await locate.graphEditor(page).press('Enter')
-  await customExpect.toExist(locate.componentBrowser(page))
-  await customExpect.toExist(locate.componentBrowserEntry(page))
+  await expectAndCancelBrowser('')
+
+  // With source node
+
+  // (+) button
+  await locate.graphNodeByBinding(page, 'benches').click()
+  await locate.addNewNodeButton(page).click()
+  await expectAndCancelBrowser('benches.')
+  // Enter key
+  await locate.graphNodeByBinding(page, 'benches').click()
+  await locate.graphEditor(page).press('Enter')
+  await expectAndCancelBrowser('benches.')
+  // Dragging out an edge
+  // `click` method of locator could be simpler, but `position` option doesn't work.
+  const outputPortArea = await locate
+    .graphNodeByBinding(page, 'benches')
+    .locator('.outputPortHoverArea')
+    .boundingBox()
+  assert(outputPortArea)
+  const outputPortX = outputPortArea.x + outputPortArea.width / 2.0
+  const outputPortY = outputPortArea.y + outputPortArea.height - 2.0
+  await page.mouse.click(outputPortX, outputPortY)
+  await page.mouse.click(40, 300)
+  await expectAndCancelBrowser('benches.')
+  // Double-clicking port
+  await page.mouse.click(outputPortX, outputPortY)
+  // TODO[ao] the above click is already treated as double (due to previous event)
+  //  But perhaps we should have more reliable method of simulating double clicks.
+  // await outputPortArea.dispatchEvent('pointerdown')
+  await expectAndCancelBrowser('benches.')
+})
+
+test('Accepting suggestion', async ({ page }) => {
+  // Clicking enry
+  await actions.goToGraph(page)
+  await locate.addNewNodeButton(page).click()
  let nodeCount = await locate.graphNode(page).count()
  await locate.componentBrowserEntry(page).nth(1).click()
  await expect(locate.componentBrowser(page)).not.toBeVisible()
  await expect(locate.graphNode(page)).toHaveCount(nodeCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText([
+    'Data',
+    '.',
+    'read_text',
+  ])

-  // Clicking at highlighted entry should also work.
+  // Clicking at highlighted entry
  nodeCount = await locate.graphNode(page).count()
-  await locate.graphEditor(page).press('Enter')
+  await locate.addNewNodeButton(page).click()
  await locate.componentBrowserSelectedEntry(page).first().click()
  await expect(locate.componentBrowser(page)).not.toBeVisible()
  await expect(locate.graphNode(page)).toHaveCount(nodeCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText([
+    'Data',
+    '.',
+    'read',
+  ])
+
+  // Accepting with Enter
+  nodeCount = await locate.graphNode(page).count()
+  await locate.addNewNodeButton(page).click()
+  await page.keyboard.press('Enter')
+  await expect(locate.componentBrowser(page)).not.toBeVisible()
+  await expect(locate.graphNode(page)).toHaveCount(nodeCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText([
+    'Data',
+    '.',
+    'read',
+  ])
+})
+
+test('Accepting any written input', async ({ page }) => {
+  await actions.goToGraph(page)
+  await locate.addNewNodeButton(page).click()
+  const nodeCount = await locate.graphNode(page).count()
+  await locate.componentBrowserInput(page).locator('input').fill('re')
+  await page.keyboard.press('Control+Enter')
+  await expect(locate.componentBrowser(page)).not.toBeVisible()
+  await expect(locate.graphNode(page)).toHaveCount(nodeCount + 1)
+  await expect(locate.graphNode(page).last().locator('.WidgetToken')).toHaveText('re')
+})
+
+test('Filling input with suggestions', async ({ page }) => {
+  await actions.goToGraph(page)
+  await locate.addNewNodeButton(page).click()
+
+  // Entering module
+  await locate.componentBrowserEntryByLabel(page, 'Standard.Base.Data').click()
+  await customExpect.toExist(locate.componentBrowser(page))
+  await expect(locate.componentBrowserInput(page).locator('input')).toHaveValue(
+    'Standard.Base.Data.',
+  )
+
+  // Applying suggestion
+  page.keyboard.press('Tab')
+  await customExpect.toExist(locate.componentBrowser(page))
+  await expect(locate.componentBrowserInput(page).locator('input')).toHaveValue(
+    'Standard.Base.Data.read ',
+  )
+})
+
+test('Filtering list', async ({ page }) => {
+  await actions.goToGraph(page)
+  await locate.addNewNodeButton(page).click()
+  await locate.componentBrowserInput(page).locator('input').fill('re_te')
+  const segments = locate.componentBrowserEntry(page).locator('.component-label-segment')
+  await expect(segments).toHaveText(['Data.', 're', 'ad', '_te', 'xt'])
+  const highlighted = locate.componentBrowserEntry(page).locator('.component-label-segment.match')
+  await expect(highlighted).toHaveText(['re', '_te'])
 })
--- a/app/gui2/e2e/locate.ts
+++ b/app/gui2/e2e/locate.ts
@ -129,9 +129,13 @@ function componentLocator<T extends string>(className: SanitizeClassName<T>) {

 export const graphEditor = componentLocator('GraphEditor')
 export const graphNode = componentLocator('GraphNode')
+export function graphNodeByBinding(page: Locator | Page, binding: string) {
+  return graphNode(page).filter({ has: page.locator('.binding').and(page.getByText(binding)) })
+}
 // @ts-expect-error
 export const anyVisualization = componentLocator('GraphVisualization > *')
 export const circularMenu = componentLocator('CircularMenu')
+export const addNewNodeButton = componentLocator('PlusButton')
 export const componentBrowser = componentLocator('ComponentBrowser')

 export function componentBrowserEntry(
@ -152,6 +156,11 @@ export function componentBrowserSelectedEntry(
  )
 }

+export function componentBrowserEntryByLabel(page: Locator | Page, label: string) {
+  return componentBrowserEntry(page).filter({ has: page.getByText(label) })
+}
+
+export const componentBrowserInput = componentLocator('CBInput')
 export const jsonVisualization = componentLocator('JSONVisualization')
 export const tableVisualization = componentLocator('TableVisualization')
 export const scatterplotVisualization = componentLocator('ScatterplotVisualization')
--- a/app/gui2/src/components/ComponentBrowser.vue
+++ b/app/gui2/src/components/ComponentBrowser.vue
@ -3,6 +3,7 @@ import { componentBrowserBindings } from '@/bindings'
 import { makeComponentList, type Component } from '@/components/ComponentBrowser/component'
 import { Filtering } from '@/components/ComponentBrowser/filtering'
 import { useComponentBrowserInput, type Usage } from '@/components/ComponentBrowser/input'
+import { useScrolling } from '@/components/ComponentBrowser/scrolling'
 import { default as DocumentationPanel } from '@/components/DocumentationPanel.vue'
 import GraphVisualization from '@/components/GraphEditor/GraphVisualization.vue'
 import SvgIcon from '@/components/SvgIcon.vue'
@ -21,7 +22,7 @@ import type { Opt } from '@/util/data/opt'
 import { allRanges } from '@/util/data/range'
 import { Vec2 } from '@/util/data/vec2'
 import type { SuggestionId } from 'shared/languageServerTypes/suggestions'
-import { computed, nextTick, onMounted, ref, watch, type ComputedRef, type Ref } from 'vue'
+import { computed, onMounted, ref, watch, type ComputedRef, type Ref } from 'vue'

 const ITEM_SIZE = 32
 const TOP_BAR_HEIGHT = 32
@ -46,16 +47,14 @@ const emit = defineEmits<{
 }>()

 onMounted(() => {
-  nextTick(() => {
-    input.reset(props.usage)
-    if (inputField.value != null) {
-      inputField.value.focus({ preventScroll: true })
-    } else {
-      console.warn(
-        'Component Browser input element was not mounted. This is not expected and may break the Component Browser',
-      )
-    }
-  })
+  input.reset(props.usage)
+  if (inputField.value != null) {
+    inputField.value.focus({ preventScroll: true })
+  } else {
+    console.warn(
+      'Component Browser input element was not mounted. This is not expected and may break the Component Browser',
+    )
+  }
 })

 // === Position ===
@ -91,12 +90,9 @@ const currentFiltering = computed(() => {

 watch(currentFiltering, () => {
  selected.value = input.autoSelectFirstComponent.value ? 0 : null
-  nextTick(() => {
-    scrollToBottom()
-    animatedScrollPosition.skip()
-    animatedHighlightPosition.skip()
-    animatedHighlightHeight.skip()
-  })
+  scrolling.targetScroll.value = { type: 'bottom' }
+  animatedHighlightPosition.skip()
+  animatedHighlightHeight.skip()
 })

 function readInputFieldSelection() {
@ -182,15 +178,15 @@ const previewDataSource: ComputedRef<VisualizationDataSource | undefined> = comp

 // === Components List and Positions ===

-const components = computed(() => {
-  return makeComponentList(suggestionDbStore.entries, currentFiltering.value)
-})
+const components = computed(() =>
+  makeComponentList(suggestionDbStore.entries, currentFiltering.value),
+)

 const visibleComponents = computed(() => {
  if (scroller.value == null) return []
-  const scrollPosition = animatedScrollPosition.value
-  const topmostVisible = componentAtY(scrollPosition)
-  const bottommostVisible = Math.max(0, componentAtY(scrollPosition + scrollerSize.value.y))
+  const scrollPos = scrolling.scrollPosition.value
+  const topmostVisible = componentAtY(scrollPos)
+  const bottommostVisible = Math.max(0, componentAtY(scrollPos + scrollerSize.value.y))
  return components.value.slice(bottommostVisible, topmostVisible + 1).map((component, i) => {
    return { component, index: i + bottommostVisible }
  })
@ -237,13 +233,18 @@ const selectedSuggestion = computed(() => {
  return suggestionDbStore.entries.get(id) ?? null
 })

-watch(selectedPosition, (newPos) => {
-  if (newPos == null) return
-  highlightPosition.value = newPos
-  if (animatedHighlightHeight.value <= 1.0) {
-    animatedHighlightPosition.skip()
-  }
-})
+watch(
+  selectedPosition,
+  (newPos) => {
+    if (newPos == null) return
+    highlightPosition.value = newPos
+    if (animatedHighlightHeight.value <= 1.0) {
+      animatedHighlightPosition.skip()
+    }
+  },
+  // Needs to be synchronous to make skipping highlight animation work.
+  { flush: 'sync' },
+)

 const highlightClipPath = computed(() => {
  let height = animatedHighlightHeight.value
@ -253,33 +254,34 @@ const highlightClipPath = computed(() => {
  return `inset(${top}px 0px ${bottom}px 0px round 16px)`
 })

+function selectWithoutScrolling(index: number) {
+  const scrollPos = scrolling.scrollPosition.value
+  scrolling.targetScroll.value = { type: 'offset', offset: scrollPos }
+  selected.value = index
+}
+
 // === Scrolling ===

 const scroller = ref<HTMLElement>()
 const scrollerSize = useResizeObserver(scroller)
-const scrollPosition = ref(0)
-const animatedScrollPosition = useApproach(scrollPosition)
-
 const listContentHeight = computed(() =>
  // We add a top padding of TOP_BAR_HEIGHT / 2 - otherwise the topmost entry would be covered
  // by top bar.
  Math.max(components.value.length * ITEM_SIZE + TOP_BAR_HEIGHT / 2, scrollerSize.value.y),
 )
+const scrolling = useScrolling(
+  animatedHighlightPosition,
+  computed(() => scrollerSize.value.y),
+  listContentHeight,
+  ITEM_SIZE,
+)
+
 const listContentHeightPx = computed(() => `${listContentHeight.value}px`)

-function scrollToSelected() {
-  if (selectedPosition.value == null) return
-  scrollPosition.value = Math.max(selectedPosition.value - scrollerSize.value.y + ITEM_SIZE, 0)
-}
-
-function scrollToBottom() {
-  scrollPosition.value = listContentHeight.value - scrollerSize.value.y
-}
-
 function updateScroll() {
-  if (scroller.value && Math.abs(scroller.value.scrollTop - animatedScrollPosition.value) > 1.0) {
-    scrollPosition.value = scroller.value.scrollTop
-    animatedScrollPosition.skip()
+  // If the scrollTop value changed significantly, that means the user is scrolling.
+  if (scroller.value && Math.abs(scroller.value.scrollTop - scrolling.scrollPosition.value) > 1.0) {
+    scrolling.targetScroll.value = { type: 'offset', offset: scroller.value.scrollTop }
  }
 }

@ -339,7 +341,7 @@ const handler = componentBrowserBindings.handler({
    if (selected.value != null && selected.value < components.value.length - 1) {
      selected.value += 1
    }
-    scrollToSelected()
+    scrolling.scrollWithTransition({ type: 'selected' })
  },
  moveDown() {
    if (selected.value == null) {
@ -347,7 +349,7 @@ const handler = componentBrowserBindings.handler({
    } else if (selected.value > 0) {
      selected.value -= 1
    }
-    scrollToSelected()
+    scrolling.scrollWithTransition({ type: 'selected' })
  },
  cancelEditing() {
    emit('canceled')
@ -383,7 +385,7 @@ const handler = componentBrowserBindings.handler({
          <div
            ref="scroller"
            class="list"
-            :scrollTop.prop="animatedScrollPosition.value"
+            :scrollTop.prop="scrolling.scrollPosition.value"
            @wheel.stop.passive
            @scroll="updateScroll"
          >
@ -393,7 +395,7 @@ const handler = componentBrowserBindings.handler({
                :key="item.component.suggestionId"
                class="component"
                :style="componentStyle(item.index)"
-                @mousemove="selected = item.index"
+                @mousemove="selectWithoutScrolling(item.index)"
                @click="acceptSuggestion(item.component)"
              >
                <SvgIcon
--- a/app/gui2/src/components/ComponentBrowser/scrolling.ts
+++ b/app/gui2/src/components/ComponentBrowser/scrolling.ts
@ -0,0 +1,45 @@
+import { useApproach } from '@/composables/animation'
+import { computed, ref } from 'vue'
+
+export type ScrollTarget =
+  | { type: 'bottom' }
+  | { type: 'selected' }
+  | { type: 'offset'; offset: number }
+
+export function useScrolling(
+  selectedPos: { value: number },
+  scrollerSize: { value: number },
+  contentSize: { value: number },
+  entrySize: number,
+) {
+  const targetScroll = ref<ScrollTarget>({ type: 'bottom' })
+  const targetScrollPosition = computed(() => {
+    switch (targetScroll.value.type) {
+      case 'selected':
+        return Math.max(selectedPos.value - scrollerSize.value + entrySize, 0)
+      case 'bottom':
+        return contentSize.value - scrollerSize.value
+      case 'offset':
+        return targetScroll.value.offset
+    }
+    return 0.0
+  })
+  const scrollTransitionTarget = ref(0.0)
+  const scrollTransition = useApproach(scrollTransitionTarget)
+  const scrollPosition = computed(() => targetScrollPosition.value + scrollTransition.value)
+
+  function scrollWithTransition(target: ScrollTarget) {
+    const old = scrollPosition.value
+    targetScroll.value = target
+    const change = scrollPosition.value - old
+    scrollTransitionTarget.value = -change
+    scrollTransition.skip()
+    scrollTransitionTarget.value = 0.0
+  }
+
+  return {
+    targetScroll,
+    scrollPosition,
+    scrollWithTransition,
+  }
+}
--- a/app/gui2/src/components/GraphEditor.vue
+++ b/app/gui2/src/components/GraphEditor.vue
@ -510,7 +510,12 @@ async function readNodeFromExcelClipboard(

 function handleNodeOutputPortDoubleClick(id: ExprId) {
  componentBrowserUsage.value = { type: 'newNode', sourcePort: id }
-  const placementEnvironment = environmentForNodes([id].values())
+  const srcNode = graphStore.db.getPatternExpressionNodeId(id)
+  if (srcNode == null) {
+    console.error('Impossible happened: Double click on port not belonging to any node: ', id)
+    return
+  }
+  const placementEnvironment = environmentForNodes([srcNode].values())
  componentBrowserNodePosition.value = previousNodeDictatedPlacement(
    DEFAULT_NODE_SIZE,
    placementEnvironment,
--- a/app/gui2/src/components/PlusButton.vue
+++ b/app/gui2/src/components/PlusButton.vue
@ -1,5 +1,5 @@
 <template>
-  <div class="circle-plus">
+  <div class="PlusButton">
    <div class="vertical"></div>
    <div class="horizontal"></div>
  </div>
@ -8,7 +8,7 @@
 <script setup lang="ts"></script>

 <style scoped>
-.circle-plus {
+.PlusButton {
  position: absolute;
  bottom: 10px;
  left: 10px;
@ -18,11 +18,11 @@
  background-color: white;
 }

-.circle-plus:hover {
+.PlusButton:hover {
  background: rgb(230, 230, 255);
 }

-.circle-plus:active {
+.PlusButton:active {
  background: rgb(158, 158, 255);
 }