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

Editable manages the viewport display #338

Merged
merged 8 commits into from
Jul 14, 2024
Merged

Conversation

amantoux
Copy link
Member

@amantoux amantoux commented Apr 30, 2024

This PR is an attempt to mimic Flutter's Editable that behaves as a viewport (invisible elements are clipped out).
The benefit here is that it eases future optimizations that would prevent from rendering element that are not visible.
It also allows us to avoid the need to define a custom SingleChildScrollView

@amantoux amantoux marked this pull request as draft April 30, 2024 17:41
@amantoux
Copy link
Member Author

amantoux commented May 9, 2024

  • All tests pass
  • Support no scroll
  • SingleChildScrollView is removed

@amantoux amantoux force-pushed the review_editable_scrollable branch 2 times, most recently from 484c9fc to d94330d Compare May 11, 2024 09:11
@amantoux amantoux force-pushed the review_editable_scrollable branch from d94330d to fe91995 Compare June 18, 2024 15:54
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 10 lines in your changes missing coverage. Please review.

Project coverage is 87.97%. Comparing base (178d2bb) to head (87d534f).
Report is 5 commits behind head on master.

Files Patch % Lines
packages/fleather/lib/src/widgets/editor.dart 90.32% 6 Missing ⚠️
packages/fleather/lib/src/rendering/editor.dart 93.61% 3 Missing ⚠️
...kages/fleather/lib/src/widgets/text_selection.dart 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   87.90%   87.97%   +0.07%     
==========================================
  Files          64       61       -3     
  Lines       10382    11105     +723     
==========================================
+ Hits         9126     9770     +644     
- Misses       1256     1335      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amantoux amantoux requested a review from Amir-P June 20, 2024 09:44
@amantoux amantoux marked this pull request as ready for review June 20, 2024 09:44
Copy link
Member

@Amir-P Amir-P left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works mostly fine but I'm facing problem when using the editor or field in a scrolling view without limiting its height. Here is an example which works fine with master:

import 'package:fleather/fleather.dart';
import 'package:flutter/material.dart';

void main() {
  runApp(const FleatherApp());
}

class FleatherApp extends StatelessWidget {
  const FleatherApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) => MaterialApp(
        debugShowCheckedModeBanner: false,
        theme: ThemeData.light(),
        darkTheme: ThemeData.dark(),
        title: 'Fleather - rich-text editor for Flutter',
        home: HomePage(),
      );
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  _HomePageState createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(elevation: 0, title: Text('Fleather Demo')),
      body: SingleChildScrollView(
        child: Padding(
          padding: const EdgeInsets.all(8.0),
          child: Column(
            crossAxisAlignment: CrossAxisAlignment.start,
            children: [
              _Bubble('Test Test Test'),
              Align(
                alignment: Alignment.topRight,
                child: _Bubble('Test Test Test Test Test Test Test Test'),
              ),
              _Bubble('Test Test Test'),
              Align(
                alignment: Alignment.topRight,
                child: _Bubble('Test Test Test Test Test Test Test Test'),
              ),
              _Bubble('Test Test Test'),
              Align(
                alignment: Alignment.topRight,
                child: _Bubble('Test Test Test Test Test Test Test Test'),
              ),
            ],
          ),
        ),
      ),
    );
  }
}

class _Bubble extends StatelessWidget {
  final String text;

  const _Bubble(this.text);

  @override
  Widget build(BuildContext context) {
    return Container(
      decoration: BoxDecoration(
        color: Colors.grey.shade300,
        borderRadius: BorderRadius.circular(8),
      ),
      constraints: BoxConstraints(maxWidth: 200),
      padding: EdgeInsets.all(8),
      child: IntrinsicWidth(
        child: FleatherEditor(
            controller: FleatherController(
                document: ParchmentDocument.fromJson([
          {'insert': '$text\n'}
        ]))),
      ),
    );
  }
}

@amantoux
Copy link
Member Author

