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

[Alex Macdonald]

On Wed, 28 Feb 2024 11:28:15 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

Left a few minor things in the comments. Overall looks good.

I'm not sure if anyone else is running Linux, but my tab navigation is still stuck if the Outline tab is opened in JMC, regardless of the flameview being open or not. Would be nice to see if this is just me or not, otherwise this issue could be updated to be (mainly) Windows-focused. Scenario 2 looks to be fixed across all platforms though, which is nice.

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

> 601: 		Display.getDefault().syncExec(new Runnable() {
> 602: 			public void run() {
> 603: 				IWorkbenchPage activePage = PlatformUI.getWorkbench().getWorkbenchWindows()[0].getActivePage();

Minor syntax nit, but I think this could be replaced with `PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage()`  ?

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

> 620: 	 * Adding key listener to transfer focus forward or backward based on 'TAB' or 'Shift + TAB'
> 621: 	 * 
> 622: 	 * @param comp

This and the following 3 methods have a `@param` but no extra details, I think maybe either remove it or just add what comp is.

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

> 833: 			var fileName = fd.getFileName().toLowerCase();
> 834: 			// FIXME: FileDialog filterIndex returns -1
> 835: 			// (https://bugs.eclipse.org/bugs/show_bug.cgi?id=546256)

As a side note, it would be nice to see if this is still an issue. The bug mentions that it was not an Eclipse issue, but rather a gtk 3 issue from 2019.

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

PR Review: https://git.openjdk.org/jmc/pull/554#pullrequestreview-1911313521
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1509102706
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1509108639
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1509111971