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

Add option to use ListView.builder instead of Container in HTML class #361

Open
jeffmikels opened this issue Jul 24, 2020 · 23 comments
Open
Labels
high-priority performance Issues related to widget performance

Comments

@jeffmikels
Copy link
Contributor

Large html documents cause scrolling problems that could be solved by using a Listview.builder on a list of top-level widgets instead of wrapping the entire document in a StyledText widget.

I'll work on a pull request.

@jeffmikels
Copy link
Contributor Author

After further review the apparent lag comes because the HtmlParser is called on every build which gets called on every scroll event. Parsing should happen only once on the first build.

@DFelten
Copy link
Contributor

DFelten commented Aug 2, 2020

Did you find a solution for this? Large html files are difficult for the performance. Especially when navigate to a page with a large html widget it's laggy. I think a ListView could maybe help here.

@DFelten
Copy link
Contributor

DFelten commented Aug 4, 2020

We found a possible solution for the problem. Maybe that helps others too. First you need to split the HTML String into parts via htmlparser from the html package. After this it's possible to insert multiple HTML widgets into a list.

In our example we're using a SliverList, but it's of course also possible with a normal list.

SliverList(
  delegate: SliverChildBuilderDelegate(
    (context, index) {
      return CustomHtml(htmlStrings[index]);
    },
    childCount: htmlStrings.length,
  ),
);

In the CustomHtml widget you just need to add the mixin AutomaticKeepAliveClientMixin. Then there is no new rebuild for the list. Only WebViews are a bit laggy because they are build as soon as the list comes into place.

This is our CustomHtml widget with the mixin:

class CustomHtml extends StatefulWidget {
  const CustomHtml(this.html);

  final String html;

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

class _CustomHtmlState extends State<CustomHtml> with AutomaticKeepAliveClientMixin {
  @override
  Widget build(BuildContext context) {
    super.build(context);
    return MediaQuery.of(context).copyWith(textScaleFactor: 1),
      child: Html(
        data: widget.html,
      ),
    );
  }

  @override
  bool get wantKeepAlive => true;
}

The MediaQuery widget is currently just the workaround for the scale.

A built-in functionality with this behavior would of course be great, since we wouldn't have to split the HTML string ourselves.

@ryan-berger
Copy link
Collaborator

This seems like a super cool idea. I really would like to implement this, but there needs to be lots more thought put into it before I am able to commit. Lazy loading HTML seems like a big TODO, and could be super breaking if the algorithm we implement doesn't work for everybody. In order to make it beneficial, we likely would have to support partial parsing of HTML, because some sort of top level splitting would only work for basic cases.

However, @jeffmikels it is concerning that we are parsing HTML each time we scroll. I will look into some sort of "parse once then done" type of thing. My guess is that is something to do with Stateless/Stateful widgets.

@jeffmikels
Copy link
Contributor Author

My workaround has been to create the Html widget outside my build function. This solves the re-parsing problem because it appears the rendering is happening on widget creation and not on every build.

I can think of a couple solutions to the problem:

  • Create a separate class called ParsedHTML or something, warn users that parsing html is an expensive operation, and recommend the parsing be done outside the build function, then the Html widget can add a parameter to optionally allow a user to pass in an already parsed object.
  • Create a const constructor for the Html widget or use Keys so that the framework knows once an Html widget is created it needs to be retained until the parent widget is disposed.

I'm getting pretty good with dart and flutter, but the specifics of when Flutter discards resources is still above my head.

@erickok
Copy link
Collaborator

erickok commented Sep 7, 2020

I don't think a List instead of Container is a very viable solution, as that would only work (or be beneficial for) cases where a clean vertical split between parts is possible. Hoewever, not parsing the html on every build seems like an obvious thing, so that would be the first step.

Having said that, if the HTML you are going to render is so big that this is visibly slow, then you are already a special case I reckon. Are you trying to render whole screens form a single blob on HTML?

@DFelten
Copy link
Contributor

DFelten commented Sep 7, 2020

In our example we are rendering post articles. Our authors write them for a WordPress system and the API sends the post content as html. First we tried a WebView, but since some limitations for Android (Not possible to show Widgets below or above a WebView) we searched for an other solution.

flutter_html works great for us when splitting the post content into parts and show them in a list view. Without the ListView longer articles would become laggy on lower or middle end devices. So we split the article. Of course alternative solutions are also welcome, but that's currently the best one. Also it's possible to show for example buttons, tables with a horizontal scroll bar, skelleton Köder for images and other stuff which are not possible within a WebView.

@ryan-berger
Copy link
Collaborator

ryan-berger commented Sep 7, 2020

@erickok I mostly do Go development, so Flutter isn't my area of expertise, but I think this will work, but feel free to tell me where I'm going wrong.

