-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] Migrate Flutter Web DOM usage to JS static interop - 19. #33347
Conversation
|
Gold has detected about 6 new digest(s) on patchset 9. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
yjbanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| class DomHTMLButtonElement extends DomHTMLElement {} | ||
|
|
||
| DomHTMLButtonElement createDomHTMLButtonElement() => | ||
| domDocument.createElement('button') as DomHTMLButtonElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the static introp allow this to be a static method on DomHTMLButtonElement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, originally I had done just that, but static methods on staticInterop classes were broken in DDC at the time. This might be fixed now, though I have to check, but we might have issues when we move to views.
If you still want to take a chance on static methods, then is it okay if we wait until the end the last migration CL to give it a shot? I can leave it as a final TODO.
| final html.HtmlElement rectangle = | ||
| html.document.createElement(tagName) as html.HtmlElement; | ||
| final DomHTMLElement rectangle = domDocument.createElement(tagName) as | ||
| DomHTMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the as DomHTMLElement necessary here? AFAICT you can create either HTML or SVG elements, and to create SVG you need to use createElementNS. So maybe domDocument.createElement should be typed as DomHTMLElement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the MDN says: A new HTMLElement is returned if the document is an HTMLDocument, which is the most common case. Otherwise a new Element is returned.
In all likelihood, it should be safe to type createElement with DomHTMLElement, but dart:html typed it with Element and I followed the pattern of dart:html out of fear of breaking something in a strange case.
If you don't think there is any possibility where this could suddenly return an Element on any browser we care about then we can change the type. Thoughts?
|
@yjbanov I'm going to land this to keep moving forward. RE: the two unresolved discussions, we can continue the discussion as we go and if necessary we can address them after the chain has landed. |

This is CL 19 in a series of CLs to migrate Flutter Web DOM usage to the new JS static interop API.