Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8268225: Support :focus-visible and :focus-within CSS pseudoclasses #475

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

@mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Apr 21, 2021

This PR adds the Node.focusVisible and Node.focusWithin properties, as well as the corresponding :focus-visible and :focus-within CSS pseudo-classes.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

  • JDK-8268225: Support :focus-visible and :focus-within CSS pseudoclasses

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/475/head:pull/475
$ git checkout pull/475

Update a local copy of the PR:
$ git checkout pull/475
$ git pull https://git.openjdk.java.net/jfx pull/475/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 475

View PR using the GUI difftool:
$ git pr show -t 475

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/475.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 21, 2021

👋 Welcome back mstr2! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@Maran23
Copy link
Member

@Maran23 Maran23 commented Apr 28, 2021

Do you have a example application or something, where the possible use case/the advantage of this can be shown? I can't imagine right now, where I may want to use that.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 28, 2021

Do you have a example application or something, where the possible use case/the advantage of this can be shown? I can't imagine right now, where I may want to use that.

If you are running Windows 10, try opening PC Settings, then go to a section like System > Notifications and actions. There are lots of switches and check boxes that you can toggle, but you'll notice that they don't have any visible focus indicator if you click on them (even tough they are focused after clicking). Only if you press the Tab key you'll see a black border as a visual focus indicator, which will disappear again after clicking.

@mstr2 mstr2 changed the title Support :focus-visible CSS pseudoclass 8268225: Support :focus-visible and :focus-within CSS pseudoclasses Jun 7, 2021
@mstr2 mstr2 marked this pull request as ready for review Jun 7, 2021
@openjdk openjdk bot added the rfr label Jun 7, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 7, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jun 7, 2021

I note that the discussion hasn't finished on the openjfx-dev mailing list as to whether to accept this feature. The proposed API and docs in this PR can be used to inform the discussion, but it's premature to start the code review.

If it ends up going forward, it will need a CSR and 2 reviewers.

/csr
/reviewers 2

@openjdk openjdk bot added the csr label Jun 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 7, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@mstr2 please create a CSR request for issue JDK-8268225. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 7, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review Jun 9, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I took an initial pass at the API docs. I have a couple global comments.

First, can you reorder the properties and their associated methods such that they are grouped together? If the docs are on the (private) field, which is what we usually do, it should be followed immediately by the associated setter (if any), getter, and property methods.

