RFR: 8204 : Keyboard traps in flamegraph view for search option [v2]

[Brice Dutheil]

On Fri, 23 Feb 2024 12:14:20 GMT, Virag Purnam <vpurnam at openjdk.org> wrote:

>> Focus Trapped Issue. Keyboard focus is trapped in three specific scenarios mentioned below:
>> 
>> 1. Focus is trapped in JMC Agent Preset Manager
>> 
>> Select Window -> JMC Agent Preset Manager
>> Click on "Import Fils..."/ "Export File" button
>> Press Esc button when dialog is displayed
>> Press "Tab" button for multi times
>> 
>> 2. Focus is trapped in Description of Template Manager
>> 
>> Select JVM Browser
>> Start Flight Recording
>> Template Manager
>> Click New
>> Click Edit
>> Focus to Description
>> 
>> 3. Focus is trapped in search tool in Flame Graph  (Mostly on Windows)
>> 
>> Launch JDK Mission Control
>> Open any JFR file
>> Navigate to "Outline" tab
>> In Java Application, choose "Threads"
>> Move focus to search tool in Flame Graph
>> 
>> Solution:
>> 1. PresetManagerPage.java : Setting the focus back to tableInspector.
>> 2. TemplateEditPage.java : Added TraverseListener() for description field.
>> 3. FlamegraphSwingView.java : Fix is not straight forward as SWT to Swing and back to SWT traversal is creating issue here. I have just implemented work around for this. I have added KeyListener() to all the swing components. Once it traverse through all the components, I am setting the Focus back to SWT.  With this there is no keyboard focus trap. 
>>  
>> These issues are kind of blocker with respect to accessibility. Could someone please review the changes.
>
> Virag Purnam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8204: Keyboard traps in flamegraph view for search option

For the different cases reported:

1. I'm not sure what's the issue, in previous versions, the focus doesn't seem to be trapped. Pressing tab seem to cycle correctly on the buttons of the _JMC Agent Preset Manager_ dialog

2. Looks good

3. I can cycle out of the search text field, but I don't know on which SWT component the focus lands.

Note: I don't know if they are "quirks" on focus traversal depending on the OS, I'm using macOs.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/wizards/PresetManagerPage.java line 209:

> 207: 
> 208: 				tableInspector.getViewer().refresh();
> 209: 				tableInspector.setFocus();

**suggestion:** Might be worth extracting all `setFocus()` statements in a single method with a comment, since the issue is not quite straightforward.

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 184:

> 182: 	private AttributeSelection attributeSelection;
> 183: 	private IToolBarManager toolBar;
> 184: 	private static boolean traverseAlready = false;

**question:** Why this flag needs to be `static` ?

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 519:

> 517: 				flamegraphComponent.setFocusTraversalKeysEnabled(true);
> 518: 
> 519: 				flamegraphComponent.addKeyListener(new KeyAdapter() {

**suggestion:** Add a comment as what is the purpose of this listener, and why it's different from the other.

Maybe extract in method as well.

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 563:

> 561: 			panel.setFocusTraversalKeysEnabled(true);
> 562: 
> 563: 			panel.addKeyListener(new KeyAdapter() {

**suggestion:** Add a comment as what is the purpose of this listener, and why it's different from the other.

Maybe extract in method as well.

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 571:

> 569: 								public void run() {
> 570: 									setTraverseAlready(!traverseAlready);
> 571: 									if (traverseAlready) {

**suggestion:** Having a setter and a direct access to `traverseAlready` is a bit awkward. I suggest to either get rid of the setter, or add a getter.

About this flag, the setter seems to only `toggle`, so if the method is kept I suggest instead to replace the setter by a `void toggleAlreadyTraversed() { traverseAlready = !traverseAlready }`.

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 688:

> 686: 			public void keyPressed(KeyEvent e) {
> 687: 				if (e.getKeyCode() == KeyEvent.VK_TAB) {
> 688: 					if (e.getModifiersEx() > 0) {

**suggestion:** `e.getModifiersEx() > 0` comes up a lot, I believe the idea here is to handle <kbd>Shift</kbd> <kbd>Tab</kbd>, I believe it's the same on all OSes. Might be worth extracting this as util method.

application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 696:

> 694: 				}
> 695: 			}
> 696: 		});

**suggestion:** This looks like the same code as above I suggest extracting this to a method with some comment on the intent.

-------------

PR Review: https://git.openjdk.org/jmc/pull/554#pullrequestreview-1900263605
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502227435
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502287654
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502315103
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502314852
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502311783
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502302815
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1502300184