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.