I think that if we lazy parse the HTML, and couple that with something similar to that SliverChildBuilderDelegate, you could achieve something that works well, and only renders what is necessary, and parses everything only once.

I envision that the layout renderer should be capable of calculating the size of a widget and recursively render only what is necessary. For example:

<span>Test</span>
<div>
  <ol>
    <li>One</li>
    <li>Two</li>
    <li>Three</li>
    <li>Four</li>
  </ol>
</div>
<div>
  <div> <img src="example"/> </div>
  <p>
     I'm a paragraph
  </p>
</div>

We can make the parser find 3 top level elements: a <span>, and two <div>s. Then, it takes whatever text is inside of these elements, and passes it down to be parsed again (eventually). Once the SliverList asks to render a child (in this case the top level elements), the child must then check to see if it's HTML has been parsed, and if it hasn't, it repeats the process, creating a new widget, passing the text down in, using some sort of SliverList and delegate.

This way, the <span>, and most of the first <div> would be rendered, but the image would not be yet. This would incur a high runtime cost with allocation and parsing as you scroll, but we already do much worse anyway with a massive startup cost, and even still, it may be the case that large pieces would be required to be rendered. This isn't a catch all. However, it may help with many of the cases that people are facing, specifically with rendering large documents.

If we could somehow allow partial children to be constructed, this would be even more helpful. In this example, we could only construct the first few <li> and lazy render the rest, and that would get rid of lots of the high allocation costs.

@erickok
Copy link
Collaborator

erickok commented Sep 7, 2020

Lots to consider here indeed.

Indeed if your HTML is vertically separable, then it might be very beneficial to split it up and render (using slivers in a CustomScrollView) this efficiently. Depending on your HTML though, this might very quickly be impossible. Consider a blob of HTML surrounded by a single <div>. Even if inside that div the layout is vertically separable, are we going to consider such cases? What if everything is inside a <table>? Can custom tag rendering still be supported in such a use-case?

Flutter itself is already very good at determining what part of the widget tree needs to be rebuild/redrawn, if we feed it properly.

That is not to say your proposal is not valid, starting with not re-parsing the html on every build. And indeed if we could detect that a vertical split is possible, using a lazy child-building widget can be a great improvement.

@DFelten We use this library for a very similar use-case, though it's a bummer that your CMS (at least with the default Wordpress API) cannot produce something better than a blob of html. 😞

@DFelten
Copy link
Contributor

DFelten commented Feb 6, 2021

We're currently using the ListBuilder solution in our app for iOS and Android. It's working really well, but some kind of asynchronous builder would be a better solution. The alternative library flutter_widget_from_html uses such builder and great caching. Even large html documents are working without lags.

Since there is so much activity here at the moment, maybe this is the right time to note this problem again. Unfortunately, large HTML documents have still a very poor performance.

The initial jank could be improved by asynchronous building, but an alternative solution needs still have to be found for large documents. Maybe caching is here solution.

@ryan-berger
Copy link
Collaborator

I agree @DFelten, I really need to find some time to look into this. I've followed flutter_widget_from_html for some time now and they have been doing some great work and definitely have become a competitor.

I know that @Sub6Resources refactored with a new parser right before he left, but I think this library badly in need of some more rewriting, purging of technical debt, and performance fixes.

Contributions are always welcome, and I've started delegating to @erickok for code reviews/PRs and soon version updates so that my college schedule won't get in the way of the library moving forward.

@erickok
Copy link
Collaborator

erickok commented Feb 6, 2021

Certainly, if we get this to work properly (either via a ListBuilder or some other lazy method) it would be a great performance improvement! I would love for this to work. I just think it would be very hard to do while still supporting all possible html cases. We should definitely take a look at that library and how it does lazy building. I don't think we should aim to replace/be a full fledged browser, but any (speed) improvement that is reliable and maintainable is welcome of course.

@DFelten
Copy link
Contributor

DFelten commented Feb 6, 2021

Contributions are always welcome, and I've started delegating to @erickok for code reviews/PRs and soon version updates so that my college schedule won't get in the way of the library moving forward.

Many of our customizations are unfortunately very specific. For example, depending on the html tag, we display WebViews via button as a separate sheet. For performance, we currently use the ListView solution mentioned above, but it offers other problems. Table of Contents, for example, must be stored in a separate list for the internal link jumps to work. Therefore, another more generic solution would be more helpful here.

Due to the very specific adjustments, contributions are currently difficult from our current code. But maybe we will find a better solution for the performance issue.

