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

Support entirely RTL documents #1170

Open
vahidooo opened this issue Feb 4, 2023 · 25 comments
Open

Support entirely RTL documents #1170

vahidooo opened this issue Feb 4, 2023 · 25 comments

Comments

@vahidooo
Copy link

vahidooo commented Feb 4, 2023

Hi,

I am working on a text editor project. Our editor is supposed to be able to handle some documents those are entirely RTL. For instance, suppose that our documents are entirely Farsi or Arabic and contain no word from English and other LTR languages.

I cloned RichtextFX project with its demos and started from org.fxmisc.richtext.demo.richtext.RichTextDemo.
I thought that I can provide RTL support just by changing the node orientation of the area. Therefore, I changed the default constructor of FoldableStyledArea like the following:

public FoldableStyledArea() {
            super(
                    ParStyle.EMPTY,                                                 // default paragraph style
                    (paragraph, style) -> paragraph.setStyle(style.toCss()),        // paragraph style setter
                    TextStyle.EMPTY.updateFontSize(12).updateFontFamily("Serif").updateTextColor(Color.BLACK),  // default segment style
                    styledTextOps._or(linkedImageOps, (s1, s2) -> Optional.empty()),                            // segment operations
                    seg -> createNode(seg, (text, style) -> text.setStyle(style.toCss())));                     // Node creator and segment style setter

            setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT);    // <--------- THIS LINE WAS ADDED
        }

But the result isn't that good as it expected. When I start to type, text is being rendered correctly from right to left, but there are some problems.

  1. The caret is still on the left side of the window and move from left to right while I am typing.
  2. I should press the left arrow key if I want to move the caret backward.
  3. Similar issue happens for selection. When I am at the end of paragraph and press the Shift and right arrow keys to select the last, nothing happens. I have to press the left arrow to select the preceding character.
  4. Another problem with selection is that when I select part of text, a blue box appears in the left side of the window. It also becomes larger in the inverse direction if I select more characters.
  5. When I am at the end of paragraph and press the Space key to start next word, the line becomes indented and by typing the next non-space character, it becomes unindented.

To be clear, I captured my sample work to demonstrate the above problems. Please see this screen record.

Best,
Thanks.

@vahidooo
Copy link
Author

vahidooo commented Feb 4, 2023

For selection, I debugged the code and found out that the problem is in ParagraphText#getRangeShapeSafely when it called from ParagraphText#updateSingleSelection. The shape returned by TextFlowExt#getRangeShape is based on left.
I checked the ParagraphText. Its nodeOrientation was null and effectiveNodeOrientation was RIGHT_TO_LEFT.

As an initial idea, It seems that the problem could be solved by rotating the shape around the middle of the ParagraphText (or ParagraphBox) to make it right anchored.
However, it requires that we know the direction of paragraph inside the ParagraphText. So, we have to add direction to Paragraph or somewhere else.
Of course, I think it is very common in the word processing application like Microsoft Word to apply direction in the paragraph level. In open xml standard, it defines in paragraph property (see this part of documentation) which is equivalent of paragraph style in our model. By defining direction in paragraphs, we also can go one step forth and handle complex documents those are containing both RTL paragraphs and LTR paragraphs.

I will be glad if you guide me to contribute and resolve this issue by myself and make a push request.

@vahidooo vahidooo changed the title Support RTL documents Support entirely RTL documents Feb 4, 2023
@Jugen
Copy link
Collaborator

Jugen commented Feb 6, 2023

The caret position and selection are tied together, so I think that the caret behavior should be fixed first and hopefully that'll lead to the selection behaving as well.

So the caret shape and position is determined here in TextFlowExt:

PathElement[] getCaretShape(int charIdx, boolean isLeading) {
return caretShape(charIdx, isLeading);
}

The method caretShape here is from JavaFX TextFlow. So the first thing to check is if TextFlow is returning the correct shape?

@vahidooo
Copy link
Author

vahidooo commented Feb 7, 2023

JavaFX TextFlow returns shape of caret in its local coordinates, as mentioned in the documentation. So the shape is independent of the node orientation. It is the same for selection, as well.

@vahidooo
Copy link
Author

vahidooo commented Feb 7, 2023

Last night I tried another approach. I rolled back the constructor of FoldableStyledArea. So its orientation changed back to the default value, namely LEFT_TO_RIGHT. But I changed the alignment of ParagraphText to TextAlignment.RIGHT. In this way, the problems of caret and selection got solved but the problem related to typing space in the leading position of paragraph(case 5) happened again. In addition, a new problem emerged. The bullets were not aligned to right.

I'm not sure that we should start from node orientation, text alignment, or another point. @Jugen Do you agree with the node orientation approach?

@Jugen
Copy link
Collaborator

Jugen commented Feb 8, 2023

From the JavaDocs it seems that the node orientation approach is the proper way to do it.

It seems to me that caretShape is returning the incorrect shape for charIdx, specifically it's returning the shape for the charIdx counting from the left and not from the right. If I change the method updateSingleCaret in ParagraphText to reverse the charIdx then the caret behaves as expected:

private void updateSingleCaret(CaretNode caretNode) {
    int charIdx = getClampedCaretPosition(caretNode);
    if ( getEffectiveNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT ) {
        charIdx = paragraph.length() - charIdx;
    }
    PathElement[] shape = getCaretShape(charIdx, true);
    caretNode.getElements().setAll(shape);
}

Could you verify this observation please.
Then same to seems to apply for selection, but that appears more complicated.

@vahidooo
Copy link
Author

vahidooo commented Feb 8, 2023

@Jugen I truly appreciate all your time spent on my question.

I checked it. There have been a few hiccups along the way.

First of all, ParagraphText#getEffectiveNodeOrientation() is returning NodeOrientation.LEFT_TO_RIGHT independently of the node orientation of the area. However I temporary set the node orientation of ParagraphText in its constructor. We'll resolve it later.

Your idea solve the problem related to the direction of caret movement(Problem 2 in the first comment of the discussion) just for LTR texts. For RTL texts, caret is always stuck to the left side of screen.

I should also note that for both RTL and LTR text, the caret is in the left side at the beggining. I mean Problem 1 still happens.

Thanks.

@Jugen
Copy link
Collaborator

Jugen commented Feb 9, 2023

Sorry I've been testing with JavaKeywordsDemo and not RichTextDemo, my bad :-)

In RichTextDemo it seems that wrap text is messing us around as well. If you click the wrap check box to switch wrapping off then it'll behave as expected with my changes. However positioning the cursor by clicking is then broken because TextFlow/HitInfo doesn't seem to return the correct charIdx for hitTest( Point2D ) here:

CharacterHit hit(double x, double y) {
TextFlowSpan span = textLayout().getLineSpan( (float) y );
Rectangle2D lineBounds = span.getBounds();
HitInfo hit = hitTest(new Point2D(x, y));
int charIdx = hit.getCharIndex();

That's the same problem as with caretShape mentioned previously as TextFlow/HitInfo is counting from the left and not from the right, even though it reports the correct NodeOrientation.

Are you sure about this:

First of all, ParagraphText#getEffectiveNodeOrientation() is returning NodeOrientation.LEFT_TO_RIGHT independently of the node orientation of the area.

Don't you mean "getNodeOrientation() is returning LEFT_TO_RIGHT" ? When I check getEffectiveNodeOrientation() it returns RIGHT_TO_LEFT as expected.

(Also just a heads up that unfortunately I don't have a lot of spare time currently so progress on this is going to be slow on my end)

@vahidooo
Copy link
Author

vahidooo commented Feb 9, 2023

I tested with disabled line wrapping but the result is identical.
I had to note that I am using javafx version '18.0.1' on java 17. Does it cause the problem?

@vahidooo
Copy link
Author

vahidooo commented Feb 9, 2023

I think that the root of the problem is that the CatetNode is child of ParagraphText. If we define a Group and make it the parent of both CaretNode and ParagraphText then the coordination of CaretNode will automatically become revered by changing the node orientation of the group. Because the origin of TextFlow is always on its top-left corner independent of its node orientation.

To demonstrate this difference between Group and TextFlow, I wrote this simple Kotlin snippet:

class JavaFXBidiText : Application() {
    override fun start(stage: Stage) {
        val font = Font("Tahoma", 48.0)

        val text1 = Text("سلام علیکم")
            .apply { this.font = font }
        val caret1 = Rectangle(0.0, 0.0, 15.0, 60.0)
            .apply { fill = Color.RED }
        val textFlow1 = TextFlow(text1, caret1)


        val text2 = Text("سلام علیکم")
            .apply { this.font = font }
        val caret2 = Rectangle(0.0, 0.0, 15.0, 60.0)
            .apply { fill = Color.BLUE }
        val textFlow2 = TextFlow(text2)
        val group = Group(textFlow2, caret2)

        val vbox = VBox(textFlow1, group)
            .apply { nodeOrientation = NodeOrientation.LEFT_TO_RIGHT }

        val scene = Scene(vbox, 650.0, 150.0, Color.WHITE)
        scene.setOnMouseClicked {
            vbox.nodeOrientation =
                if (vbox.nodeOrientation == NodeOrientation.LEFT_TO_RIGHT) NodeOrientation.RIGHT_TO_LEFT
                else NodeOrientation.LEFT_TO_RIGHT
        }
        stage.scene = scene
        stage.show()
    }
}

fun main(args: Array<String>) {
    Application.launch(JavaFXBidiText::class.java, *args)
}

Now let's run it. Initially, everything is LTR:
image

But when I press click, the result is this:
image

@vahidooo
Copy link
Author

vahidooo commented Feb 10, 2023

I implemented my idea. I mean wrapping the text flow, caret, and selection into a common parent, namely a Region or Group. In this way, all issues, except the last one, are resolved.
@Jugen Could you please take a look at my work?

If my solution is correct and ok with you the next step is solving the problem of trailing space(Problem 5). It seems that it is a bug comming up from the lower layers. I asked a question about this issue on SO. How do you feel about that?

@Jugen
Copy link
Collaborator

Jugen commented Feb 12, 2023

That's all great, thanks for the effort. From SO looks like it may be a Mac thing ?

With regards to your work its hard to see exactly what you have changes as your IDE has auto formatted some stuff.
Could you please either provide a version with just your changes (preferred), or make an initial commit with just the IDE changes and then your changes on top of that. Then it'll be far easier to assess. Thanks :-)

@vahidooo
Copy link
Author

vahidooo commented Feb 14, 2023

I'm sorry for my messy commit. I tidied it up. Could you please take a look at it once again?

It seems that my work solve the selection and caret issues but generate a new bug :)
If you change the style of a word, the previous word moves to left.
For example, in the following figure, I didn't delete the space between the first and second words. I just set underline for the second word.

Screenshot 1401-11-25 at 22 44 21

By the way, I tested the trailing space problem in Linux. It never happens. You are right.

@vahidooo
Copy link
Author

I just found out that my work also has ruined line wrapping feature, unfortunately.
But I still think that it could be a good starting point to solve the RTL issues step by step.

@Jugen
Copy link
Collaborator

Jugen commented Feb 20, 2023

Okay so I've had a look at your proposal and I see what you've done and I think I understand why it works. :-)
The line wrap isn't problem, just binding the TextFlow width & height to ParagraphText seems to fix it:

textFlow = new TextFlowExt();
textFlow.prefHeightProperty().bind( heightProperty() );
textFlow.prefWidthProperty().bind( widthProperty() );

With regards to styling: underlining is a special case, can you try strike-through instead and see if it also misbehaves or is it just underlining ?

@Jugen
Copy link
Collaborator

Jugen commented Feb 20, 2023

I also think additional changes besides the ones you've already made need to be made in GenericStyledAreaBehavior so that [Home], [End], and [Ctrl]+[A] also work correctly.

@vahidooo
Copy link
Author

According to your suggestion, line wrapping issue is resolved. I just pushed a new commit.

And about underline, not only underline and strike-through but also other kinds of formatting, e.g. changing text color or bolding, have the same issue, I explained.

There are also some other wrong behaviors. For example, changing background color doesn't make any change.
A disaster case happens when applying bold and italic on a single word. It makes a complete mess. letters become changed and broken.

I also had a look at GenericStyledAreaBehavior to correct the aforementioned shortcuts. But couldn't understand where I should start. I need your help.

By the way, I would like to apologize for my poor English.

@vahidooo
Copy link
Author

Of course your solution for line wrapping cause a new issue. The text doesn't adhere to the side of window. If you resize the window from its right side text doesn't move with it.

@Jugen
Copy link
Collaborator

Jugen commented Feb 22, 2023

Okay I'll have a look at that.
I started looking at the GenericStyledAreaBehavior issues which led me to a RTL bug in TextFlowLayout which I'll also need to fix. I'm hoping this will also fix some of the other issues you're experiencing, we'll see.

By the way, your English is fine and certainly not poor :-)

@vahidooo
Copy link
Author

This week I have a lot of time. I hope that I could fix the bug. Could you please explain it?

@Jugen
Copy link
Collaborator

Jugen commented Feb 28, 2023

I have fixed the TextFlowLayout issue. Replace method getLineCount with:

