singerdmx / flutter-quill

Rich text editor for Flutter
https://pub.dev/packages/flutter_quill
MIT License
2.58k stars 833 forks source link

checkBox does not work in RTL languages, when you try to check the checkbox it does not check #2085

Open jonasbernardo opened 3 months ago

jonasbernardo commented 3 months ago

Is there an existing issue for this?

Flutter Quill version

10.1.2

Steps to reproduce

checkBox does not work in RTL languages, when you try to check the checkbox it does not check

Expected results

checkBox does not work in RTL languages, when you try to check the checkbox it does not check

Actual results

checkBox does not work in RTL languages, when you try to check the checkbox it does not check

Code sample

Code sample ```dart [Paste your code here] ```

Additional Context

Screenshots / Video demonstration [Upload media here]
Logs ```console [Paste your logs here] ```
jonasbernardo commented 3 months ago

https://github.com/user-attachments/assets/a9297c03-e730-4d99-b429-97ad45a6bce5

jonasbernardo commented 3 months ago

@CatHood0 take a look at the video above

jonasbernardo commented 3 months ago

I looked in the flutter inspector, and look how the checkboxpoint class is, I don't know how to fix this, can anyone help me? @CatHood0 Captura de tela de 2024-08-02 12-50-37

jonasbernardo commented 3 months ago

@AtlasAutocode @singerdmx

CatHood0 commented 3 months ago

I'll take a look. Give me some time. The issue is related with the harcoded widgets on the editor.

jonasbernardo commented 3 months ago

right, it's because after the update the checkboxes for RTL languages ​​became unusable

CatHood0 commented 3 months ago

I notice the issue is more related from the Rendering. I still trying to fix it.

jonasbernardo commented 2 months ago

How is the progress going? Would it be possible to revert this change so it return to work as before?

CatHood0 commented 2 months ago

I apologize, time is what I have had the least of so far. I will take the time to be able to solve this since I have to understand how the editor renders the checkbox and solve the root problem.

CatHood0 commented 1 month ago

Update: This error seems to be more of a Flutter issue because the objects we render go to the right place, but it seems that widgets like GestureDetector, or InkWell no matter how much we force them, will not be able to move from the left.

However, I'm still trying to figure out how to solve this issue.

CatHood0 commented 1 month ago

I'm still unable to fix this issue. The InkWell of the check can be moved even if you specify the Offset where it should be displayed. I think could be a issue from Flutter. When i have more time i try to fix again.

jonasbernardo commented 1 month ago

It would be good to revert the changes you made, because the checkboxes in rtl languages no longer work

CatHood0 commented 1 month ago

@EchoEllet what do you think? should we revert this by now?

EchoEllet commented 1 month ago

Yes, we should revert any related changes that caused related regression after the review.

2060 and #2063 Fix RTL issues as the title says, so it's supposed to fix this issue and related? The branch name called fix ltr (opposite of rtl)?

I haven't seen any bug reports regarding RTL support before. Is it related to another fix or feature?

jonasbernardo commented 1 month ago

I evaluate it better, it really got better with the change, because before the enumerated lists were in ltr even though they were in an rtl language, it fixed a lot of things but the checkbox was still missing.

jonasbernardo commented 1 month ago

thinking better it is better with this change you made than the previous one, do not do the reversal, I did some tests here

EchoEllet commented 1 month ago

Thank you for the insight. Have not tested it yet, if this is the case then we should not revert, instead improve the fix.

CatHood0 commented 1 month ago

The branch name called fix ltr (opposite of rtl)?

You shouldn't pay attention to the names of the branches I create, because I name them first and then analyze the root of the problem that the branch is going to solve.

Take an example in Feat: support for dynamic placeholders the branch is called improve_headers because at first I was only going to focus on that, but then I ended up noticing that a better implementation could be created and it became a name that has nothing to do with its main objective

CatHood0 commented 1 month ago

I have not been able to solve this problem because InkWell or a GestureDetector cannot be moved even by forcing them. I don't know why these widgets cannot be moved using the offset into text_line (RenderEditableTextLine draws the leading of the lines)

EchoEllet commented 1 month ago

You shouldn't pay attention to the names of the branches I create, because I name them first and then analyze the root of the problem that the branch is going to solve.