It works mostly fine but I'm facing problem when using the editor or field in a scrolling view without limiting its height. Here is an example which works fine with master

Thanks @Amir-P for the example.
As I see it the previous behavior was incorrect.
By default, FleatherEditor is built with scrollable set to true. It should attempt to expand as much as possible (it seems to be the default behavior with scrollable in general, isn't it?). To achieve the desired behavior, one should pass scrollable: false to the constructor.
The problem is the presence of an assert that compels scrollController to be non null when scrollable = false

All in all I propose to remove the assert and from now, to implement the desired behavior, you should pass scrollable: false.

WDYT?

@Amir-P
Copy link
Member

Amir-P commented Jun 25, 2024

@amantoux TBH I didn't check the code at that time. Your suggestion sounds reasonable to me. Anyway, I found another problem that might be good to look into.
It's possible to use a multiline TextField inside of a scrollable without any problem and it manages to bring into view the selected part of text by manipulating the parent scrolling view position. Doing the same with FleatherEditor or FleatherField results in BoxConstraints forces an infinite height error and with scrollable: false although laid out correctly, fails to bring into view the selected part of text. As far as I know the TextField uses a Scrollable in both multiline and non-multiline mode.

import 'dart:convert';
import 'dart:io';

import 'package:fleather/fleather.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(const FleatherApp());
}

class FleatherApp extends StatelessWidget {
  const FleatherApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) => MaterialApp(
        debugShowCheckedModeBanner: false,
        theme: ThemeData.light(),
        darkTheme: ThemeData.dark(),
        title: 'Fleather - rich-text editor for Flutter',
        home: HomePage(),
      );
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  _HomePageState createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  final FocusNode _focusNode = FocusNode();
  FleatherController? _controller;

  @override
  void initState() {
    super.initState();
    if (kIsWeb) BrowserContextMenu.disableContextMenu();
    _initController();
  }

  @override
  void dispose() {
    super.dispose();
    if (kIsWeb) BrowserContextMenu.enableContextMenu();
  }

  Future<void> _initController() async {
    try {
      final result = await rootBundle.loadString('assets/welcome.json');
      final doc = ParchmentDocument.fromJson(jsonDecode(result));
      _controller = FleatherController(document: doc);
    } catch (err, st) {
      print('Cannot read welcome.json: $err\n$st');
      _controller = FleatherController();
    }
    setState(() {});
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(elevation: 0, title: Text('Fleather Demo')),
      body: _controller == null
          ? Center(child: const CircularProgressIndicator())
          : SingleChildScrollView(
              child: Column(
                children: [
                  FleatherToolbar.basic(controller: _controller!),
                  Divider(height: 1, thickness: 1, color: Colors.grey.shade200),
                  // FleatherEditor(
                  //   controller: _controller!,
                  //   focusNode: _focusNode,
                  //   padding: EdgeInsets.only(
                  //     left: 16,
                  //     right: 16,
                  //     bottom: MediaQuery.of(context).padding.bottom,
                  //   ),
                  //   onLaunchUrl: _launchUrl,
                  //   maxContentWidth: 800,
                  //   embedBuilder: _embedBuilder,
                  //   spellCheckConfiguration: SpellCheckConfiguration(
                  //       spellCheckService: DefaultSpellCheckService(),
                  //       misspelledSelectionColor: Colors.red,
                  //       misspelledTextStyle:
                  //           DefaultTextStyle.of(context).style),
                  // ),
                  TextField(
                    controller: TextEditingController(
                        text: _controller!.document.toPlainText()),
                    maxLines: null,
                  ),
                ],
              ),
            ),
    );
  }

  Widget _embedBuilder(BuildContext context, EmbedNode node) {
    if (node.value.type == 'icon') {
      final data = node.value.data;
      // Icons.rocket_launch_outlined
      return Icon(
        IconData(int.parse(data['codePoint']), fontFamily: data['fontFamily']),
        color: Color(int.parse(data['color'])),
        size: 18,
      );
    }

    if (node.value.type == 'image') {
      final sourceType = node.value.data['source_type'];
      ImageProvider? image;
      if (sourceType == 'assets') {
        image = AssetImage(node.value.data['source']);
      } else if (sourceType == 'file') {
        image = FileImage(File(node.value.data['source']));
      } else if (sourceType == 'url') {
        image = NetworkImage(node.value.data['source']);
      }
      if (image != null) {
        return Padding(
          // Caret takes 2 pixels, hence not symmetric padding values.
          padding: const EdgeInsets.only(left: 4, right: 2, top: 2, bottom: 2),
          child: Container(
            width: 300,
            height: 300,
            decoration: BoxDecoration(
              image: DecorationImage(image: image, fit: BoxFit.cover),
            ),
          ),
        );
      }
    }

    return defaultFleatherEmbedBuilder(context, node);
  }
}

