FWD » Core Development » User Interface

Hynek/Vladimir: I'm going to need both of you to carefully review 11211a/rev. 16438 as these are some "risky" changes (i.e., the whole mouse wheel source detection is refactored).

• Web: In p2j.screen.js -> raiseMouseEvent() there is this code:
  

  if (me.hasOsEvent(evt, wThis, mouseOp, wid))
  {
     // high-level event, do not process here
     return;
  }

  
  Do we need to also check this on the client? If so, is it enough to verify if the actions of the widget have at least 1 OS event, such as COPY, CUT, PASTE, etc... ?
• Swing: In MouseHandler.mouseWheelMoved() the window of the mouse target can actually be different from the window returned by ews (i.e., when the focus is on the window and we are scrolling with the mouse pointer being on another window). Should we post the event on the window of the target or the window of the mouse pointer? I guess changing the ews does not make sense as scrolling on another window does not have any effect on it in 4GL.

• Non-legacy widgets can be scrolled, such as the window message area
• A mouse wheel event does have effect on the widgets from a non-sensitive (blocked) window

Aaaand, another one. If it's alright, I would like both Alex and Serban to look at the changes. Thank you!

Code review 11211a revisions 16437..16438

• [CRITICAL] functional EntryFieldGuiImpl.mouseActions: The override returns only { MouseEvent.MOUSE_WHEEL }, completely replacing the parent FillInGuiImpl.mouseActions(). The fix should append MOUSE_WHEEL to the parent's array rather than replacing it.
• [MINOR] style UiUtils (imports): Import changed from wildcard import com.goldencode.p2j.ui.client.event.listener.* to explicit import ...ActionListener. Per GoldenCode convention, wildcard imports should be used unless conflicts require explicit class imports.
• [MINOR] style UiUtils.findMouseWheelSource: The Javadoc for the hasShift parameter says {@code false} but should say {@code true}, matching the hasCTRL parameter pattern. Copy-paste error.
• [MINOR] style MouseEvt (imports): Added import com.goldencode.p2j.ui.Keyboard does not appear to be used in the changed code within MouseEvt.java.
• [MINOR] style p2j.screen.js.raiseMouseEvent: Variable wid is declared with var twice - once in the outer scope and again inside the if block. The inner var keyword is redundant and should be removed.
• [MINOR] functional ScrollPaneGuiImpl.mouseActions: In the first branch (when child is ScrollableSelectionListGuiImpl), MouseEvent.MOUSE_WHEEL was already present in the array before this change. The diff appends a second duplicate entry. The other two branches correctly add MOUSE_WHEEL for the first time. Harmless at runtime because the array is consumed via RoaringBitmap which deduplicates, but the duplicate is clearly unintentional and should be removed.

In the following chunk window() is likely not correct. It may return a different top-level window from the one source is located in. Use topLevelWindow instead.

+         int widgetId = ((Widget<?>) source).getId().asInt();
+         int windowId = ((Widget<?>) source).window().getId().asInt();

Rebased 11211a to trunk rev. 16514.
Please check rev. 16516.

Code review 11211a revisions 16514..16516

• [CRITICAL] functional TreeBodyGuiImpl.canHandleMouseWheel: TreeBodyGuiImpl implements mouseWheelMoved() that scrolls the tree (via tree.hModel/tree.vModel.increment) and registers MOUSE_WHEEL in its mouseActions(), yet does not override canHandleMouseWheel() to return true. After the new UiUtils.findMouseWheelSource() routing, when a tree body is focused the returned target inherits the default false from AbstractWidget, causing wheel events to be rerouted to the parent frame's scroll pane rather than reaching the tree. This breaks TREEVIEW/TREELIST wheel scrolling. Please check this case.