I assumed I misunderstood something since a fix regarding some RTL issues had already made, there was a minor issue that caused on LTR that was fixed after that.

should we revert https://github.com/singerdmx/flutter-quill/pull/2060 by now?

I'm still not quite sure why you suggested reverting your fix when your fix is not a regression that caused this issue.

CatHood0 commented 1 month ago

I'm still not quite sure why you suggested reverting your fix when your fix is not a regression that caused this issue.

I thought it deserved a revert since it affects users to a certain extent (not too much). But now I think more calmly, it's true, it's a minor problem that only affects under certain conditions.

jonasbernardo commented 1 month ago

I was trying to find the problem, the code gives an offset in the paint child, but this offset is not working to change the position of the clickable area, maybe there is a way to give an offset to the clickable area

if (textDirection == TextDirection.ltr) { final parentData = _leading!.parentData as BoxPar final effectiveOffset = offset + parentData.offset; context.paintChild(_leading!, effectiveOffset); } else { final parentData = _leading!.parentData as BoxParentData; final effectiveOffset = offset + parentData.offset; context.paintChild( _leading!, Offset( size.width - _leading!.size.width, effectiveOffset.dy, ), ); }

essa entData;

jonasbernardo commented 1 month ago

I think this video can help?: https://youtu.be/svb41OLzCDY?si=Dg7EHrHZdj6IximQ&t=1516

jonasbernardo commented 1 month ago

@CatHood0 @singerdmx @EchoEllet I managed to change the position of the clickable area, now I just need to be able to calculate the correct horizontal offset, here is the method ready the -90 in the offset made the clickable area more to the right

Above the paint method inside class RenderEditableTextLine.class

@override
bool hitTest(BoxHitTestResult result, {required Offset position}) {
  final attrs = line.style.attributes;
  final attribute =
      attrs[Attribute.list.key] ?? attrs[Attribute.codeBlock.key];
  //Here
  final isCheck =
      attribute == Attribute.checked || attribute == Attribute.unchecked;

  if (isCheck) {
    final parentData = _leading!.parentData as BoxParentData;
    final effectiveOffset = position + parentData.offset;
    return super.hitTest(result,
        position: Offset(effectiveOffset.dx - 90, effectiveOffset.dy));
  } else {
    return super.hitTest(result, position: position);
  }
}
CatHood0 commented 1 month ago

Thanks for taking the efforts and the time. I'm unable to do something by some personal reasons. Are you going to fix it? If not, give me some time and i will fix it (using the new info. Thanks for it)

CatHood0 commented 1 month ago

@jonasbernardo Can you show me your results? I mean a video showing how the position of the clickable area changes when you make your changes.

I ask you this because I already tested these changes (i made some modifications to be able to put it on the right based on the operating system's text direction), and i couldn't notice any relevant change, beyond that now when you hover over the checkbox it doesn't change (which normally doesn't happen).

Let's keep in mind that when we override the hitTest of RenderEditableTextLine we are applying it to the ENTIRE line. That is, we could be making a mistake in which area we should touch. Maybe we should look at hitTestChildren which does the same thing. I say this because I'm basing it on what I tested and obtained. It's possible that i'm even confusing myself.

jonasbernardo commented 1 month ago

here is the video, depending on the offset value, the clickable area changes if (isCheck) { final parentData = _leading!.parentData as BoxParentData; final effectiveOffset = position + parentData.offset; return super.hitTest(result, position: Offset(effectiveOffset.dx - 90, effectiveOffset.dy)); } else { return super.hitTest(result, position: position); }

https://github.com/user-attachments/assets/ec77e8f9-1976-493b-b441-3858dde2f360

jonasbernardo commented 1 month ago

for example if I put -100, it moves more to the right

jonasbernardo commented 1 month ago

@CatHood0

CatHood0 commented 1 month ago

We need to take care more about this. Adding hitTest to the RenderEditableTextLine could cause more issues. We need to test this on all platforms. I saw your example works on android as expected, but, for desktop could have some unexpected behaviors. Let me take some time to debug it. Thanks for provide the example video.

jonasbernardo commented 1 month ago

Ok, If you send me the ready code I can test it on Android and IOS

jonasbernardo commented 3 weeks ago

any news?