FWD » Core Development » User Interface

A simple combo-box should scroll 1 item per notch. Tested on multiple OE + Windows versions.

Hynek: Please check 11071a/rev. 16726.

Code review 11071a revisions 16725..16726

The points below look legit.

• [MAJOR] functional ScrollPaneGuiImpl.scrollVerticallyTo: The new if (newPos vBar.getPosition()) return; guard permanently swallows wheel-up in the SIMPLE combo-box whenever vBar's cached position has drifted below the list's real top row, and there is a live user path that creates exactly that drift. Trigger, with the issue's own reproducer (9 items, SIZE 20 BY 3 ⇒ visibleRows 2): type item2 into the combo's entry field. EntryFieldGuiImpl.processKeyEvent (SE_VALUE_CHANGED) → ComboBoxGuiImpl.entryFieldChanged (:1481) → ScrollableSelectionListGuiImpl.setTopRow(1) (:548-552), which first calls ensureRowVisible(1, true) → DefaultList.setCurrentRow(1, true): there calculateScrollYPosition() returns rowTop 0 both before and after, so pos newPos and postScrollEvent is not called — no listener is notified and vBar.position stays 0. setTopRow then assigns rowTop = fixTopRow(1) = 1 directly, again with no notification. Derived state: rowTop 1, vBar.getPosition() 0. Nothing resyncs the bar upward from the widget — adjustScrollLayoutImpl (:1016-1158) snapshots vBar.getPosition() and adjustScrollBar (:1263-1266) only clamps down (if (bar.getPosition() > max); 0 > 7 is false), after which :1141-1149 nulls newYPosition because the bar did not move, so getScrollWidget().scroll(...) is never called even though draw() runs it on every repaint; ScrollableSelectionListGuiImpl.draw (:266-270) only self-corrects when rowTop + rowsVisible > modelSize (1 + 2 > 9 is false). One wheel notch up then computes newPos = -1 * 1 + 0 = -1 → clamped to 0 → equals vBar.getPosition() → return. No ScrollEvent is posted, item1 can never be reached by wheeling up, and every further wheel-up is swallowed identically. Before this change the unconditional ScrollEvent(w, null, 0) reached ScrollableSelectionListGuiImpl.scroll(null, 0), which set rowTop = 0 and repainted — i.e. the old post also acted as the de-facto resync. Fix by comparing against the scrolled widget's actual offset (getTopRow()) rather than vBar.getPosition(), or by making ScrollableSelectionListGuiImpl.setTopRow notify its scroll listeners after assigning rowTop. Note the mirror-image drift (bar above the widget, e.g. typing item9) does self-heal via the clamp-down in adjustScrollBar, so only the bar-below-widget direction is affected; no drift source was found for the other users of the shared helper (frames, editors, drop-downs).

• [MINOR] functional ComboBoxGuiImpl.mouseWheelMoved: On the web driver the new step is one item per wheel message, not per physical notch, so high-resolution input over-scrolls a SIMPLE combo relative to the pre-change behavior. e.getWheelRotation() is ±1 by construction on the web driver — GuiWebDriver splits the JS rotation into direction = (wheelParameters[0] > 0) ? 1 : -1 and scrollAmount = Math.abs(wheelParameters[0]) (:1806-1807) and passes direction as the AWT wheelRotation (:1841-1843) — so the gesture magnitude survives only in getScrollAmount(), which scrollBySteps() discards. Trigger: web GUI client, focused SIMPLE combo-box, macOS trackpad / Windows precision touchpad / high-resolution wheel, where Chrome sends deltaMode 0 with many small deltaY values per gesture; each event now scrolls a full list item, whereas the old scrollStepsFromPixels(e.getScrollAmount()) path (CoordinatesConversion:738-748, (int) Math.round(heightFromNative(pixels))) returned 0 steps for deltas below about half a text row (~7px at a typical pixelsPerRow of 15-21) and so ignored them. MouseEvt.mergeWith() sums rotations as well as scroll amounts (:632-633), so queue coalescing provides no damping. Kept at MINOR rather than MAJOR because the effect is limited to viewport over-scroll — scrollBySteps() only posts a ScrollEvent, the selection is untouched, and the excursion is clamped to vBar.getMax() and trivially reversible — and because it is not a new anomaly: SliderGuiImpl (:470), ScrollableListGuiImpl (:399), TreeBodyGuiImpl (:578), EditorGuiImpl (:1441), SelectionListBodyGuiImpl (:552) and ComboBoxGuiImpl's own closed-drop-down branch (:656-687) all already move one unit per web wheel event, so this diff extends an existing codebase-wide web convention to the SIMPLE combo rather than regressing it in isolation. Optional, non-blocking follow-up: reuse the accepted mechanism from BrowseGuiImpl.mouseWheelMoved (:1523-1537), which accumulates Math.abs(e.getScrollAmount()) / 100.0 against ThinClient.getMouseScrollThreshold() behind a gd.isWeb() guard — ideally hoisted into shared code so all rotation-only web widgets benefit at once. A classic discrete notch (deltaY 100, or Firefox deltaMode 1) is unaffected and in fact improved by this diff (was ~6 rows / 3 rows, now 1 item), which is exactly what the issue asks for.