• [MAJOR] functional FrameGuiImpl.canHandleMouseWheel: FrameGuiImpl overrides canHandleMouseWheel() to return true but neither FrameGuiImpl nor Frame implements mouseWheelMoved(). In findMouseWheelSource() the branch routeToFrame |= !(target instanceof FrameGuiImpl) && (hasShift || hasCTRL) is specifically designed so that when the target is already a frame the scroll pane is NOT substituted. Consequently a wheel event whose resolved target is the frame itself is delivered to the frame and dropped by AbstractWidget.mouseWheelMoved() (no-op). Either route through frameScroll or implement mouseWheelMoved() in FrameGuiImpl delegating to frameScroll. Is this case an issue? For example when a frame itself s focused?

• [MAJOR] functional EntryFieldGuiImpl.canHandleMouseWheel: Returning true unconditionally is inconsistent with actual wheel handling: EntryFieldGuiImpl.mouseWheelMoved() only forwards to comboBoxRef when non-null and otherwise does nothing. For a standalone fill-in (not a combo-box editor), findMouseWheelSource() returns the fill-in as target (since it "handles" the wheel), so the mouse wheel is consumed without scrolling the parent frame. Consider returning comboBoxRef != null here, or letting routing fall through to the parent frame for plain fill-ins. Is this an issue? Does scrolling work correctly when comboBoxRef == null?

• [MAJOR] functional ScrollBarGuiImpl.canHandleMouseWheel: ScrollBarGuiImpl implements a substantive mouseWheelMoved() (computing scroll step and calling onButtonClick) and AbstractScrollBar lists MOUSE_WHEEL in its MOUSE_ACTIONS, but neither class overrides canHandleMouseWheel(). Is this expected? At least at the source code level this looks suspicious.

• [MAJOR] functional MouseHandler.mouseWheelMoved: ((Widget) source).topLevelWindow().getId().asInt() is unguarded. AbstractWidget.topLevelWindow() is documented to return null when the widget is detached during creation/destruction lifecycle transitions. A race where findMouseWheelSource returns a widget that is just detached will NPE before the event is posted.

• [MAJOR] functional GuiWebDriver.raiseMouseEvent: Same NPE risk as MouseHandler.mouseWheelMoved: source.topLevelWindow().getId().asInt() can throw NPE when topLevelWindow() returns null for a widget that was just detached from its window hierarchy.

• [MAJOR] functional SwingMouseHandler.mouseWheelMoved: Behavioral change from the previous implementation. Previously, when no source was found (processAction returning false with null source), control fell through to SwingMouseHandler.super.mouseWheelMoved(e), which still posted the event via tc.postMouseEvent(e, ews.getWindowId()). The new code returns silently and posts nothing, dropping wheel events that cannot be attributed to a specific widget (e.g., wheel scrolls over a region with no focused non-legacy widget). Any window/frame-level wheel handling that relied on receiving events without a resolved source will break. Please check this case.

• [MAJOR] functional Screen.canProcessOsEvent (p2j.screen.js): Allowing WheelEvent to bypass win.processOsEvents is a real semantics change. processOsEvents = false is set by WindowManager.modalizeWindow() to silence non-active windows while a modal dialog is displayed. The existing !p2j.isDialogDisplayed() guard only detects internal JS dialogs, not 4GL modal windows. Consequently, wheel events will now be processed on silenced windows behind a modal 4GL window, which is unwanted. Please check this case. I think it is enough to test mouse wheels work the same in dialog-box as non-modal top-level windows, also that mouse wheels are not sent to the non-modal windows while modal is active.