Certainly, if we get this to work properly (either via a ListBuilder or some other lazy method) it would be a great performance improvement! I would love for this to work. I just think it would be very hard to do while still supporting all possible html cases. We should definitely take a look at that library and how it does lazy building. I don't think we should aim to replace/be a full fledged browser, but any (speed) improvement that is reliable and maintainable is welcome of course.

Yeah, ListViews are unfortunately not the best solution. The problem here is for example that the height of the article is unknown and so a ScrollBar is really buggy. I'm not exactly sure what flutter_widget_from_html is doing, but it seems they do some sort of caching for the widgets (here I asked him the reason for his great performance).

A browser and showing (large) wordpress posts is something different I think :) It's more a replacement of a WebView for html content with more customizable options.

@erickok
Copy link
Collaborator

erickok commented Feb 7, 2021

So he mentions two things about performance.

The HTML is parsed only once. We should also do that. I remember seeing some issue about this, but have to look for it. Should not even be that hard, of we don't do this already.

I do remember starting in that issue already though that flutter is very fast in dealing with widgets and smart in what it renders. If we work on performance we should only do that if we can test the performance reliably too, in a representative example.

Which comes to his second point, keeping taps in performance in automated tests. A good idea. Testing is... Severely underdeveloped here, I agree.

@tneotia
Copy link
Collaborator

tneotia commented Mar 7, 2021

I did try something very basic with a ListView.builder:

    dom.Document document = parseHTML(htmlData);
    StyledElement lexedTree = lexDomTree(
      document,
      customRender.keys.toList(),
      blacklistedElements,
      navigationDelegateForIframe,
    );
    StyledElement inlineStyledTree = applyInlineStyles(lexedTree);
    StyledElement customStyledTree = _applyCustomStyles(inlineStyledTree);
    StyledElement cascadedStyledTree = _cascadeStyles(customStyledTree);
    StyledElement cleanedTree = cleanTree(cascadedStyledTree);
    List<InlineSpan> parsedTreeList = [];
    print(document.getElementsByTagName("body")[0].children.length);
    cleanedTree.children[0].children.firstWhere((element) => element.name == "body").children.forEach((element) {
      parsedTreeList.add(parseTree(
        RenderContext(
          buildContext: context,
          parser: this,
          style: Style.fromTextStyle(Theme.of(context).textTheme.bodyText2!),
        ),
        element,
      ));
    });
    // This is the final scaling that assumes any other StyledText instances are
    // using textScaleFactor = 1.0 (which is the default). This ensures the correct
    // scaling is used, but relies on https://github.com/flutter/flutter/pull/59711
    // to wrap everything when larger accessibility fonts are used.
    return LayoutBuilder(
      builder: (context, constraints) {
        return Container(
          height: constraints.maxHeight,
          child: ListView.builder(
            shrinkWrap: true,
            itemBuilder: (bc, index) {
              return StyledText(
                textSpan: parsedTreeList[index],
                style: cleanedTree.style,
                textScaleFactor: MediaQuery.of(context).textScaleFactor,
                renderContext: RenderContext(
                  buildContext: context,
                  parser: this,
                  style: Style.fromTextStyle(Theme.of(context).textTheme.bodyText2!),
                ),
              );
            },
            itemCount: parsedTreeList.length,
          ),
        );
      }
    );

