Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@joshualitt
Copy link
Contributor

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

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label May 14, 2022
@skia-gold
Copy link

Gold has detected about 6 new digest(s) on patchset 9.
View them at https://flutter-engine-gold.skia.org/cl/github/33347

@joshualitt joshualitt marked this pull request as ready for review May 18, 2022 21:52
@joshualitt joshualitt requested a review from yjbanov May 18, 2022 21:52
@flutter-dashboard
Copy link

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.

Changes reported for pull request #33347 at sha 455d03c

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

class DomHTMLButtonElement extends DomHTMLElement {}

DomHTMLButtonElement createDomHTMLButtonElement() =>
domDocument.createElement('button') as DomHTMLButtonElement;
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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?

@joshualitt
Copy link
Contributor Author

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-web Code specifically for the web engine will affect goldens

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants