Add support for anchor links within the same document #535
Conversation
| parser: this, | ||
| style: Style.fromTextStyle(Theme.of(context).textTheme.bodyText2), | ||
| ), | ||
| return Scrollable( |
There was a problem hiding this comment.
I wonder what the impact of this is. 'Suddenly' we are now always a scrollable container, even in shrinkWrap mode. I don't quite know yet if this really is a problem, but it surely impacts how our Html widget is to be used right?
There was a problem hiding this comment.
I don't think shrinkWrap mode affects it, the scrollable is the same API that a Column or ListView uses, so it can still be used in other Flex widgets with no problems. As the docs say:
Scrollable implements the interaction model for a scrollable widget, including gesture recognition, but does not have an opinion about how the viewport, which actually displays the children, is constructed.
Therefore I don't think its a huge issue.
| @@ -247,7 +257,7 @@ class HtmlParser extends StatelessWidget { | |||
| RenderContext newContext = RenderContext( | |||
| buildContext: context.buildContext, | |||
| parser: this, | |||
| style: context.style.copyOnlyInherited(tree.style), | |||
| style: context.style?.copyOnlyInherited(tree.style) ?? Style.fromTextStyle(Theme.of(context.buildContext).textTheme.bodyText2), | |||
There was a problem hiding this comment.
Its not, this was fixed by a different PR of mine. It can be removed before merging, this was just temporary to avoid a "red screen of death" upon launching the app.
| final Map<String, Style> style; | ||
| final Map<String, CustomRender> customRender; | ||
| final Map<ImageSourceMatcher, ImageRender> imageRenders; | ||
| final List<String> blacklistedElements; | ||
| final NavigationDelegate navigationDelegateForIframe; | ||
| final Map<String, BuildContext> buildContexts = {}; |
There was a problem hiding this comment.
So this is sort of your list of points in the document where you can link to, right? I wonder though if we could use Flutter's keys instead of maintaining our own list? That would also dramatically reduce the changes required in this PR.
In StyledElement we already have the elementId it seems. The StyledText that we eventually will create for (all? almost all?) tags could set its StatelessWidget key. With this key (which I think can be a ValueKey, as ids shoud be unique in html?) you can retrieve the currentContext which give you the context you wanted to scroll to. Or am I missing something?
You wrote that you tried this, but then tried to access the renderobject which was always null. Can't we do Scrollable.ensureVisible(key.currentContext)?
There was a problem hiding this comment.
Yeah I know, it would be the ideal solution and its why I was not super pleased with what turned out here. It needs to be a GlobalKey because otherwise you cannot access currentContext.
I may have wrote incorrectly, but it was actually currentContext coming back as null which was more concerning. That makes me think the key wasn't even applying to the widgets somehow?? I used Flutter Inspector to check this and sure enough every widget had a different key ID assigned to it than it should have. I also checked my algorithm by printing the key ID right before the widget was rendered and it did print the correct ID, but still the widget itself had a different key.
|
|
||
| /// This class is a placeholder class so that the renderer can distinguish | ||
| /// between WidgetSpans and TextSpans | ||
| class CustomTextSpan extends WidgetSpan { |
There was a problem hiding this comment.
But if this is always a WidgetSpan, does this not go straight into the problem that inline spans are not - well - inline any more?
There was a problem hiding this comment.
I'm not super familiar with the technicalities of the InlineSpan in Flutter, but at least visually, there was no difference with this implementation (for the example app HTML at least, with some extra stuff included to test links thoroughly). I included a parameter on this class for the original InlineSpan that would have been rendered beforehand. This parameter is used especially when rendering the InteractableElements, so the original rendering is preserved as much as possible.
The class was a workaround just so I could use a Builder widget when rendering each span, that way I could add the buildContext to the Map that handles the list of possible contexts when a link is pressed.
|
I think we can close this in favor of #555? |
Possibly fixes #268
I'm going to open this by saying I'm not too particularly proud of this PR. I've been working on this since Saturday and haven't been able to achieve something I'm satisfied with - this solution just seems awfully hacky to me.
First, I tried using globalkeys to get the current context and then the render object, but the current context would come back as null no matter what I did.
Then I transitioned to a
Builderto get thebuildContextwhich does work, but came with another whole host of issues since you can't returnInlineSpans. I tried usingWidgetSpans everywhere at first, lots of errors. Then I made this class that extends theWidgetSpanand has a field to input the original inline span for use when rendering the links. This has worked, but as I said, I think its pretty hacky.I bet there's also a way to significantly slim down the
CustomTextSpanclass, but I don't know the best way to go about that.@erickok if you could check and make sure this doesn't break all the fixes you made for links that would be great - I did checking on my part and it seems fine.
As for the PR itself, it supports scrolling to anchor links when one is pressed. I also added a method to the
Htmlwidget where the user can scroll the widget to the top from anywhere, in the example app I used a floating action button to demonstrate this.