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

Feature: Allow paragraphs to fill from bottom to top #502

Closed
JordanMartinez opened this issue May 11, 2017 · 11 comments
Closed

Feature: Allow paragraphs to fill from bottom to top #502

JordanMartinez opened this issue May 11, 2017 · 11 comments

Comments

@JordanMartinez
Copy link
Contributor

Already reported in FXMisc/Flowless#34, but I thought I'd add it here so that people are aware of it.

RTFX currently fills the viewport from the top down with any extra space appearing at the bottom when there isn't enough content to completely fill the viewport. This feature would allow the option of specifying whether the paragraphs fill from top to bottom (current) or from bottom to top (new feature).

@neilccbrown
Copy link

As a demonstration, I've put together a build of RichTextFX which demonstrates this.

Source: https://github.com/twistedsquare/RichTextFX/tree/styleable_gravity_demo
(you'll need to first install 0.6-SNAPSHOT locally from my flowless branch: https://github.com/twistedsquare/Flowless/tree/styleable_gravity )
Binaries: https://github.com/twistedsquare/RichTextFX/releases/tag/v0.7-M5n

The JavaKeywords demo has dynamically changeable gravity: if you press F8 while there aren't enough lines to fill the pane, they swap from showing at the top to showing at the bottom. This is done via setting a CSS pseudo-class which in turn toggles the gravity property.

@JordanMartinez
Copy link
Contributor Author

@twistedsquare Could you update your build to use #504? I'm curious how NavigationTests.ViewportTests's Page Up/Down tests will do.

@neilccbrown
Copy link

It looks like those tests are unaffected. Before I change gravity, I get 170 tests run, 93 failed, 9 ignored (is this expected? I'm running on Mac OS X if that matters). If I then change gravity to rear for all text areas, I get 170 run, 97 failed, 9 ignored. The four new failures are:

ClickAndDragTests$WhenAreaIsEnabled$AndTextIsNotSelected.pressingMouseOverTextAndDraggingMouseSelectsText
ClickAndDragTests$WhenAreaIsEnabled$AndTextIsNotSelected.singleClickingAreaMovesCaretToThatPosition
ClickAndDragTests$WhenAreaIsEnabled$AndTextIsSelected.pressingMouseOnSelectionAndDraggingDisplacesCaret
ClickAndDragTests$WhenAreaIsEnabled$AndTextIsSelected.singleClickingWithinSelectedTextMovesCaretToThatPosition

Digging into those, looks like the issue is InlineCssTextAreaAppTest.firstLineOfArea which positions the mouse 5x5 pixels from the top left of the pane; not too surprising that switching the gravity causes this to behave differently. But the ViewportTests tests are unaffected: testShiftPageDown, testShiftPageUp and testPageUp all pass, and testPageDown is ignored.

@JordanMartinez
Copy link
Contributor Author

Before I change gravity, I get 170 tests run, 93 failed, 9 ignored (is this expected? I'm running on Mac OS X if that matters). If I then change gravity to rear for all text areas, I get 170 run, 97 failed, 9 ignored.

93-97? Wow... The tests all pass on my machine (Linux) with some showing their flakiness at times. Mac might be a reason, but I don't think it'd cause that large of an issue. Can you post their reasons for failure?

the ViewportTests tests are unaffected: testShiftPageDown, testShiftPageUp and testPageUp all pass, and testPageDown is ignored.

I was a bit surprised at this at first, but then I realized why that makes sense. Tests pass based on whether they select the right things and the caret position is in the expected spot. However, I don't have a bounds test that checks whether the caret's bounds on screen moves up/down depending on page up/down. When your code gets implemented, we'd need to write a variant of this test suite to account for the inverted paragraph layout.

@JordanMartinez
Copy link
Contributor Author

On another hand... how are you running 170 tests? There's only 100 that run on mine.

@neilccbrown
Copy link

Hmm, you're right, something is wrong there. I had just used IntelliJ and right-clicked and selected Run All Tests on the richtextfx/src/test/java directory. Looking again, a whole bunch fail with a test harness exception, and it seems that some tests are trying to run twice, and failing the second time. I think perhaps it has found a bunch of class files generated by gradle, and a bunch generated by IntelliJ, and one of the two fails. Now I've run via gradle's test task: 100 tests, 27 failed before gravity changed, 31 failed with gravity flip. So that explains the 170, at least -- sorry for that.

As for the failures: there's some which seem to happen because the paste shortcut isn't being triggered, e.g.

Expected :Once upon a time, text by a knight.
Actual   :Once upon a time, v by a knight.

tells you a lot. Bunch of those for cut and for paste, with x and v appearing. Most of the rest are failures in NavigationTests, where various up/down/home/end tests fail.

@JordanMartinez
Copy link
Contributor Author

Hmm, you're right, something is wrong there. I had just used IntelliJ and right-clicked and selected Run All Tests on the richtextfx/src/test/java directory.

Ah, that is probably because I'm using NestedRunner to create these test suites.

the paste shortcut isn't being triggered

Strange... TestFX wouldn't convert Shortcut to is OS-specific shortcut key in 0.4.4-alpha, but it does properly convert it in the version you tried out, 0.4.5-alpha. The next release 0.4.6-alpha changes some of the event handling in TestFX, but I'm not sure if it would make a difference.
I've also noticed some differences between type(SHORTCUT, V) and press(SHORTCUT).type(V).release(SHORTCUT). That may also be a factor.

Most of the rest are failures in NavigationTests, where various up/down/home/end tests fail.

That's probably the NavigationTests.MultiLineTests test suite. As stated in the PR, if I remove the "force-display stage" code, it builds correctly on Travis. However, removing that code makes it fail on my machine.

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented May 17, 2017

Ok, #504 should now break your example of RTFX.

@neilccbrown
Copy link

Same results as before: the page up/down tests pass. I wonder if there is a misunderstanding here due to me not describing the feature accurately enough. The gravity setting doesn't alter the paragraph order. It solely alters whether the lump of paragraphs appears glued to the top or glued to the bottom (like JavaFX's -fx-alignment style) when there aren't enough paragraphs to fill the viewport. Here's screenshots of the Java demo with the two gravity settings:

richtextfx-gravity-top

richtextfx-gravity-bottom

So page up will still move the cursor up, page down will move it down. The only tests that it breaks are the ones that expect clicking just inside the top of the pane and dragging will select some text, but of course there is no text there if gravity is set to bottom (they just need to look for the position of the first line, rather than assuming it is a couple of pixels down from the top).

@JordanMartinez
Copy link
Contributor Author

Silly me! Yes, that was my understanding until you cleared it up. Now that I think about it, laying out the paragraphs from bottom to top doesn't make sense in the first place...

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented Oct 3, 2017

I've now been granted access to Flowless under the FXMisc organization. So, I made a new snapshot release (still 0.6-SNAPSHOT).
When using that snapshot, one can achieve this using the following CSS in an external file:

# if using virtual flow in multiple places in the same scenegraph
.styled-text-area > .virtual-flow {
    # this will fill things from the bottom-up
    -flowless-gravity: rear;
}

Screenshot below of hyperlink demo:
2017-10-03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants