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

How to set padding for text from borders of editable area? #255

Closed
alt-grr opened this issue Feb 17, 2016 · 34 comments
Closed

How to set padding for text from borders of editable area? #255

alt-grr opened this issue Feb 17, 2016 · 34 comments

Comments

@alt-grr
Copy link

alt-grr commented Feb 17, 2016

I tried this

.styled-text-area Navigator {
    -fx-padding: 5px;
}

but it not worked 😕

@JordanMartinez
Copy link
Contributor

Per this CSS tutorial, shouldn't you write something like this:

.styled-text-area .navigator {
    -fx-padding: 5px;
}

or perhaps this:

.styled-text-area > .navigator {
    -fx-padding: 5px;
}

@alt-grr
Copy link
Author

alt-grr commented Feb 17, 2016

Nope, I inspected CSS classes and styles using ScenicView - Navigator object doesn't have any CSS classes, so your code will not work.

CSS from my first comment correctly set padding to Navigator object, but problem is that visually nothing changed.

@JordanMartinez
Copy link
Contributor

Are you adding the style sheet to the Scene?

@JordanMartinez
Copy link
Contributor

What about this per the styled-text-area.css file

.styled-text-area .paragraph-box .paragraph-text {
    -fx-padding: 5px;
}

@alt-grr
Copy link
Author

alt-grr commented Feb 17, 2016

  1. Yes, I'm adding CSS file to scene. I already said that I could see changes in ScenicView.

  2. I found this code and it's setting padding between paragraphs. I want to set padding between paragraphs and borders of entire text area.

@TomasMikula
Copy link
Member

The right CSS to use would be

.styled-text-area {
    -fx-padding: 5px;
}

Not sure why you are trying to access Navigator or ParagraphBox.

But it doesn't work anyway.

Workaround: Wrap the area in some Pane and set padding on that.

Fix: In StyledTextArea.layoutChildren(), change

virtualFlow.resize(getWidth(), getHeight());

to

Insets ins = getInsets();
virtualFlow.resizeRelocate(ins.getLeft(), ins.getTop(), getWidth() - ins.getLeft() - ins.getRight(), getHeight() - ins.getTop() - ins.getBottom());

Comment: It is super easy to forget to account for some kind of JavaFX quirk, such as padding (insets). In Region, JavaFX promises the users that they can use -fx-padding whenever they have a Region, but does not enforce in any way that implementations of Region actually take the value of padding into account. You could say that StyledTextArea is broken, because it doesn't handle padding properly. I say their design is broken. Properly handling idiosyncrasies like padding adds unnecessary complexity to otherwise straightforward code (compare the two code fragments above), and code like this has to be repeated in each third-party control. Ideally padding would be taken care of once and for all somewhere in the framework.

@alt-grr
Copy link
Author

alt-grr commented Feb 18, 2016

Thank you for your reply. I tried setting padding on .styled-text-area, but this adds padding including scrollbar, which is not what I want.

I will try using your proposed fix tomorrow.

@TomasMikula
Copy link
Member

Good point! This fits well with my comment about breaking things apart. Turns out in the master branch @JordanMartinez already took scrollbars out of StyledTextArea, and into a wrapper node VirtualizedScrollPane. This way, if you add padding to StyledTextArea, the padding stays inside the scrollbars.

@JordanMartinez
Copy link
Contributor

Yeah, we really need to release version 0.7 ....

@kuc I'm sorry for not reading your comments more carefully. Also, CSS is something with which I'm not well versed. Glad Tomas could help you out though!

@alt-grr
Copy link
Author

alt-grr commented Feb 18, 2016

I tried described fix on version from master branch, after wrapping StyleClassedTextArea into VirtualizedScrollPane, and it works. Only minor detail is wrong: top/bottom padding is always visible. It should be only visible if textarea is scrolled fully to top/bottom.

Also, SNAPSHOT version of StyleClassedTextArea has some serious issues:

  • wrong mouse cursor
  • delete, backspace and arrow keys are not working
  • requestFocus() is not working maybe it's because I'm now calling it on VSP, I will check this again
  • TextAreaSkin and TextAreaBehavior are removed, so I don't know how to now have equivalent of calling textAreaBehavior.traversePrevious() / textAreaBehavior.traverseNext()

😞

@JordanMartinez
Copy link
Contributor

Also, SNAPSHOT version of StyleClassedTextArea has some serious issues:

  • wrong mouse cursor
  • delete, backspace and arrow keys are not working
  • requestFocus() is not working

I just tested the snapshot version of SCTA and those issues did not arise.

Also, what do you mean by TextAreaSkin and TextAreaBehavior?

@alt-grr
Copy link
Author

alt-grr commented Feb 18, 2016

I just tested the snapshot version of SCTA and those issues did not arise.

That's weird. I only updated library version and this all problems showed up...

what do you mean by TextAreaSkin and TextAreaBehavior?

I'm talking about removed getSkin() method from StyleClassedTextArea

@JordanMartinez
Copy link
Contributor

I'm talking about removed getSkin() method from StyleClassedTextArea

Ah! Ok. Since so many other developers needed access to the skin to further modify things, we decided to change how that works.
Now, model content of StyledTextArea (the Control) was moved to StyledTextAreaModel and StyledTextAreaView (the Skin) is now StyledTextArea.

@JordanMartinez
Copy link
Contributor

Thus, StyledTextArea no longer extends Control and doesn't use getSkin().

@JordanMartinez
Copy link
Contributor

Only minor detail is wrong: top/bottom padding is always visible. It should be only visible if textarea is scrolled fully to top/bottom.

My guess is that the top and bottom inset should only be factored into the layoutChildren() call when either the first or last child is visible. So, something like this code would fix it:

Edit: made code account for possibility where top and bottom are both visible

Insets ins = getInsets();
ObservableList<Cell<Paragraph<PS, S>>> cells = virtualFlow.visibleCells();
boolean topVisible = cells.contains(virtualFlow.getCell(0));
boolean bottomVisible = cells.contains(
    // since no way to know what the last cell index is in virtualFlow
    // just use the number of paragraphs
    virtualFlow.getCell(getParagraphs().size() - 1)
);

double y, height;
if (topVisible) {
    y = ins.getTop();
    height = bottomVisible
                  ? getHeight() - ins.getTop() - ins.getBottom()
                  : getHeight() - ins.getTop();
} else {
    y = 0;
    height = bottomVisible
                ? getHeight() - ins.getBottom()
                : getHeight();
}

virtualFlow.resizeRelocate(
    ins.getLeft(), 
    y, 
    getWidth() - ins.getLeft() - ins.getRight(), 
    height
);

@alt-grr
Copy link
Author

alt-grr commented Feb 18, 2016

I really only needed getSkin() method to implement this workaround, originally for standard TextArea:
http://stackoverflow.com/questions/12860478/tab-key-navigation-in-javafx-textarea

But after removing TextAreaSkin and TextAreaBehavior from StyleClassedTextArea, I not longer can find any method for switching focus: https://github.com/TomasMikula/RichTextFX/search?utf8=%E2%9C%93&q=focus

Seems that implementation of these methods were part of TextInputControlBehavior - but you said that StyledTextArea no longer extends Control, so now they are gone...

@JordanMartinez
Copy link
Contributor

Yeah, that does present a problem.... I'm not sure if this will still be supported underneath this new design structure as this is a base off of which others can build. I'll let @TomasMikula answer that.

In case it won't be, there is the focusFX library

@JordanMartinez
Copy link
Contributor

Per Tomas' comment in #234:

Originally, you could only enter the StyledTextArea by Tab, not leave it, because, as you notice, the Tab key handler is overridden. So entering by Tab is all that needs to be done in this issue.

I don't think it will be supported.

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

@JordanMartinez

I just tested the snapshot version of SCTA and those issues did not arise.

I found a way how to reproduce this bug: #257

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

Edit: made code account for possibility where top and bottom are both visible

@JordanMartinez I tested your code and currently it changes nothing in comparison to @TomasMikula proposed fix 😞
But I get your idea, and will try to make this work 👍

@JordanMartinez
Copy link
Contributor

I tested your code and currently it changes nothing in comparison to TomasMikula proposed fix 😞

Really? I wonder why....

@TomasMikula
Copy link
Member

so I don't know how to now have equivalent of calling textAreaBehavior.traversePrevious() / textAreaBehavior.traverseNext()

Even in JavaFX, these are private API, so you were not supposed to call them in the first place. Btw, some sort of public focus traversal API has a good chance to be added in JavaFX 9 (see JDK-8091673). I haven't really looked at it, but hopefully it will be something that we can then support.

@TomasMikula
Copy link
Member

Using private API, you can override the Tab to do

area.impl_traverse(com.sun.javafx.scene.traversal.Direction.NEXT);

Taken from BehaviorBase.

Addendum: This will stop working in JavaFX 9, but then again, there should be a public API by then.

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

BTW: I find out that cell is marked as "invisible" only when it's 100% displayed offscreen.


Even in JavaFX, these are private API, so you were not supposed to call them in the first place.

Ok, I can live without this... Next/previus focus targets could be always hardcoded.


Really? I wonder why....

I don't know, but I think that I just found the best possible solution.
If first and last ParagraphBox or ParagraphText nodes would have CSS classes like first-paragraph/last-paragraph, then I could give them different top/bottom padding. I just tested this by setting padding to all paragraphs, and top-padding works correctly, even with scrolling! But this also sets padding between all paragraphs, so I must style only first and last one.

@TomasMikula @JordanMartinez Do you think that this would be much work to add that two CSS classes? Because I don't know RichTextFX codebase well, so maybe you could fix that more quickly than me.

@JordanMartinez
Copy link
Contributor

BTW: I find out that cell is marked as "invisible" only when it's 100% displayed offscreen.

Gotcha...

If first and last ParagraphBox or ParagraphText nodes would have CSS classes like first-paragraph/last-paragraph, then I could give them different top/bottom padding.

If we implemented that idea, it'd probably be better to have an index as opposed to first-paragraph and last-paragraph. (Not sure if that can be done or how). But then it would incorporate other situations that might arise if paragraph n needed to do something special.

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

it'd probably be better to have an index

I don't think so, how would you style the last paragraph with this?
To clarify, I'm talking about first and last from "virtual" paragraphs nodes, not the "real" last and first paragraph. I can already lookup() them, but they not always exists etc., what's making this method not so easy.

@TomasMikula
Copy link
Member

@kuc It could be implemented, and you would be able to get the desired visual output, but I don't think that it is "the best possible solution". It will make the padding a property of a paragraph, while logically it is a property of the whole document (so should be handled by some node containing containing the whole document). I think it should be handled by the VirtualFlow.

Still, the solution you propose might be a faster way to get the desired result.

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

but I don't think that it is "the best possible solution". It will make the padding a property of a paragraph, while logically it is a property of the whole document

I mean "best possible solution" = "works as excepted without any quirks (maybe....)" 😉

You are right, but because whole document is virtualized, some "obvious" things can be not easy to implement in straightforward way.

@JordanMartinez
Copy link
Contributor

I don't think so, how would you style the last paragraph with this?

Oh, right. I forgot about that 😄

@TomasMikula
Copy link
Member

OK, there you go. There are now pseudo classes :first-paragraph, :last-paragraph and also :has-caret on ParagraphBox. Since the default stylesheet already specifies padding for .paragraph-text with pretty high specificity, you have to be even more specific to override that. This will work:

.styled-text-area .paragraph-box:first-paragraph .paragraph-text {
    -fx-padding: 10 0 0 0;
}

.styled-text-area .paragraph-box:last-paragraph .paragraph-text {
    -fx-padding: 0 0 10 0;
}

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

Wow, thank you so much! 😃
I found that you must style last paragraph before first paragraph, because otherwise when only one paragraph is displayed, it will be styled as the last paragraph 😉


BTW, seems that with different paragraphs heights, virtual scroll pane does not allow to fully scroll up/down using arrow keys - but this a minor issue, and probably fixable by adding some listener somewhere.


Because list of workarounds is somewhat complete, I think that I can close this issue.
I can also create PR with fix for padding, that you posted in this thread, to allow people more choices.

@alt-grr alt-grr closed this as completed Feb 19, 2016
@TomasMikula
Copy link
Member

BTW, seems that with different paragraphs heights, virtual scroll pane does not allow to fully scroll up/down using arrow keys - but this a minor issue, and probably fixable by adding some listener somewhere.

The intended behavior of StyledTextArea is to scroll just enough to make the caret visible. If you have some additional padding, then you can not get there by just arrow keys, which only change the caret position. Does this explain the behavior, or is there some other quirk to it?

@TomasMikula
Copy link
Member

when only one paragraph is displayed, it will be styled as the last paragraph 😉

Both styles should be used. But if there is some conflict, as is the case in the padding example I gave, then the latter will take precedence. If JavaFX supported padding-top and padding-bottom properties, then using -fx-padding-top for :first-paragraph and -fx-padding-bottom for :last-paragraph would nicely apply both to the paragraph that is both first and last. As an alternative, you can use

... .paragraph-box:first-paragraph:last-paragraph ... {
    ...
}

to be more explicit about what you want in the case when there is only one paragraph, and since this is a more specific selector than the others, it will take precedence.

@alt-grr
Copy link
Author

alt-grr commented Feb 19, 2016

The intended behavior of StyledTextArea is to scroll just enough to make the caret visible. If you have some additional padding, then you can not get there by just arrow keys, which only change the caret position. Does this explain the behavior, or is there some other quirk to it?

This explains it all. Moreover, I just tested <textarea> on Chrome and it behaves exactly the same when I am using arrows to move the caret.

Both styles should be used.

Yes, they are both correctly set on node. But in your stylesheet example they have the same priority, so last recently defined style will be used.

As an alternative, you can use paragraph-box:first-paragraph:last-paragraph

Thanks for suggestion, I am aware of this :)

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

No branches or pull requests

3 participants