@amantoux
Copy link
Member Author

amantoux commented Jul 7, 2024

@amantoux TBH I didn't check the code at that time. Your suggestion sounds reasonable to me. Anyway, I found another problem that might be good to look into. It's possible to use a multiline TextField inside of a scrollable without any problem and it manages to bring into view the selected part of text by manipulating the parent scrolling view position. Doing the same with FleatherEditor or FleatherField results in BoxConstraints forces an infinite height error and with scrollable: false although laid out correctly, fails to bring into view the selected part of text. As far as I know the TextField uses a Scrollable in both multiline and non-multiline mode.

@Amir-P Here is the behavior I propose:
If scrollable is true:

  • if expand is true, the editor fills all the space available in parent (therefore will cause an assert error if place in a Column or a Scrollable). If text inside is greater than the available space, the scroll is effective.
  • if expand is false, the editor behave as a shrink wrap ListView: it takes only the required space but can grow indefinitely as text is being added to the document. If text inside overflows the available space, the scroll is effective.

If scrollable is false:

  • if expand is true, the editor takes all space available in parent. If text inside is greater than the available space, it is clipped.
  • if expand is false, the editor takes only required space, if the latter is bigger that the available space in parent, text will be clipped if greater than available space

WDYT?

@Amir-P
Copy link
Member

Amir-P commented Jul 10, 2024

Sorry Alan I was busy. Will try to get back to you today. @amantoux

@Amir-P
Copy link
Member

Amir-P commented Jul 13, 2024

The behavior you've described sounds good to me, Alan. Do you need to make changes? @amantoux

@amantoux
Copy link
Member Author

The behavior you've described sounds good to me, Alan. Do you need to make changes? @amantoux

@Amir-P Yes I pushed the changes

@amantoux amantoux requested a review from Amir-P July 14, 2024 08:43
Copy link
Member

@Amir-P Amir-P left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks.

@amantoux amantoux merged commit ce6bb18 into master Jul 14, 2024
4 checks passed
@Amir-P Amir-P deleted the review_editable_scrollable branch July 18, 2024 09:39
@kane-knowby
Copy link

@amantoux TBH I didn't check the code at that time. Your suggestion sounds reasonable to me. Anyway, I found another problem that might be good to look into. It's possible to use a multiline TextField inside of a scrollable without any problem and it manages to bring into view the selected part of text by manipulating the parent scrolling view position. Doing the same with FleatherEditor or FleatherField results in BoxConstraints forces an infinite height error and with scrollable: false although laid out correctly, fails to bring into view the selected part of text. As far as I know the TextField uses a Scrollable in both multiline and non-multiline mode.

import 'dart:convert';
import 'dart:io';

import 'package:fleather/fleather.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(const FleatherApp());
}

