Skip to content
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] De-singletonize MouseCursor for multi-view #45295

Merged
merged 9 commits into from Sep 18, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Aug 30, 2023

MouseCursor is a singleton that works by accessing flutterViewEmbedder directly. After this PR, MouseCursor becomes a non-singleton that takes a FlutterView and controls the mouse cursor for said view.

Part of flutter/flutter#134443

@mdebbar mdebbar requested a review from ditman August 30, 2023 19:20
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Aug 30, 2023
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Minor nits to simplify this a little bit.

lib/web_ui/lib/src/engine/window.dart Outdated Show resolved Hide resolved
///
/// In addition to everything defined in [ui.FlutterView], this class adds
/// a few web-specific properties.
abstract class EngineFlutterView extends ui.FlutterView {
Copy link
Member

Choose a reason for hiding this comment

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

I like this interface here, but I don't think it needs to extends ui.FlutterView. In the class hierarchy, IIRC, EngineFlutterWindow already extends ui.FlutterView (through ui.SingletonFlutterWindow)

Also, I think this should be abstract interface class EngineFlutterView, if we can use that feature of Dart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought here was that EngineFlutterView will become the base for all window and non-window views. Other non-window views will directly extend EngineFlutterView since they don't need ui.SingletonFlutterWindow.

So the hierarchy becomes:

                         ui.FlutterView
               (dart:ui interface across platforms)
                                |
                                |
                       EngineFlutterView (new)
           (web-specific additions to the interface)
                /                               \
               /                                 \
     NonWindowFlutterView (future)          EngineFlutterWindow
(non-sintleton, non-window views)        (legacy singleton window)

Also, I think this should be abstract interface class EngineFlutterView, if we can use that feature of Dart.

We can! We already use it in platform_location.dart :)

Copy link
Member

Choose a reason for hiding this comment

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

Ahhh, I gotcha. The advantage of the current version:

ui.View -> ui.Window -> WebWindow implements WebOnlyAccessors

Is that we're only extending the base classes defined by the core of the engine. In my mind that means that if there's an upcoming change in the base structure of classes in package:ui, it should be easier for ui to do changes to the base class, and for us to adapt to those changes?

(Of course, it depends on the changes, so our class extensions might not matter too much!)

@mdebbar mdebbar requested a review from ditman September 13, 2023 21:07
'cursor',
_mapKindToCssValue(kind),
);
element.style.cursor = _mapKindToCssValue(kind);
Copy link
Member

Choose a reason for hiding this comment

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

The only problem I can imagine with this is that if the element gets detached/deleted from the DOM, and then a system cursor event message comes, this might attempt to operate on a null instance. Since this is speculation and not a bug that we've seen so far, I say we don't worry about it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a problem at all! The element is non-nullable and cannot become null. Yes, it can be detached/removed from the DOM tree, but it is still a valid element. In fact, in cursor_test.dart, we are using detached elements to test MouseCursor.

@@ -29,8 +31,17 @@ const bool debugPrintPlatformMessages = false;
/// The view ID for the implicit flutter view provided by the platform.
const int kImplicitViewId = 0;

/// Represents all views in the Flutter Web Engine.
Copy link
Member

Choose a reason for hiding this comment

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

I think that the class that represents all views in the Flutter Web Engine is EngineFlutterWindow (that's the type that was used to define the implicitView, for example), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EngineFlutterWindow is a misnomer. It should be called EngineSingletonFlutterWindow (it actually extends ui.SingletonFlutterWindow). Views in the multi-view world shouldn't inherit from this singleton class. They should inherit directly from ui.FlutterView (and implement EngineFlutterView to satisfy all the web-specific needs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a follow up PR to rename this class to make it clear that it's a singleton: #45981

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 18, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 18, 2023

auto label is removed for flutter/engine/45295, due to - The status or check suite Linux linux_android_debug_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 18, 2023
@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 18, 2023
@auto-submit auto-submit bot merged commit 549525b into flutter:main Sep 18, 2023
26 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 18, 2023
…134956)

flutter/engine@be7a039...e1c784e

2023-09-18 skia-flutter-autoroll@skia.org Roll Skia from f8065ca00d0c to 0c990ab9e097 (7 revisions) (flutter/engine#45979)
2023-09-18 mdebbar@google.com [web] ScreenOrientation singleton (flutter/engine#45304)
2023-09-18 mdebbar@google.com [web] De-singletonize MouseCursor for multi-view (flutter/engine#45295)

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 bdero@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…lutter#134956)

flutter/engine@be7a039...e1c784e

2023-09-18 skia-flutter-autoroll@skia.org Roll Skia from f8065ca00d0c to 0c990ab9e097 (7 revisions) (flutter/engine#45979)
2023-09-18 mdebbar@google.com [web] ScreenOrientation singleton (flutter/engine#45304)
2023-09-18 mdebbar@google.com [web] De-singletonize MouseCursor for multi-view (flutter/engine#45295)

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 bdero@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Update tap_region.dart

Update editable_text.dart
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
`MouseCursor` is a singleton that works by accessing `flutterViewEmbedder` directly. After this PR, `MouseCursor` becomes a non-singleton that takes a `FlutterView` and controls the mouse cursor for said view.

Part of flutter/flutter#134443
@mdebbar mdebbar deleted the mouse_cursor_multi_view branch December 11, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
2 participants