Support inner links

[erickok]

Support for links to anchors inside the html document itself.

Fixes #268 and is an alternative implementation to #535.

This should work with links to ids applied wherever in your document tree. If you have multiple tags with the same id, errors will be thrown. Whilst this is a programmer's error, we might want to guard against that (in AnchorKey's factory constructor).

If the Html is not used in a scroll container, the links are simply ignored (as in, nothing will happen when tapping). If the achor style link (#some-id) doesn't exist in the document, the link is ignored. Scrolling is 'instant' i.e. no smooth scrolling.

An implementer could even link to known parts in the document by manually looking up the anchor key and scrolling to it: Scrollable.ensureVisible(AnchorKey.forId(key, "some-id").currentContext)

[tneotia]

Yeah I'm going to go ahead and say this is what we should merge, don't even consider mine as an alternative lol - I'll close #535 once this is reviewed. Really intrigued as to why this worked but when I tried with GlobalKeys it was failing...

[erickok]

@tneotia I am not sure what you tried exactly of course, but the tricky part here is that you want to retrieve a certain context via a globalkey, but for the plain GlobalKey in Flutter you'd need a reference to it. GlobalObjectKey seemed promising but that'd still require the identical object to be used as key value, and in Dart two strings are not identical (they are equals, but not the same object reference). So I created our own AnchorKey which is a GlobalKey variant that just checks on the actual id value. And parent key becasue you could have two Html widgets which contain the same ids and in that case it shoudl work as separate keys.

[tneotia]

I was accessing the GlobalKeys via a Map, where the key is the id of the widget and the value is its GlobalKey. If I understand correctly you have a similar idea, instead you just created an extension on GlobalKey to make it a little more seamless?

If that's the case then that raises more questions than answers. At least it works now though!

[erickok]

@tneotia That approach could have worked (and I tried something very similar until I found our how GlobalKey works internally, which is pretty much exactly this - a map between the keys and the build context (element, to be precise).

If you have time, could you try to test and break my solution, maybe test some edge cases?

[tneotia]

Also what do you think about exposing a Html.scrollToTop() static method? I implemented this in my PR, I thought it might be useful for people who want to have one of those scroll to top buttons if they are displaying particularly long content (maybe in the form of a FAB).

[erickok]

Right, I did see that, but it sounds like something the implementer can do herself? We don't even have control over the scroll container, as it stands. Moreover, what scroll to top and not to bottom?

[tneotia]

It just depends. In my use case of the package, I am getting HTML from an API that I don't have access to on the other side to ensure there is a scroll to top button/implementation. If I really wanted, I could parse the HTML and add those elements in, but the convenience of a method would be way nicer.

I suggested scroll to top just since its more common than scroll to bottom. Also I don't think scroll to bottom would be feasible unless we put an empty container with an id and a AnchorKey at the bottom?

I didn't even notice you didn't include a Scrollable widget... I didn't think it would work without that? In that case you might have to do something similar to the scroll to bottom implementation for a scroll to top method?

Whatever you think is best, I was just trying to provide different perspectives.

[erickok]

I like the different perspectives. It's healthy and you've shown more than once that it's necessary. For this case, indeed no scrollable is even needed (to support the feature). If (and only if) the html is embedded in a scrollable container, the scrolling would work, and I think this is reasonable. Scrolling to the top, even though indeed being the most likely action, is something easily implemented by the implementer via her own scolling container.

[tneotia]

I see what you're saying now. The user would do

Scrollable(
viewportBuilder: (BuildContext context, ViewportOffset position) {  
   return Html()
},
)

in their own app. That's fine by me!

[erickok]

@ryan-berger how about this one? Perhaps decide on this before we make a new release again?

[tneotia]

This one still needs some fixes before merging as per my review I think (don't know if you saw those, maybe they are resolved but just not marked, I haven't tested it since then).

[ryan-berger]

This looks good to me. The only problem that I can foresee is that we are creating a UniqueKey if the key does not exist. This may become a problem since the HTML widget gets rebuilt often due to it being a stateless widget, and there are performance implications.

Is there any reason why we need it? If the AnchorKey doesn't exist, we should be safe due to null-safety right?

[erickok]

@ryan-berger what do you mean by creating a unique key if the key does not exist? Could you elaborate?

Do you mean for the html widget itself? Yes indeed. Hmm ill have to see if we can do without that, though we'd have to assume in that case that there is no clashing ids in multiple Html widgets.

[ryan-berger]

@erickok In the constructor of the Html widget, we have key ?? UniqueKey() which I'm understanding as "if no key is passed to the widget, instantiate a UniqueKey"

[tneotia]

Maybe creating a final UniqueKey _uniqueKey = UniqueKey() instead of instantating a new instance every time can help? Don't know for sure if that would throw some error or not.

[ryan-berger]

@tneotia The "correct" way of handling key instantiation would be to use a Stateful widget, which I'm not super keen on doing. I'm not quite sure why we need this anyway though..... I think we don't need the ??, we can just pass down the key (I think)

[tneotia]

I'm not quite sure why we need this anyway though.....

The UniqueKey helps differentiate between different Html that might be on the same page. Lets say one Html has HTML with ids id1, id2, and id3 (with links to all) and the second Html has HTML with just id1.

The issue is that the onTap is going to search for an AnchorKey with the specific id. In the case of id1, it now has two different results, so how does it resolve that issue? In the current solution @erickok uses the Key to help differentiate.

As far as I know this issue is unavoidable without some sort of identifying information for each Html widget, and the easiest way would be a Key, or if that's really an issue, possibly a random string generator - which, indeed, would limit the number of widgets to a finite amount but it is so astronomically high that it really doesn't matter.

[erickok]

Indeed the reason I wanted to ensure a key is because you can otherwise have potential conflicting duplicate keys and Flutter will not like that (beyond being a potential bug).

What would the performance impact be here?

[ryan-berger]

@erickok @tneotia My point is not that we don't need any keys, it is just that we don't need to be instantiating a new UniqueKey on every rebuild.

If people are not using the feature, do they need the UniqueKey? Shouldn't the link tap just fire off with no key, and thus not scroll to the correct spot?

From my understanding there may be a performance cost associated with constructing a key on every rebuild, but as I can't find where I found that yesterday, I also want to point out something else which I found which is that rebuilding a UniqueKey can cause chaos as seen here:

https://medium.com/flutter/keys-what-are-they-good-for-13cb51742e7d

Basically my contention is summarized like this:

• We should leave passing in the key to the user
• Leaving the key passing to the user won't break anything, and allows them to opt-in to the feature
• There are (possible) performance implications for rebuilding a key so often (which key ?? UniqueKey() will run every build since the Html widget's constructor will fire off each time)

[tneotia]

I completely agree, which is why I had suggested to make some sort of private final field for the UniqueKey to prevent creation on rebuild: final UniqueKey _uniqueKey = UniqueKey(), and then use key ?? _uniqueKey. As I said previously I didn't test that in code so I have no idea if it works.

Allowing the user to opt-in would be fine too, and the functionality can be disabled if a key is not passed to the widget, like you said.

[ryan-berger]

@tneotia The problem here is that, from my understanding:

class Html extends StatelessWidget {
    final _uniqueKey = UniqueKey();
    // build fn goes here
}

will run into the problem that:

Html()

Forces the evaluation of the UniqueKey(), so if Flutter decides to reconstruct the widget or call the constructor again, it will end up getting run as many times as Flutter does so.

The only way around this is a StatefulWidget which again, has a lot of drawbacks just to implement this single feature

[tneotia]

Hm I was under the impression that final variables declared in the class would not be re-created/re-evaluated on rebuild, since (in my mind) only the build function is called. If that is not the case then I guess the best option is to require the user to pass a key (if they'd like the anchor link functionality).

[erickok]

You guys are confusing me... 😕 Keys are typically used for the exact opposite of that you are saying, namely they can be used (beyond identifying things) to prevent having to rebuild a widget (element). A key identifies a widget in the tree. Without a key the type would be used. With a key the key is used to identify it. UniqueKey is a key that makes a widget unique - not over multiple rebuilds of the Element but within the widget tree. There is no rebuilding of the widget just because you use a UniqueKey.

[ryan-berger]

@erickok It is true that a UniqueKey makes a widget unique in the widget tree, but my point is that when a rebuild happens, the Widget becomes a "new widget" and therefore creates a new UniqueKey.

[erickok]

True, if a parent causes a rebuild, this Html would be a new widget. Do you see any solution to this problem?

[DFelten]

I think the safest option is to remove creating a new UniqueKey and activate this anchor links only when a developer ads a key by himself into the html widget.

Of course we need some documentation in the class that it's only working when a key is added.

An other question to this pull request, is it somehow possible to use anchor links also with multiple html widgets? We're creating a list of html widgets for a better performance and currently we have to implement the anchor links by ourself relatively complicated.

[tneotia]

An other question to this pull request, is it somehow possible to use anchor links also with multiple html widgets? We're creating a list of html widgets for a better performance and currently we have to implement the anchor links by ourself relatively complicated.

At this moment in time I think this PR tries to avoid that situation and will only anchor link to the html in the immediate html widget. Maybe an option for this can be added? I don't know - @erickok maybe has an idea here.

We're creating a list of html widgets for a better performance

We really need to try and up the performance. I'm going to see what I can figure out today and maybe report back with a discussion if I find anything.

[erickok]

To use anchor links between html widgets is not possible with the current setup. Here is why:

• Linking works via Flutter's Scrollable.ensureVisible(someKey)
• The key to link to needs to be a GlobalKey - which is what our newly introduced AnchorKey is (+ a the id as reference to look up).
• As GlobalKeys need to be unique, and we can potentially have multiple Html widgets with elements that use the same id, I needed to ensure uniqueness through using the Html widget's key.
• So AnchorKeys only link within their own Html

To get back to the problem that UniqueKey causes creating a new Element upon rebuilds, I tested and we can also use GlobalKey instead of the UniqueKey. I think that deals with your concerns?

[erickok]

Any feedback @ryan-berger ? I think I addressed your concerns?

[tneotia]

@erickok I added a PR to this PR in the linked PR above - just letting you know.

[erickok]

@tneotia 1 comment on that PR

[tneotia]

Responded back :)

[giorgio79]

This no longer works. Scrollable.ensureVisible(someKey) expects a context, not a key...