Focus issue

[Maxoudela]

Hi guys,

I have an issue with the requestFocus method applied on a LocalDateTextField.

Currently, we have a very nice thing in CalendarTextFieldSkin which detects when the focus is given to the CalendarTextField and it gives it to the TextField :

`/**
     * When the control is focus, forward the focus to the textfield
     */
    private void initFocusSimulation() 
    {
        getSkinnable().focusedProperty().addListener( (observableValue, wasFocused, isFocused) -> {
            if (isFocused) {
                Platform.runLater( () -> {
                    textField.requestFocus();
                });
            }
        });
    }`

(Question aside, why the Platform.runLater ?)

But if you use a LocaleDateTextField which is another Control, there is no binding on this focus. Therefore calling "requestFocus" on a LocaleDateTextField will give the focus, but the actual TextField will not have it.

My quick solution is to add in LocalDateTextFieldSkin, where all the bindings are done, a listener like that :

getSkinnable().focusedProperty().addListener((observableValue, wasFocused, isFocused) -> {
                if (isFocused) {
                    calendarTextField.requestFocus();
                }
            });

It works. But I find it a bit heavy. I have the focus, so I give it to the CalendarTextField which gives it to the TextField itself.. But maybe it's the only solution.

[tbee]

Well, reusing the CalendarTextField in the LocalDate*TextField has saved me a lot of duplicate code (and corresponding maintenance), but it comes at a price that you have such forwarding code.

I'm not sure why the runLater is there; apparently it was needed to make it work :-)

Would you mind creating a PR for this change, this time with a test?

[Maxoudela]

Yes I'll do it as soon as possible!

[Maxoudela]

I'm working on the PR and doing some tests on my application.

I came across this bug because I want to put the editor inside a TableCell when edition is requested on a TableView. What I do is that I request the focus and do a "select all" inside the Textfield. Therefore, if the user is double clicking on a cell, he can start typing directly and the text is erased by what he's typing.

Also if the user is typing directly into a cell not in edition, I manually start the edition. But because the runLater is there, the focus is not given immediately, and the user's input is gone. Can I remove this runLater in the code? There is no comment explaining why it was there in the first place..

Also, do you have any idea how to make public the "selectAll" method of the TextField through the LocalDateTextField?

[tbee]

Apparently the runLater was needed, otherwise it would not be there. A comment would have been good, shame on me. That said, the original code was pretty darn old, back to the prerelease of JavaFX 2.0, so chances are focus handling has changed. You may remove the runLater if all the test keep running.

The selectAll, because the control is named "TextField" it is allowed to assume that its skin will implement a TextField, so the selectAll method can be added to the Calendar/LocalDateTextField control and forwarded to the skin. For this I usually create an interface and have the control assume all skins have implemented that.

[Maxoudela]

Allright, I'll run the tests but I don't think the runLater is needed.

How would you forward the selectAll()? You mean creating an Interface for the skin containing like : public void selectAll();

And the Calendar/LocalDateTextField will have :

public void selectAll(){
((MySkin)getSkin()).selectAll();
}

Something like that..?

[tbee]

Exactly like that. The interface requires what the skins must implement. But this is only applicable for the CalendarTextField skin(s), the LocalDateTextField will forward the selectAll at control level (just like the focus change).

[Maxoudela]

But the focus change is forwarded in the LocalDateTextFieldSkin .

So the selectAll method in the LocalDateTextField will call the selectAll of the LocalDateTextFieldSkin, which will call selectAll of the CalendarTextField control?

[tbee]

Yes, CalendarTextField will have a selectAll(). LocalDateTextFieldSkin uses a CalendarTextField control without addressing its skin directly, so it should call the selectAll() on CalendarTextField.

Please also add the selectAll on the other *TextField controls?

[Maxoudela]

Allright I think I got it. Yes I will add this method on other controls and some tests as soon as possible.

[Maxoudela]

Well without modifying any files, I have some test failing.. Is the test "slideStep15" of CalendarTimePickerTest of JfXtras Controls succeeding on your side?

[tbee]

Do you use a mac?

[Maxoudela]

Nope I have a Windows..

Le 21 nov. 2016 20:30, "Tom Eugelink" notifications@github.com a écrit :

Do you use a mac?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JFXtras/jfxtras/issues/73#issuecomment-262041662, or mute the thread https://github.com/notifications/unsubscribe-auth/AIi-baN5duC-HAYINTCxgl_seRtotOVJks5rAfFegaJpZM4Ku8o_ .

[tbee]

That is strange (so have I). Yes, last time I ran the tests, a week ago or so, all ran ok.

[tbee]

I see it failing now as well.

[tbee]

My bad! Fixed it.

[Maxoudela]

Allright, I've commited the things on my repo. Now I have (again!) another issue !

When the popup is shown, and you press escape, the focus is sent to the next element. That annoys me and it doesn't feel right. Maybe you clicked on the calendar (or maybe it was opened by the code) but you actually want to do some input with the keyboard. Thus you press escape, and you want to type something.

But since the focus is away, you have to manually click inside the TextField to input something. I think putting the focus inside the TextField after the popup is hidden is better for keyboard user experience.

Any thought on that?

The test is ready (all other tests are running with the modification )and should be looking like that :

@Test
    public void inputAfterEscape() {
        // open the popup
        TestUtil.runThenWaitForPaintPulse(() -> {
            calendarTextField.setPickerShowing(true);
        });
        type(KeyCode.ESCAPE);

        type(KeyCode.NUMPAD2);

        //TextField should be focused
        Assert.assertTrue(find(".text-field").isFocused());

        Assert.assertEquals("2", ((TextField) find(".text-field")).getText());
    }

I have to look a bit deeper because the CalendarTimeTextFieldSkin doesn't have a setOnHiding method like the CalendarTextFieldSkin, but I think it should also have.

[tbee]

Apparently you are the first one to put the control through its paces like this. Thank you for that. I'll get back to you.

[Maxoudela]

Also on a side note, what is "getText()" supposed to give in CalendarTextField? Should it return the exact same thing as "getText()" on the TextField?

Because apparently, it's updated only when a value is set, or when we parse the Text. But we don't parse it each time a key is typed. So since the documentation is a bit light, I'm wondering if this "textProperty" should reflect the "textProperty" of the Textfield, or it has another purpose that I'm missing.

EDIT : I'm sorry to ask so many questions, but I'm using these calendars in very specific mode where everything is important.. I don't want to fork the project and do things on my side. I really want to give my feedback on real applications in order to improve these controls for other people. And like always, I'm able to provide pull requests for every demand I make.

[tbee]

You're question are highly appreciated! Yes, getText should return what getText on textfield does.

[Maxoudela]

Allright, I'll try to modify the management of this textProperty and add some tests.

[Maxoudela]

I've opened a separate issue for the getText issue. Waiting on your opinion regarding the focus on popup hiding. When you have some spare time of course ;)