class FleatherApp extends StatelessWidget {
  const FleatherApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) => MaterialApp(
        debugShowCheckedModeBanner: false,
        theme: ThemeData.light(),
        darkTheme: ThemeData.dark(),
        title: 'Fleather - rich-text editor for Flutter',
        home: HomePage(),
      );
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  _HomePageState createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  final FocusNode _focusNode = FocusNode();
  FleatherController? _controller;

  @override
  void initState() {
    super.initState();
    if (kIsWeb) BrowserContextMenu.disableContextMenu();
    _initController();
  }

  @override
  void dispose() {
    super.dispose();
    if (kIsWeb) BrowserContextMenu.enableContextMenu();
  }

  Future<void> _initController() async {
    try {
      final result = await rootBundle.loadString('assets/welcome.json');
      final doc = ParchmentDocument.fromJson(jsonDecode(result));
      _controller = FleatherController(document: doc);
    } catch (err, st) {
      print('Cannot read welcome.json: $err\n$st');
      _controller = FleatherController();
    }
    setState(() {});
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(elevation: 0, title: Text('Fleather Demo')),
      body: _controller == null
          ? Center(child: const CircularProgressIndicator())
          : SingleChildScrollView(
              child: Column(
                children: [
                  FleatherToolbar.basic(controller: _controller!),
                  Divider(height: 1, thickness: 1, color: Colors.grey.shade200),
                  // FleatherEditor(
                  //   controller: _controller!,
                  //   focusNode: _focusNode,
                  //   padding: EdgeInsets.only(
                  //     left: 16,
                  //     right: 16,
                  //     bottom: MediaQuery.of(context).padding.bottom,
                  //   ),
                  //   onLaunchUrl: _launchUrl,
                  //   maxContentWidth: 800,
                  //   embedBuilder: _embedBuilder,
                  //   spellCheckConfiguration: SpellCheckConfiguration(
                  //       spellCheckService: DefaultSpellCheckService(),
                  //       misspelledSelectionColor: Colors.red,
                  //       misspelledTextStyle:
                  //           DefaultTextStyle.of(context).style),
                  // ),
                  TextField(
                    controller: TextEditingController(
                        text: _controller!.document.toPlainText()),
                    maxLines: null,
                  ),
                ],
              ),
            ),
    );
  }

  Widget _embedBuilder(BuildContext context, EmbedNode node) {
    if (node.value.type == 'icon') {
      final data = node.value.data;
      // Icons.rocket_launch_outlined
      return Icon(
        IconData(int.parse(data['codePoint']), fontFamily: data['fontFamily']),
        color: Color(int.parse(data['color'])),
        size: 18,
      );
    }

    if (node.value.type == 'image') {
      final sourceType = node.value.data['source_type'];
      ImageProvider? image;
      if (sourceType == 'assets') {
        image = AssetImage(node.value.data['source']);
      } else if (sourceType == 'file') {
        image = FileImage(File(node.value.data['source']));
      } else if (sourceType == 'url') {
        image = NetworkImage(node.value.data['source']);
      }
      if (image != null) {
        return Padding(
          // Caret takes 2 pixels, hence not symmetric padding values.
          padding: const EdgeInsets.only(left: 4, right: 2, top: 2, bottom: 2),
          child: Container(
            width: 300,
            height: 300,
            decoration: BoxDecoration(
              image: DecorationImage(image: image, fit: BoxFit.cover),
            ),
          ),
        );
      }
    }

    return defaultFleatherEmbedBuilder(context, node);
  }
}

Hi, I've been having an issue and it seems to relate to the behaviour listed above. I'm trying to use FleatherField like I would a normal TextFormField, but the cursor always ends up underneath the keyboard. Other than this one issue I'm loving Fleather, but this is unfortunately blocking me, as we have multiple editable fields in a scrollable list, so they need to respect keyboard correctly.

Reading the above I hoped the issue was resolved but doesn't seem to be from my testing. @Amir-P was the issue you found with scrolling fixed as part of this PR?

@Amir-P
Copy link
Member

Amir-P commented Sep 4, 2024

Hey @kane-knowby, Thanks for bringing this to our attention. The issue you are referring to wasn't fixed with this PR since I realized that we had the issue even before this PR. But it's something that we need to address. Can you please create an issue for it?

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

Successfully merging this pull request may close these issues.

3 participants