focus handling using JToolBar is not correct

tbee commented 3 years ago

If you have a button in a JToolBar then normally when you click that button the focus should be pulled away from any editing component. This does not happen. This results in situations where a user clicks on "save" and the edit is not committed prior to the save action.

Java 15, FlatLAF 1.3.

Below a small application demonstrating this. Just click on the toolbar button and see if the editing textfield or table is committed. Behavior is different from when the button at the bottom is clicked.

import java.awt.BorderLayout;
import java.awt.KeyboardFocusManager;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JTextField;
import javax.swing.JToolBar;
import javax.swing.SwingUtilities;
import javax.swing.UIManager;

public class Test {

    public static void main(String[] args) throws Exception {
        KeyboardFocusManager.getCurrentKeyboardFocusManager().addPropertyChangeListener("focusOwner", evt -> {
            System.out.println(evt.getOldValue() + " -> " + evt.getNewValue());
        });

        SwingUtilities.invokeAndWait(() -> new Test().swing());
    }

    private void swing() {
        try {
            UIManager.setLookAndFeel(com.formdev.flatlaf.FlatLightLaf.class.getName());
        } 
        catch (Exception e) {
            e.printStackTrace();
        }

        JFrame jFrame = new JFrame();
        jFrame.setLayout(new BorderLayout());

         jFrame.add(new JTextField(20), BorderLayout.CENTER);
//      TableModel dataModel = new AbstractTableModel() {
//          public int getColumnCount() {
//              return 10;
//          }
//
//          public int getRowCount() {
//              return 10;
//          }
//
//          public Object getValueAt(int row, int col) {
//              return Integer.valueOf(row * col);
//          }
//
//          public boolean isCellEditable(int row, int col) {
//              return true;
//          }
//
//          @Override
//          public void setValueAt(Object value, int row, int col) {
//              System.out.println("setValueAt " + value);
//          }
//      };
//      JTable jTable = new JTable(dataModel);
//      jTable.putClientProperty("terminateEditOnFocusLost", Boolean.TRUE);
//      jFrame.add(new JScrollPane(jTable), BorderLayout.CENTER);

        jFrame.add(newButton("south"), BorderLayout.SOUTH);

        JToolBar jToolBar = new JToolBar();
        jToolBar.add(newButton("toolbar"));
        jFrame.add(jToolBar, BorderLayout.NORTH);

        jFrame.pack();
        jFrame.setLocationByPlatform(true);
        jFrame.setVisible(true);
    }

    private JButton newButton(String label) {
        JButton jButton = new JButton(label);
        jButton.addActionListener((e) -> System.err.println("clicked " + label));
        return jButton;
    }
}

DevCharly commented 3 years ago

Toolbar buttons in FlatLaf are not focusable:

https://github.com/JFormDesigner/FlatLaf/blob/c708205593dc1161cda194bd2247976dbced4c09/flatlaf-core/src/main/java/com/formdev/flatlaf/ui/FlatToolBarUI.java#L57-L63

The reason for this is that in modern applications, the toolbar is not focusable these days. Try Windows Explorer, macOS apps, Thunderbird, Crome, IntelliJ, Eclipse, etc.

One exception is Firefox where you use Tab key to move focus into toolbar and then use arrow keys to navigate focus within toolbar. But clicking on a toolbar button in Firefox, does not focus it!

Currently there is no way to change this in FlatLaf, but I'll make this configurable.

A workaround is to invoke setFocusable( true ) on the button after adding it to the toolbar:

JButton button = new JButton( "save" );
toolbar.add( button );
button.setFocusable( true );

But currently no focus indicator is painted for FlatLaf toolbar buttons. I'll add this for 1.4.

Anyway, IMHO you should not depend on focus events in your save method. If the user presses Ctrl+S there is also no focus event. Or if you have a menubar and the user selects "Save" from the menubar, the table will not loose permanent focus and does not commit.

Better explicitly commit. E.g. when getting value from table.

tbee commented 3 years ago

You are right about the focus behavior, and this is actually a bigger problem; if the user is editing some field and then triggers a save action -being it via the toolbar, menu, or keyboard shortcut- he expects the edit to be committed prior to the save action being executed. Which I feel is a perfectly sensible expectation, at least I expect this. Consider Excel where the edit in a cell is not committed when pressing ctrl+s. I figure one could scan the whole component tree for components still in edit mode... Hm.

The focus lost specifically for tables is widely recognized. It is something that is haunting the table in JavaFX, and in Swing has is a special property that can be set on JTable to make sure commit-on-focus-lost is done (table.putClientProperty("terminateEditOnFocusLost", Boolean.TRUE)).

Thank you for making it configurable. However, IMHO, the default behavior of FlatLaF toolbar is a breaking change to the expected behavior, every other LaF moves the focus upon button click. So I would suggest making the default to move the focus :-).

DevCharly commented 3 years ago

FlatLaf 1.4 is now out and you can reenable focusable toolbar buttons with:

UIManager.put( "ToolBar.focusableButtons", true );

The default value is still false, but I consider to change this in a future release.

Have also implemented a focus traversal policy for the toolbar, which enables Firefox style traversal: (but this is not yet in the git repo)

arrow keys for navigation within a toolbar
Tab key moves focus always out of toolbar

This makes the "tab" navigation easier because it is no longer press Tab key 20 times to skip a toolbar with 20 buttons...

tbee commented 3 years ago

Great! I'll give this a try soon!

DevCharly commented 2 years ago

FlatLaf 2.0-rc1 contains some toolbar improvements.

Arrow-keys-only navigation

If UI property ToolBar.arrowKeysOnlyNavigation is true (the default) and ToolBar.focusableButtons is changed to true (default is still false) then navigation works as follows:

if a toolbar button has focus, then arrow keys move focus within toolbar
tab-key moves focus out of toolbar
if moving focus into the toolbar, recently focused toolbar button is focused

This is the same navigation as used in Firefox.

Floatable

Toolbars are no longer floatable by default (dots on left side of toolbar that allows dragging toolbar). Modern applications do not use floatable toolbars these days. Set UI property ToolBar.floatable to true if you want the old behavior.

Docs

The UI properties are documented here: https://www.formdev.com/flatlaf/components/toolbar/

JFormDesigner / FlatLaf