int getLineCount() {
   
    if ( lineCount > -1 ) return lineCount;

    lineCount = 0;
    lineMetrics.clear();
    boolean ltr = NodeOrientation.LEFT_TO_RIGHT == flow.getEffectiveNodeOrientation();
    double totLines = 0.0, prevMaxY = -1.0;
    int totCharSoFar = 0;

    for ( Node n : flow.getChildrenUnmodifiable() ) if ( n.isManaged() ) {
       
        Bounds nodeBounds = n.getBoundsInParent();
        int length = (n instanceof Text) ? ((Text) n).getText().length() : 1;
        PathElement[] shape = flow.rangeShape( totCharSoFar, totCharSoFar+length );
        double lines = Math.max( 1.0, Math.floor( shape.length / 5 ) );
        double nodeMinY = Math.max( 0.0, nodeBounds.getMinY() );

        if ( lines > 1.0 ) {                                              // Staggered multiline text node
            if ( totLines > 0.0 ) lines--;
            totLines += lines;
        }
        else if ( nodeMinY >= prevMaxY ) {                                // Node is on next line 
            totLines++;
        }

        if ( lineMetrics.size() < totLines ) {                            // Add additional lines
           
            if ( shape.length == 0 ) {
               lineMetrics.add( new TextFlowSpan( totCharSoFar, length, nodeMinY, nodeBounds.getWidth(), nodeBounds.getHeight() ) );
                totCharSoFar += length;
            }
            else for ( int ele = (ltr?1:0); ele < shape.length; ele += 5 ) {
                double charHeight, lineMinY, segWidth;
                Point2D endPoint;

                // Calculate the segment's line's length and width up to this point
                if ( ltr ) {
                    LineTo eleLine = (LineTo) shape[ele];
                    segWidth = eleLine.getX(); lineMinY = eleLine.getY();
                    charHeight = ((LineTo) shape[ele+1]).getY() - lineMinY;
                    endPoint = new Point2D( segWidth-1, lineMinY + charHeight / 2 );
                } else {
                    MoveTo eleLine = (MoveTo) shape[ele];
                    double endX = eleLine.getX(); lineMinY = eleLine.getY();
                    charHeight = ((LineTo) shape[ele+3]).getY() - lineMinY;
                    endPoint = new Point2D( endX+1, lineMinY + charHeight / 2 );
                    segWidth = endX - ((LineTo) shape[ele+1]).getX();
                }

                // hitTest queries TextFlow layout internally and returns the position of the
                // last char (nearest endPoint) on the line, irrespective of the current Text node !
                int segLen = flow.hitTest( endPoint ).getCharIndex();
                segLen -= totCharSoFar - 1;

                if ( ele < 5 && nodeMinY < prevMaxY ) {
                    if ( ltr ) segWidth -= ((MoveTo) shape[ele-1]).getX();
                    adjustLineMetrics( segLen, segWidth, charHeight );
                }
                else {
                   lineMetrics.add( new TextFlowSpan( totCharSoFar, segLen, lineMinY, segWidth, charHeight ) );
                }

                totCharSoFar += segLen;
            }
        }
        else {
            // Adjust current line metrics with additional Text or Node embedded in this line 
            adjustLineMetrics( length, nodeBounds.getWidth(), nodeBounds.getHeight() );
            totCharSoFar += length;
        }

        prevMaxY = Math.max( prevMaxY, nodeBounds.getMaxY() );
    }
    
    lineCount = (int) totLines;
    if ( lineCount > 0 ) return lineCount;
    lineCount = -1;
    return 0;
}

This fixes [Home] and [End] behavior.

Unfortunately I haven't had time to fix the width resizing problem. It seems that a refresh layout isn't being immediately triggered, but if the text changes then it's updated.

Also have a look at [Ctrl]+[Left Arrow] and [Ctrl]+[Right Arrow] and see if you can reverse their behavior as you did for Left and Right arrow.

@vahidooo
Copy link
Author

vahidooo commented Mar 7, 2023

I am sorry. Last week I got cold and couldn't spend any time. I just tested your update on getLineCount method.
At this time I can find these things:

  1. I think [Home] and [End] are still buggy. However I think it is a mac issue again and also these shortcuts are not important now.
  2. After resizing, only the current paragraph is moved to the correct position when I start typing.
  3. When I press [Command+A], only the last paragraph gets highlighted correctly. Other paragraphs are highlighted inversely.
  4. Paragraph with more than one line causes a complete mess! Suppose that I have written a long paragraph with several lines (line wrapping is resolved as you said). When I press [Enter] the new paragraph is created behind the second line of the previous paragraph.
  5. This styling problem is still happening.

Many thanks for your time.

@vahidooo
Copy link
Author

Case 3 is resolved by replacing textFlow.prefHeightProperty().bind(heightProperty()) with prefHeightProperty().bind(textFlow.heightProperty()) in the constructor of ParagraphText.

Case 4 is also seems a mac thing. I will test it on Linux.

@Jugen
Copy link
Collaborator

Jugen commented Jan 9, 2024

JDK-8319844 may relate to this ?

@Jugen
Copy link
Collaborator

Jugen commented Mar 5, 2024

The PR for JDK-8319844 has been integrated for FX 23, so we can reassess this after its release when we have time.

@vahidooo
Copy link
Author

vahidooo commented Mar 5, 2024

Yes, we will!

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

2 participants