-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web] Move all DOM creation to DomManager #48123
Conversation
|
||
StyleManager.styleSemanticsHost( | ||
semanticsHost, | ||
devicePixelRatio, |
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.
Looks like devicePixelRatio
is only used here, and also it seems like a bug? How do we update the semantics host when devicePixelRatio
changes?
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.
Looks like
devicePixelRatio
is only used here
Not sure what you mean here. Yes, it's not used anywhere else in this method.
and also it seems like a bug? How do we update the semantics host when
devicePixelRatio
changes?
The code that listens for resize events (and updates the semantics host) is still in FlutterViewEmbedder
for now:
engine/lib/web_ui/lib/src/engine/embedder.dart
Lines 220 to 224 in aae07e9
void _metricsDidChange(ui.Size? newSize) { | |
StyleManager.scaleSemanticsHost( | |
_semanticsHostElement, | |
window.devicePixelRatio, | |
); |
I do plan to move it out of there though. Stay tuned.
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.
I'm just having a feeling that this is the wrong place for styling and scaling the semantics host. In fact, I'm thinking maybe the semantics host is the wrong element to style and scale. Should it be the responsibility of EngineSemanticsOwner
? Then we won't need DomManager
to be concerned about devicePixelRatio
.
Having said that, I'm totally fine with not making this change in this PR.
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.
Yeah, I think it makes sense for the semantic owner to "own" the styling of the semantics tree.
I'll leave it as is for now, but feel free to move it to the semantics owner as part of your overhaul there.
|
||
/// This is where accessibility announcements are inserted. | ||
DomElement get announcementsHost => _embedder.announcementsHostDEPRECATED; | ||
DomElement announcementsHost; |
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.
Can we make all DOM elements final?
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.
YES!
} | ||
|
||
DomShadowRoot _attachShadowRoot(DomElement element) { | ||
if (getJsProperty<Object?>(element, 'attachShadow') == null) { |
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.
I think we can remove this check now. I believe @ditman removed the non-shadow DOM mode a long time ago.
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.
It was me who removed it, and I kept this check so that we show a somewhat meaningful error to the user instead of an obscure null check error. I'm happy to remove it if we think there's no value in having this error message.
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.
There's probably some value, but also, this leaks into production code and increases code size, so there's also value in removing it.
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.
Ok, let me put it behind an assert.
'mode': 'open', | ||
// This needs to stay false to prevent issues like this: | ||
// - https://github.com/flutter/flutter/issues/85759 | ||
'delegatesFocus': false, |
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.
I think we don't need this either, since @htoor3 moved all focusables (text input, semantics) outside the shadow root.
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.
I would prefer to not make functional changes in this PR :)
I'll file an issue though, so we make this change in the future.
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.
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.
…138637) flutter/engine@90c3ada...141a01c 2023-11-17 dnfield@google.com Reland "[Impeller] Fail if software backend is chosen and Impeller is enabled on iOS." (flutter/engine#46275) 2023-11-17 matanlurey@users.noreply.github.com Make `impeller/geometry/...` compatible with `.clang-tidy`. (flutter/engine#48154) 2023-11-17 matanlurey@users.noreply.github.com Make `impeller/{archivist|compiler|core|entity}/...` compatible with â�¦ (flutter/engine#48153) 2023-11-17 109111084+yaakovschectman@users.noreply.github.com Assign mojom `kSwitch` role to switches (flutter/engine#48146) 2023-11-17 mdebbar@google.com [web] Move scene DOM management to DomManager (flutter/engine#47460) 2023-11-17 30870216+gaaclarke@users.noreply.github.com [Impeller] Unify around "transform" (flutter/engine#48184) 2023-11-17 skia-flutter-autoroll@skia.org Roll Dart SDK from a9c212f2f54b to 03cddb5d740d (1 revision) (flutter/engine#48182) 2023-11-17 matanlurey@users.noreply.github.com Actually make `status_or.h` compatible with `.clang-tidy`. (flutter/engine#48151) 2023-11-17 jonahwilliams@google.com [Impeller] Cleanups to geometry interfaces. (flutter/engine#48180) 2023-11-17 mdebbar@google.com [web] Move all DOM creation to DomManager (flutter/engine#48123) 2023-11-17 15619084+vashworth@users.noreply.github.com Reenable UnobstructedPlatformViewTests testMultiplePlatformViewsWithOverlays (flutter/engine#48139) 2023-11-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 46e8b18047eb to a9c212f2f54b (1 revision) (flutter/engine#48176) 2023-11-17 skia-flutter-autoroll@skia.org Roll Skia from bcd22e8f95bc to 8e9e168418a0 (1 revision) (flutter/engine#48173) 2023-11-17 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from M0zM3CJLIrd5lb0u0... to Bcq9TZdt-vtTSL5YH... (flutter/engine#48172) 2023-11-17 skia-flutter-autoroll@skia.org Roll Skia from c8ee25282849 to bcd22e8f95bc (1 revision) (flutter/engine#48170) 2023-11-17 bdero@google.com [Flutter GPU] Add Textures. (flutter/engine#48118) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from M0zM3CJLIrd5 to Bcq9TZdt-vtT If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
FlutterViewEmbedder
doesn't create the DOM tree anymore (instead,DomManager
does).DomManager
only creates the DOM tree (with styles) but doesn't insert it into the document.EngineFlutterView
takes the root element fromDomManager
and inserts it into the document.cc @yjbanov since I'm making a few changes that probably conflict with your semantics changes.
Part of flutter/flutter#134443
Part of flutter/flutter#137447