[web] Only create one <style> for SelectableRegion#161682
[web] Only create one <style> for SelectableRegion#161682auto-submit[bot] merged 11 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
the
framework
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
This LGTM, I left some comments but I don't think they're blocking.
| // Create css style for _kClassName. | ||
| final web.HTMLStyleElement styleElement = | ||
| web.document.createElement('style') as web.HTMLStyleElement; | ||
| web.document.head!.append(styleElement as JSAny); |
There was a problem hiding this comment.
Would it make sense to give a uniqueID to this styleElement, so it can be re-accessed without caching it in the element (it's supposedly a stateless widget?)
I'm not sure what the "guarantees" are that the stateless widget won't be re-created at any time (causing the code to attempt to re-register the view?)
There was a problem hiding this comment.
The registration methods and variables are all static, so there's only a single registration call across all widgets.
| sheet.insertRule(_kClassRule, 0); | ||
| sheet.insertRule(_kClassSelectionRule, 1); | ||
|
|
||
| _registerViewFactory(_viewType, (int viewId, {Object? params}) { |
There was a problem hiding this comment.
Could this be reimplemented with the fromTag constructor, instead of having to register the view? That way we wouldn't need to keep a flag for the factory being registered or not.
There was a problem hiding this comment.
Yes it could, and I would like that of course :)
But I'll keep this PR focused on fixing the issue at hand. Refactors can come in a follow up PR.
| if (view == null) { | ||
| throw PlatformException( | ||
| code: 'error', | ||
| message: 'Trying to dispose a platform view with unknown id: $id', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Maybe replace these comparisons with null by asserts? The code of this helper would be simplified quite a bit!
assert(view != null, 'Trying to dispose a platform view with unknowin id: $id');?
(Or are we expecting a PlatformException elsewhere in these tests?)
There was a problem hiding this comment.
I don't think we are "expecting" a PlatformException in tests, but the real platform channel throws a PlatformException when an error occurs. So this "fake" registry should throw the same type of exception as the real registry.
flutter-dashboard
bot
removed
the
autosubmit
2 participants
We used to create and insert a new
<style>element for eachSelectableRegionwidget. That's unnecessary. All we need is one<style>element that contains the style sheets that we want to apply.Most of this PR is re-working the tests to be able to check that the issue is actually fixed.
Fixes #161519