Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]
On Wed, 27 Jan 2021 11:21:19 GMT, Ajit Ghaisas wrote: >> Robert Lichtenberger has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8228363: ContextMenu.show with side=TOP does not work the first time in >> the presence of CSS >> >> Removed node orientation from API doc. >> Removed extra blank lines. > > modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java > line 312: > >> 310: if (getItems().size() == 0) return; >> 311: >> getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation()); >> 312: setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); > > Do we need to handle RTL case here? I don't think so. At least we didn't handle the RTL case here until now. The call to setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); is only neccessary to "reset" the anchor location in case we called show with a Side before. - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]
On Wed, 27 Jan 2021 10:41:25 GMT, Ajit Ghaisas wrote: >> Robert Lichtenberger has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8228363: ContextMenu.show with side=TOP does not work the first time in >> the presence of CSS >> >> Removed node orientation from API doc. >> Removed extra blank lines. > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java > line 49: > >> 47: import static com.sun.javafx.scene.control.ContextMenuContentShim.*; >> 48: >> 49: import java.io.File; > > These 3 imports are unused. Remove them. Good catch. I removed the (now unused) imports. - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]
On Mon, 25 Jan 2021 11:40:08 GMT, Robert Lichtenberger wrote: >> By using the anchor location facility of PopupWindows we can avoid >> miscalculation of the >> menu's height entirely. >> This fix also cleans up some documentation issues. >> This fix introduces tests that check the correct positioning (test_position_) >> test_position_withCSS reproduces the problem that is fixed with this patch. >> The other test_position_ cases serve as "proof" that no regressions are >> introduces. >> They work before and after the fix is introduced. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8228363: ContextMenu.show with side=TOP does not work the first time in the > presence of CSS > > Removed node orientation from API doc. > Removed extra blank lines. modules/javafx.controls/src/test/java/test/javafx/scene/control/ContextMenuTest.java line 49: > 47: import static com.sun.javafx.scene.control.ContextMenuContentShim.*; > 48: > 49: import java.io.File; These 3 imports are unused. Remove them. modules/javafx.controls/src/main/java/javafx/scene/control/ContextMenu.java line 312: > 310: if (getItems().size() == 0) return; > 311: > getScene().setNodeOrientation(anchor.getEffectiveNodeOrientation()); > 312: setAnchorLocation(AnchorLocation.CONTENT_TOP_LEFT); Do we need to handle RTL case here? - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]
On Mon, 25 Jan 2021 11:40:08 GMT, Robert Lichtenberger wrote: >> By using the anchor location facility of PopupWindows we can avoid >> miscalculation of the >> menu's height entirely. >> This fix also cleans up some documentation issues. >> This fix introduces tests that check the correct positioning (test_position_) >> test_position_withCSS reproduces the problem that is fixed with this patch. >> The other test_position_ cases serve as "proof" that no regressions are >> introduces. >> They work before and after the fix is introduced. > > Robert Lichtenberger has updated the pull request incrementally with one > additional commit since the last revision: > > 8228363: ContextMenu.show with side=TOP does not work the first time in the > presence of CSS > > Removed node orientation from API doc. > Removed extra blank lines. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/383
Re: RFR: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS [v3]
> By using the anchor location facility of PopupWindows we can avoid > miscalculation of the > menu's height entirely. > This fix also cleans up some documentation issues. > This fix introduces tests that check the correct positioning (test_position_) > test_position_withCSS reproduces the problem that is fixed with this patch. > The other test_position_ cases serve as "proof" that no regressions are > introduces. > They work before and after the fix is introduced. Robert Lichtenberger has updated the pull request incrementally with one additional commit since the last revision: 8228363: ContextMenu.show with side=TOP does not work the first time in the presence of CSS Removed node orientation from API doc. Removed extra blank lines. - Changes: - all: https://git.openjdk.java.net/jfx/pull/383/files - new: https://git.openjdk.java.net/jfx/pull/383/files/67a7da14..b02dcb02 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=383=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=383=01-02 Stats: 8 lines in 1 file changed: 0 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/383.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/383/head:pull/383 PR: https://git.openjdk.java.net/jfx/pull/383