in the build method (you need to remove the SingleChildScrollView in the example app. With this and the long html document linked in the flutter_widget_from_html issue, scrolling is visibly smoother, and I mean much smoother - the difference seems like 20fps vs 60fps.

However, there are 2 issues:

  1. I cannot get the styling to work because it only builds widgets for the children for the HTML <body>, so it does not know to apply
Style(
        margin: EdgeInsets.all(8.0),
        display: Display.BLOCK,
);

to all the elements. Its likely something trivial to fix though.

  1. Inline text breaks. Take this string for example: Solve for <var>x<sub>n</sub></var>: log<sub>2</sub>(<var>x</var><sup>2</sup>+<var>n</var>) = 9<sup>3</sup>.
    The loop through the HTML body is going to go through var -> sub -> var -> sup -> var -> sup rather than treating the whole inline text as one element. If you surround that in <p> then it works great, but this is something important to think about when designing a listview renderer.

@marcjoha
Copy link

marcjoha commented Aug 2, 2021

I'm seeing significant yank for big HTML pages.

@tneotia you don't happen to have that code in a fork somewhere? Not supporting inline code might be a trade-off I can live with.

@tneotia
Copy link
Collaborator

tneotia commented Aug 2, 2021

I don't have that code in a fork but you should be able to clone the repo and simply change the initState of html_parser.dart to the code I posted above.

@abhi36456
Copy link

No solution is there for this issue? @Sub6Resources

@Sembauke
Copy link

Sembauke commented Dec 15, 2021

So for the ones wanting to lazy load your big html files this is how I did it.

I started by making a method called htmlHandler this uses the fluttter/html package not flutter/flutter_html it should already by installed. I used the build-in HtmlParser.parseHTML() function to parse the html into Elements. Next I made sure to get the children from the body from the result variable and use their outerHTML .

Next I iterated over the length of the children which added Html() widgets from the flutter/flutter_html package to the Widget List.

The htmlWidgetBuilder returns an instance of the Html() widget.

  static List<Widget> htmlHandler(html, context, article) {
    var result = HtmlParser.parseHTML(html);

    List<Widget> elements = [];

    elements.add(NewsArticlePostHeader(article: article));

    for (int i = 0; i < result.body!.children.length; i++) {
      elements
          .add(htmlWidgetBuilder(result.body!.children[i].outerHtml, context));
    }
    return elements;
  }

To show your html() widgets you need to make use of a list view. When initializing the Listview make sure to call the function you made (htmlHandler) in a Widget of type ListView and return it's length in the itemCount parameter.

ListView lazyLoadHtml(String html, BuildContext context,
    NewsArticlePostViewModel model, Article article) {
  var htmlToList = model.initLazyLoading(html, context, article);
  return ListView.builder(
      shrinkWrap: true,
      itemCount: htmlToList.length,
      physics: const ClampingScrollPhysics(),
      itemBuilder: (BuildContext context, int i) {
        return Row(
          children: [
            Expanded(child: htmlToList[i]),
          ],
        );
      });

Please Note that when you make use of a SingleChildScrollView or put this ListView inside another ListView the lazy loading will not work and everything will be loaded at once.

If you have any widgets that are displayed above the ListView Like an image or a title you can add them as the first item in the array of the htmlHandler function. You might make use of a custom class to add multiple widgets to the List.

Hope this clears thing up for you guys.

Here a link to my pull-request

Thanks to @erickok for leading me to this issue.

@tneotia
Copy link
Collaborator

tneotia commented Dec 15, 2021

Is this true lazy loading? It seems like you lazy load each individual Html widget in your list, rather than lazy-loading the HTML content within those individual Html widgets.

@Sembauke
Copy link

Yep that was the best solution I could come up with.

@Sembauke
Copy link

Is this true lazy loading? It seems like you lazy load each individual Html widget in your list, rather than lazy-loading the HTML content within those individual Html widgets.

I do wonder if there is a better way of doing this, there probably is.

This would probably not work for everyone.

@erickok
Copy link
Collaborator

erickok commented Dec 16, 2021

I feel there is a lot of misconception on 'lazy loading' the html rendering or even why flutter_html is 'slow'.

I can tell that the actual parsing of the html string is not slow,. Or rather, it can be a bit slow but you can do this on an isolate even and then feed the dom to flutter_html. Next, building a widget tree from the html Elements is not really slow by itself.

What people really describe as 'slow' is the actual scrolling behaviour. This is indeed bad if you have a (very) large blob of html, because flutter_html effectively creates what in the end is a single gigantic Text widget and indeed Flutter will render this full thing at once.

So 'lazy loading' as was suggested in this ticket is targeted towards not ending up with a single giant Text widget but - how you work with long lists - chopping it up and providing the parts in a ListView.builder (or similar) which would only have to render small(er) bits. The method suggested by @Sembauke is an example of this as it the experiment from @tneotia . It is also what the flutter_widget_from_html package has implemented (daohoangson/flutter_widget_from_html#484) in their renderMode. That's what we want as well.

So yes @Sembauke 's is lazy loading in the sense that Flutter can render much smaller blocks of html at once and threfore is will give a vastly superior experience over the out-of-the-box method for this specific use-case.

The tricky part here is to chop up the body and still render correctly as well as support all different types of html. If you know that you have discrete blocks that can be rendered underneath each other, then great! Chop it up. To do this in the library though we have to handle many potential cases. For example, what it the whole thing is wrapped in a single <div>? Maybe we can be smart about it, but at least you'd have to properly pass through any formatting and styling. What if the whole thing is in one giant <table>? Okay maybe we in this case we just do not 'optimize'; so what do or don't we support? Anchors between the different parts need to work too. And most tricky I suspect: text flowing needs to work properly.

This sounds like a solvable problem but I am afraid there is so much to do before we can attempt this that I don't think we will have something official for a while. Perhaps we can instead offer in the README some pointers on how to do this manually, given you know something about the html you want to render.

@Sub6Resources Sub6Resources added this to the Priority Goals milestone May 17, 2023
@Sub6Resources Sub6Resources added the performance Issues related to widget performance label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority performance Issues related to widget performance
Projects
Status: Todo
Development

No branches or pull requests

9 participants