The order should be:

    /**
     * EXISTING API DOCS FOR FOCUSED PROPERTY
     */
    private FocusPropertyBase focused;

    protected final void setFocused(boolean value) {

    public final boolean isFocused() {

    public final ReadOnlyBooleanProperty focusedProperty() {

    /**
     * API DOCS FOR NEW FOCUSVISIBLE PROPERTY
     */
    private FocusPropertyBase focusVisible;

    public final boolean isFocusVisible() {

    public final ReadOnlyBooleanProperty focusVisibleProperty() {

    ...

Second, the two new properties need both a @defaultValue and an @since NN tag. You can optimistically use @since 17 or you can use @since 18 (the latter is more likely).

@kevinrushforth kevinrushforth self-requested a review Jun 11, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

The API looks reasonable. I left one question inline.

The implementation will take some time to go through. I doubt that this will make JavaFX 17 (which I alluded to earlier). I'm especially curious about some of the patterns that I see, mostly surrounding the FocusPropertyBase implementation class.

  1. FocusPropertyBase overrides set (which is unusual). The current implementation (of FocusedProperty ) has some custom logic as well, but it didn't try to use the set method of a writable property. Also, you use a writable property and don't override invalidated. Can you say why you made the changes you made?
  2. I'm curious about the approach for notifying listeners. It seems similar to what is currently done for focusedProperty, but there are differences. Also, I'm not sure that the two newly added properties need to handle notifications in the same way as focused. I can think of at least one potential issue. If focus shifts from one node to another in the same parent, you might end up notifying a parent's listeners even though its focusWithin is unchanged.
  3. I think the way you are propagating focusWithin might not work if nodes are added or removed from the scene graph.
  4. Normally a read-only property that needs to be settable by the implementation would use ReadOnlyBooleanWrapper. And then the public property method would call getReadOnlyProperty to return the read-only property to the user. The implementation would access the wrapper and you don't need to cast. You could consider having FocusPropertyBase extend ReadOnlyBooleanWrapper.

One thing to consider would be to keep the existing (internal) class for focused and use a more standard ReadOnlyBooleanWrapper class for the two new ones. But maybe there are some implications I haven't thought of yet.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Jun 26, 2021

  1. FocusPropertyBase overrides set (which is unusual). The current implementation (of FocusedProperty ) has some custom logic as well, but it didn't try to use the set method of a writable property. Also, you use a writable property and don't override invalidated. Can you say why you made the changes you made?

Technically, it doesn't override set.
FocusPropertyBase extends ReadOnlyBooleanPropertyBase, and also defines a new set method. As implemented, setting the value is completely decoupled from notifying listeners, which leads me directly to your next question...

  1. I'm curious about the approach for notifying listeners. It seems similar to what is currently done for focusedProperty, but there are differences. Also, I'm not sure that the two newly added properties need to handle notifications in the same way as focused. I can think of at least one potential issue. If focus shifts from one node to another in the same parent, you might end up notifying a parent's listeners even though its focusWithin is unchanged.

FocusPropertyBase.set just records the new value, and the decision of whether to notify listeners is deferred until after all focus-related properties have been set on the current node and any of its parents. In the scenario you describe, the recorded value of focusWithin is changed twice, but ends up being the same value as before. In this case, FocusPropertyBase.notifyListeners will not fire a change notification.

There's a test for this scenario: FocusTest.testFocusWithinListenerIsNotInvokedIfPropertyDidNotEffectivelyChange.

  1. I think the way you are propagating focusWithin might not work if nodes are added or removed from the scene graph.

Good point, I'll try to come up with some test cases to see if there are any issues.

  1. Normally a read-only property that needs to be settable by the implementation would use ReadOnlyBooleanWrapper. And then the public property method would call getReadOnlyProperty to return the read-only property to the user. The implementation would access the wrapper and you don't need to cast. You could consider having FocusPropertyBase extend ReadOnlyBooleanWrapper.

That could certainly be an option, although it would increase the memory footprint of Node.
I'm thinking instead to eagerly instantiate all three focus-related properties. In this case, the property field could be referenced directly, which would also remove the need for most casts.

I think that having the same deferred-notification property implementation for all three focus-related properties is the best approach, since the implementation guarantees that listeners are only notified after all changes are committed. At least, I see no upside to not guarantee this. Maybe the atomicity guarantee for focused, focusVisible and focusWithin should be included in the documentation.

@kevinrushforth kevinrushforth self-requested a review Jul 7, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Aug 2, 2021

  1. I think the way you are propagating focusWithin might not work if nodes are added or removed from the scene graph.

I've added a test for this case: FocusTest.testFocusStatesAreClearedFromFormerParentsOfFocusedNode.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The API looks good. The docs look good with a couple suggestions.

I'll get back to reviewing the implementation soon. One thing I will note is that the Linux unit test failures are real. I see them on my local Ubuntu 20.04 system, too.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Aug 20, 2021

One thing I will note is that the Linux unit test failures are real.

That was a bit tough to track down, and a good reminder of why global static state should be avoided at all costs.

@kevinrushforth kevinrushforth self-requested a review Aug 23, 2021
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Sep 9, 2021

Just curious: with this in place, would it be possible to use for supporting JDK-8087926 (it's a bit vague, though, at least for me)?

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Sep 9, 2021

Just curious: with this in place, would it be possible to use for supporting JDK-8087926 (it's a bit vague, though, at least for me)?

Yes, :focus-within can be used to select an ancestor of the currently-focused node.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Sep 9, 2021

Just curious: with this in place, would it be possible to use for supporting JDK-8087926 (it's a bit vague, though, at least for me)?

Yes, :focus-within can be used to select an ancestor of the currently-focused node.

cool - thanks :)

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Oct 5, 2021

I've created a CSR for this feature.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Nov 10, 2021

@kleopatra Can you be a reviewer on this PR?

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Nov 11, 2021

@kleopatra Can you be a reviewer on this PR?

css is not my turf, sry

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 11, 2021

@aghaisas and I will review it.

@kevinrushforth kevinrushforth requested a review from aghaisas Nov 11, 2021
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 6, 2021

  1. I think the way you are propagating focusWithin might not work if nodes are added or removed from the scene graph.

I've added a test for this case: FocusTest.testFocusStatesAreClearedFromFormerParentsOfFocusedNode.

might also need a test that verifies the focusWithin of a parent added somewhere above the focused node? hmm .. or maybe not, that would require to re-arrange a complete subtree ..

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 6, 2021

Just some musings (slightly overwhelmed by the magnitude of the suggested change ;) On first look it sounds like this has 2 separated (though related) parts:

  • focus-visible: that's visual sugar (*ducking)
  • focus-within: that might be a game changer (or enhancer) for both internal and external usage

From my current focus - *haha - on cell editing, the latter feels very promising for allowing fine-grained control of defining, implementing and configuring the required, much debated and (until now) unsuccessful tries on "commit-on-focusLost" semantics.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Dec 6, 2021

might also need a test that verifies the focusWithin of a parent added somewhere above the focused node? hmm .. or maybe not, that would require to re-arrange a complete subtree ..

Inserting a parent into a scene graph such that the existing subtree at that position becomes a subtree of the newly inserted parent can't be done as an atomic operation. First, you'll need to remove the subtree at the insertion point from the existing scene graph (otherwise you'll get an exception saying that a node can only appear once in a scene graph). Then you can add the new parent with the removed subtree as its child.

But what happens if the removed subtree contains a focused node? Since we can't know whether the removed subtree will ever be re-attached to the scene graph, we probably shouldn't keep its focus flags set. Moreover, Scene.focusOwner probably also should not refer to a node that is not part of the scene graph anymore.

But that's not what happens:

// Create a focusable rect
var rect = new Rectangle();
rect.setFocusTraversable(true);

// Add the rect to a group and requestFocus on the rect
var root = new Group(rect);
scene.setRoot(root);
rect.requestFocus();

// Now, the rect is focused and it is the focus owner of the scene
assertTrue(rect.isFocused());
assertSame(rect, scene.getFocusOwner());

// Remove the rect from the scene graph
root.getChildren().clear();

// This is what I would now assume to be true (but isn't):
assertFalse(rect.isFocused());  // FAILED: rect.isFocused() == true
assertNotSame(rect, scene.getFocusOwner()); // FAILED: rect == scene.getFocusOwner()

I'm inclined to think that this behavior is a bug.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 7, 2021

thanks for taking a closer look :)

might also need a test that verifies the focusWithin of a parent added somewhere above the focused node? hmm .. or maybe not, that would require to re-arrange a complete subtree ..

Inserting a parent into a scene graph such that the existing subtree at that position becomes a subtree of the newly inserted parent can't be done as an atomic operation. First, you'll need to remove the subtree at the insertion point from the existing scene graph (otherwise you'll get an exception saying that a node can only appear once in a scene graph). Then you can add the new parent with the removed subtree as its child.

yeah that's true, but might happen behind the scenes - we can move a focused node from one parent to another by simply adding it the new.

So I'm not sure I would agree:

I'm inclined to think that this behavior is a bug.

arguable, though :) Below is an example of moving a focused node (button "Moving", the other is just for having something else that might get the focus) between 2 parents by F1: the button keeps its focused state in its new parent (and scene's focusOwner doesn't change) - I think that's what a user would expect also when dragging a focused node around (s/he might not even be aware of the hierarchy). I suspect that changing the behavior would break existing applications - except if it could be done completely transparently.

The example:

private Parent createContent() {
    first = new VBox();
    first.setBackground(new Background(new BackgroundFill(Color.ALICEBLUE, null, null)));
    first.getChildren().add(new Label("label on first"));
    second = new VBox();
    second.setBackground(new Background(new BackgroundFill(Color.LAVENDER, null, null)));
    second.getChildren().add(new Label("label on second"));

    moving = new Button("moving");
    first.getChildren().add(moving);
    Button move = new Button("move");
    move.setOnAction(e -> {
        move();
    });
    move.setDefaultButton(true);
    HBox content = new HBox(10, first, second, move);
    content.addEventFilter(KeyEvent.KEY_PRESSED, e -> {
        if (e.getCode() == KeyCode.F1) {
            move();
        }
    });
    return content;
}

private void move() {
    Parent parent = moving.getParent();
    Pane target = parent == first ? second : first;
    target.getChildren().add(moving);
}

@Override
public void start(Stage stage) throws Exception {
    Scene scene = new Scene(createContent(), 300, 300);
    scene.focusOwnerProperty().addListener((scr, ov, nv) -> {
        System.out.println("focusOwner: " + nv);
    });
    stage.setScene(scene);
    stage.show();
}

private VBox first;
private VBox second;
private Button moving;

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Dec 7, 2021

yeah that's true, but might happen behind the scenes - we can move a focused node from one parent to another by simply adding it the new.

Yes, you're right, I wasn't thinking of that behavior. I think that should be logically seen as an atomic operation, even though it might not be implemented as such. So in this case, Scene.focusOwner wouldn't change. However, I'd still argue that a node that is not part of the scene graph (non-atomically) should not be the focus owner of the scene graph.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Dec 8, 2021

However, I'd still argue that a node that is not part of the scene graph (non-atomically) should not be the focus owner of the scene graph.

I agree :)

But that's what seems to happen: in my example, add a handler to remove the focused "moving" button. When removed, the scene's focus owner is either the next focusable (the "move" button) or null if there is none. On re-adding the button, it's either unfocused (if there had been a next focusable) or focused (if there is none). Not sure what happens in your test snippet ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants