Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v2]
On Mon, 6 Dec 2021 13:05:16 GMT, Jeanette Winzenburg wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) I added a junit which fails without patch and succeeds with it. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8276170: Add junit for VirtualFlow.scrollToTop(int) - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/a7820448..588a7e41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=656=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=656=01-02 Stats: 37 lines in 1 file changed: 37 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v7]
On Fri, 20 Aug 2021 05:15:49 GMT, Michael Strauß wrote: >> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as >> well as the corresponding `:focus-visible` and `:focus-within` CSS >> pseudo-classes. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > fixed undeterministic test failures 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. - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8268225: Support :focus-visible and :focus-within CSS pseudoclasses [v2]
On Mon, 2 Aug 2021 13:20:29 GMT, Michael Strauß wrote: > > 3. 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 .. - PR: https://git.openjdk.java.net/jfx/pull/475
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 09:23:53 GMT, eduardsdv wrote: >> eduardsdv has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains two additional commits >> since the last revision: >> >> - Merge branch 'openjdk:master' into >> bugfix/JDK-8276167-VirtualFlow-scrollToTop >> - JDK-8276167: fixing VirtualFlow.scrollToTop(int) > > I tested it again. Without the changes in this PR the bug is still there. > If the item is larger than the viewport, the VirtualFlow.scrollToTop(int) > scrolls to the end instead of to top of the item. @eduardsdv This would be a good fix, but can you add a test that fails before and succeeds after this patch? Let me know if you need help with that. Thanks! - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 11:26:24 GMT, Jeanette Winzenburg wrote: >> I tested it again. Without the changes in this PR the bug is still there. >> If the item is larger than the viewport, the VirtualFlow.scrollToTop(int) >> scrolls to the end instead of to top of the item. > >> I tested it again. Without the changes in this PR the bug is still there. >> **If the item is larger than the viewport**, the >> VirtualFlow.scrollToTop(int) scrolls to the end instead of to top of the >> item. > > ahhh .. finally I understand what you mean - bolding of the quote ;) Might be > obvious for everybody except me .. but the bug report is missing that crucial > information ... I agree with @kleopatra . It would be good to have this info in JBS, as well as with a small sample app. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 09:23:53 GMT, eduardsdv wrote: > I tested it again. Without the changes in this PR the bug is still there. > **If the item is larger than the viewport**, the VirtualFlow.scrollToTop(int) > scrolls to the end instead of to top of the item. ahhh .. finally I understand what you mean - bolding of the quote ;) Might be obvious for everybody except me .. but the bug report is missing that crucial information ... - PR: https://git.openjdk.java.net/jfx/pull/656
Integrated: 8276313: ScrollPane scroll delta incorrectly depends on content height
On Tue, 2 Nov 2021 10:49:45 GMT, Michael Strauß wrote: > This PR fixes an issue where the scroll delta of ScrollPane incorrectly > depends on the size of its content. > This leads to extremely slow scrolling when the content is only slightly > larger than the ScrollPane. This pull request has now been integrated. Changeset: 5805bf8e Author:Michael Strauß Committer: Johan Vos URL: https://git.openjdk.java.net/jfx/commit/5805bf8e5de90427a68164e52c1fbbd08bfd0aa4 Stats: 73 lines in 2 files changed: 53 ins; 11 del; 9 mod 8276313: ScrollPane scroll delta incorrectly depends on content height Reviewed-by: kcr, aghaisas, jvos - PR: https://git.openjdk.java.net/jfx/pull/660
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
On Mon, 6 Dec 2021 08:22:37 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Merge branch 'openjdk:master' into > bugfix/JDK-8276167-VirtualFlow-scrollToTop > - JDK-8276167: fixing VirtualFlow.scrollToTop(int) I tested it again. Without the changes in this PR the bug is still there. If the item is larger than the viewport, the VirtualFlow.scrollToTop(int) scrolls to the end instead of to top of the item. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v2]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'openjdk:master' into bugfix/JDK-8276167-VirtualFlow-scrollToTop - JDK-8276167: fixing VirtualFlow.scrollToTop(int) - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/087e11d3..a7820448 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=656=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=656=00-01 Stats: 4192 lines in 92 files changed: 3532 ins; 274 del; 386 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276313: ScrollPane scroll delta incorrectly depends on content height [v3]
On Thu, 2 Dec 2021 22:49:45 GMT, Michael Strauß wrote: >> This PR fixes an issue where the scroll delta of ScrollPane incorrectly >> depends on the size of its content. >> This leads to extremely slow scrolling when the content is only slightly >> larger than the ScrollPane. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > Changes per review This looks good to me. This new approach is based on the delta between the total height and the visible height, instead of only the total height. It might be good to capture this idea somehow in the commit message, but since it is clear from the code, I am totally ok with not changing the commit message. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/660