• [MINOR] functional ScrollPaneGuiImpl.mouseWheelMoved: The hBar null && hBar.isVisible() → hBar != null && hBar.isVisible() correction is right and removes a real crash — && evaluates its right operand only when the left is true, i.e. exactly when hBar is null, so the old guard was an unconditional NPE for Bars.VERTICAL-only panes (DropDownGuiImpl:222, ComboBoxGuiImpl:2148, MessageAreaGuiImpl:157) and the body was genuinely dead. But it brings that previously-dead statement to life, and the statement mixes units. scrollStep is w.getVisibleDimension().height, which is native pixels for every pane that can actually have a visible hBar — NativeScrollContainer (:110-120, dimensionToNative(pane.calcInitialViewportSize())), used by the frame scroll container and WindowWorkSpace — whereas hBar.height() returns character rows as a double (ScrollBarGuiImpl.init sets setHeight(cc.heightFromNative(theme.getScrollBarGaugeWidth())), and ScrollBarGuiImpl.height():269 only subtracts the 3D offset). The decisive corroboration is adjustScrollLayoutImpl (:1066-1070), which performs the analogous correction as maxV += screen().coordinates().heightToNative(barSize(Orientation.HORIZONTAL)) — it must convert to pixels; mouseWheelMoved does not. Per JLS 15.26.2 the compound scrollStep -= hBar.height() is (int)(scrollStep - hBar.height()), so a ~0.8-0.9 row value truncates the correction to ~1px: a 200px viewport becomes 199 instead of the intended 184, and the page scroll skips roughly one text row hidden behind the horizontal bar. Trigger: Windows wheel setting "One screen at a time" (Swing WHEEL_BLOCK_SCROLL) or a browser sending deltaMode == 2 (GuiWebDriver:1808-1811), wheeling with focus on an ordinary widget inside a SCROLLABLE frame whose content overflows both ways so both bars are visible — UiUtils.findMouseWheelSource falls through to FrameGuiImpl.getMouseWheelTarget() → frameScroll (created with Bars.BOTH, FrameGuiImpl:2127). Use barSizeNative(Orientation.HORIZONTAL) (:987-1011, returns hBar.physicalDimension().height in pixels) or screen().coordinates().heightToNative(hBar.height()).

Hynek: Please check 11071a/rev. 16727.

All items were valid. For the second one, I removed the cumulative mouse wheel scrolling from browse and moved it at the driver-level for WEB clients. Tested on a laptop trackpad and it seems OK.

Code review 11071a revisions 16725..16727

Please check the major points below, they look legit.

• [MAJOR] functional GuiWebDriver.MOUSE_SCROLL_THRESOLD: The static final constant is initialized from ThinClient.getMouseScrollThreshold() when GuiWebDriver is first loaded. That happens at new GuiWebDriver(config) (WebClientSpawner for the spawned web client, OutputManager.init otherwise), which is before ThinClient.initializePost assigns mouseScrollThreshold = server.getMouseScrollThreshold(0.8). ThinClient.mouseScrollThreshold has no static default, so the constant is always 0.0. As a result, the headStartPixels branch in wheelNotches never runs and the threshold is ignored, including the 0.8 default. The removed BrowseGuiImpl constant was loaded only when the first browse was created, after initializePost, so it did get 0.8. Trigger: a pixel-mode device reporting less than 100 px per notch, e.g. the 85 px case from the code comment, or Chrome/Edge at 110% page zoom (about 90 px). The first notch of the session and the first notch after every change of direction does nothing over a browse, SIMPLE combo, editor or selection list. Read the threshold lazily, at the first wheel event, instead of in a static initializer.

• [MAJOR] functional GuiWebDriver.wheelNotches: The driver assumes a fixed 100 px per notch (WHEEL_NOTCH_PIXELS). Chromium-based browsers on Windows report pixel deltaY = Windows "lines per notch" x 100/3 px, divided by the page zoom, and writeInt32BinaryMessage then truncates it. So the rotation produced here does not count physical notches. This contradicts the agreed #11071 target (1 item per notch regardless of the Windows setting) for ComboBoxGuiImpl.mouseWheelMoved in SIMPLE mode (scrollBySteps(e.getWheelRotation())). Compared to trunk, it also changes the consumers that use only the rotation; these used to get +/-1 per message, which matches the Swing semantics where the lines setting lives in getScrollAmount().
  • 6 lines per notch (deltaY = 200, rotation 2 per notch): the SIMPLE combo moves 2 items per notch, the closed DROP-DOWN-LIST (oldIdx + scroll) skips every other value, and the browse, tree and ScrollableListGuiImpl double their step.
  • 1 line per notch (33 px): the SIMPLE combo and the other rotation consumers move once every 3 to 4 notches.
  • Page zoom 90% (111 px): about every 9th notch moves 2 items. Zoom 110% (90 px): about every 10th notch does nothing. Zoom 125% (80 px): every 5th notch does nothing.
  • Rev. 16726 (+/-1 per message) met the target for a classic wheel at any lines setting; 16727 meets it only at 3 lines and 100% zoom. Firefox is unaffected because it stays in line mode.
  • The rotation should not be derived from a fixed pixel distance per notch. The browser does not expose a notch count in pixel mode, so this needs a different approach, e.g. counting each discrete classic-wheel event as one notch and accumulating only for smooth-scrolling devices.

• [MAJOR] functional ComboBoxGuiImpl.createSimpleList: simpleList.set(idx) / simpleList.setTopRow(idx) run before simpleListPane.setScrollBars(Bars.VERTICAL). The new postScrollEvent(null, rowTop) in setTopRow therefore reaches ScrollPaneGuiImpl.handleScrollEvent while vBar is still null, so only the list is scrolled. The bar is then created at position 0, and neither createScrollBar nor adjustScrollBar/adjustScrollLayoutImpl syncs it with rowTop (adjustScrollBar only clamps when position > max). No later value change calls setTopRow again, so the drift persists until the user wheels down once. Trigger: the #11071 repro with INITIAL "item7" (idx = 6, fixTopRow(6) = 6). The list shows item7 at the top while vBar.position is 0. The first wheel-up is swallowed by the new newPos == vBar.getPosition() guard in scrollVerticallyTo, and wheel-down jumps back to item2. Before the branch, the stale wheel-up at least jumped to item1. Create the scroll bars before set/setTopRow, or (more robust, it also covers the shiftCurrentRow finding) base scrollBySteps and the no-op guard on the list's real top row (getTopRow()) instead of vBar.getPosition().

• [MINOR] style GuiWebDriver.MOUSE_SCROLL_THRESOLD: Typo in the identifier ("THRESOLD"); since the constant is moved into new code, fix the name as part of the move.

• [MINOR] style GuiWebDriver.wheelNotches: The local direction shadows the direction variable the caller (raiseMouseEvent) assigns from the result, which hurts readability. The comment also mixes the "threshold" and "head start" terms.