• [MAJOR] style p2j.screen.js (file header): Erroneous duplicate text in the history table between entries 098 and 099 (** Added a new method updateWindowState to update the window state. Refs #9973a.); this text belongs to the earlier 096 IMS entry and must not be repeated.

• [MINOR] functional UiUtils.findMouseWheelSource: The browse-edit-mode branch returns null if browse.getActiveEditor() returns null. If editor was dismissed between focus read and this call. I'm not sure this case is possible, but a fallback to scroll some of the parent containers wouldn't hurt.

• [MINOR] functional UiUtils.findMouseWheelSource: Does not check whether the focused widget is disabled to trigger the fallback path (as per Redmine summary). The only isEnabled() check is inside the browse-editor branch; a disabled non-browse focused widget is still used normally. Please check.

• [MINOR] functional UiUtils.findMouseWheelSource: The initial loop while (target != null && target.getId() == null) target = target.parent() escalates focus to a non-focus ancestor when the focused widget is a purely client-side (id-less) sub-widget (dropdown item, calendar cell, tree editor subcomponent). This can mis-route the wheel event to an ancestor rather than letting the real sub-widget's own wheel handling run. This can be confirmed by wheel scrolling a virtual widget.

• [MINOR] functional UiUtils.findMouseWheelSource: locateFrame(target) returns the innermost ancestor frame. For nested frames where the inner frame's scroll pane has no visible scrollbar but the outer one does, ScrollPaneGuiImpl.mouseWheelMoved() returns early and the event never bubbles. If scroll-bubbling across nested frames is a supported scenario, try the parent frame when the inner one cannot scroll. Please check this case.

• [MINOR] functional Frame.getScrollPane: Declared return type is AbstractContainer<O>, but the field frameScroll is typed ScrollPane<O>. Returning the broader type loses information and forces callers to use Widget-level API only. Prefer ScrollPane<O>.

• [MINOR] functional GuiWebDriver.raiseMouseEvent: After the new wheel block remaps windowId to the focused widget's top-level window, subsequent code uses direct.getWindowEmulatorSafe(windowId).getWindowLocation() for screen-coordinate computation while mouseX/mouseY still come from the original cursor window. The resulting MouseWheelEvent's XOnScreen/YOnScreen are inconsistent (computed relative to a different window than the one producing the raw coordinates). Wheel handlers relying on screen coordinates see incorrect values.

• [MINOR] functional SwingMouseHandler.mouseWheelMoved: e.setSource(mouseSource) is placed inside the !processAction(...) branch. However, processAction(mouseSource, e) invokes action.mouseWheelMoved((MouseWheelEvent) e) while e.getSource() still points to the placeholder. If any registered MouseWidgetAction inspects e.getSource(), it will see the placeholder rather than the resolved widget. Setting the source before processAction yields more consistent semantics.

• [MINOR] functional MouseEvt.BOGUS_AWT_COMPONENT: Duplicates the private BOGUS_AWT_COMPONENT already defined in GuiWebDriver (line 420). Consolidate to a single public definition to eliminate duplication.

• [MINOR] performance MouseEvt (static init): BOGUS_AWT_COMPONENT = new Component() { } forces eager AWT/Toolkit initialization (native peer infrastructure) in environments that otherwise would not need it. Using a lighter stand-in or reusing the existing one in GuiWebDriver would avoid this.

• [MINOR] style UiUtils.findMouseWheelSource: Inconsistent parameter naming - hasCTRL uses all-caps acronym while hasShift does not. Choose one style consistently.

Wheel events are now sent to Java driver uncoditionally. Do we still need to declare MOUSE_WHEEL in mouseActions?

Hynek Cihlar wrote:

• [MAJOR] functional SwingMouseHandler.mouseWheelMoved: Behavioral change from the previous implementation. Previously, when no source was found (processAction returning false with null source), control fell through to SwingMouseHandler.super.mouseWheelMoved(e), which still posted the event via tc.postMouseEvent(e, ews.getWindowId()). The new code returns silently and posts nothing, dropping wheel events that cannot be attributed to a specific widget (e.g., wheel scrolls over a region with no focused non-legacy widget). Any window/frame-level wheel handling that relied on receiving events without a resolved source will break. Please check this case.

I could not identify any places where the logic relied on receiving events without a resolved source.

• [MAJOR] functional Screen.canProcessOsEvent (p2j.screen.js): Allowing WheelEvent to bypass win.processOsEvents is a real semantics change. processOsEvents = false is set by WindowManager.modalizeWindow() to silence non-active windows while a modal dialog is displayed. The existing !p2j.isDialogDisplayed() guard only detects internal JS dialogs, not 4GL modal windows. Consequently, wheel events will now be processed on silenced windows behind a modal 4GL window, which is unwanted. Please check this case. I think it is enough to test mouse wheels work the same in dialog-box as non-modal top-level windows, also that mouse wheels are not sent to the non-modal windows while modal is active.

A modal window is guaranteed to hold the focus until it is closed, so the final target can never really be a widget from a silenced window.
The modification was made for the situation where you have a non-modal and a modal window visible and you scroll with the mouse pointer being on the silenced (non-modal) window. We need to propagate this event to the client to be redirected to a widget from the modal window (since it's guaranteed it has focus).

• [MINOR] functional UiUtils.findMouseWheelSource: The initial loop while (target != null && target.getId() == null) target = target.parent() escalates focus to a non-focus ancestor when the focused widget is a purely client-side (id-less) sub-widget (dropdown item, calendar cell, tree editor subcomponent). This can mis-route the wheel event to an ancestor rather than letting the real sub-widget's own wheel handling run. This can be confirmed by wheel scrolling a virtual widget.

• public static void handleCursorRestore(int widgetId) from MouseHandler
• public void raiseMouseEvent(MouseEvent event, int windowId, int widgetId) from WebMouseHandler

• [MINOR] functional UiUtils.findMouseWheelSource: locateFrame(target) returns the innermost ancestor frame. For nested frames where the inner frame's scroll pane has no visible scrollbar but the outer one does, ScrollPaneGuiImpl.mouseWheelMoved() returns early and the event never bubbles. If scroll-bubbling across nested frames is a supported scenario, try the parent frame when the inner one cannot scroll. Please check this case.

In the new revision, locateFrame can not be called for frames. In addition, the maximum level of mouse wheel propagation should be only to widget's frame and not further up the stack.

Wheel events are now sent to Java driver uncoditionally. Do we still need to declare MOUSE_WHEEL in mouseActions?

• Some JS logic may depend on a widget having the MOUSE_WHEEL action
• This action is still checked in GuiWebDriver.raiseMouseEvent():
  

  RoaringBitmap actions = getWebActions(source);
  // Check if the widget can process MOUSE_WHEEL on Web.
  if (!actions.contains(mop.code))
  {
     return;
  }

Refactored the whole UiUtils.findMouseWheelSource() to take a more clean approach. Renamed canHandleMouseWheel() to getMouseWheelTarget(). The widgets that implement this method will need to return the delegated widget to which the mouse wheel event should be forwarded.

Please check 11211a/rev. 16517.

Code review 11211a revisions 16514..16517

Two minor points, otherwise the changes look good. The functional point is worth checking. ChUI regression testing will be needed - while the changes are related only to GUI there are also common code changes.

• [MINOR] functional GuiWebDriver.raiseMouseEvent: For wheel events, widgetId is overwritten with the wheel-source widget id (source.getId().asInt()) before MouseHandler.handleCursorRestore(widgetId) runs. handleCursorRestore resolves the cursor from the hovered widget's getEffectiveMousePointer(), but for wheel events the source resolved via UiUtils.findMouseWheelSource is focus-derived and may differ from the hovered widget. Trigger: a 4GL SET-POINTER has set a global User32 cursor, focus is on widget A while the mouse hovers a different widget B, and the user wheel-scrolls — handleCursorRestore then uses A's effective pointer instead of B's. The mismatch self-corrects on the next MOUSE_MOVED. Consider preserving the original JS-supplied widgetId for the handleCursorRestore call, or skipping the cursor-restore step for the wheel branch.

• [MINOR] style ScrollBarGuiImpl: History entry 039 contains a typo: "Overidden" should be "Overridden".

Please check 11211a/rev. 16518.

Please merge 11211a to trunk now.