From 8aee4dde853b364c4151e5c976edbf8858216e75 Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Fri, 20 Oct 2023 13:12:17 -0400 Subject: [PATCH 1/9] Roll Skia from de628929015d to b960e9140f56 (2 revisions) (#47164) https://skia.googlesource.com/skia.git/+log/de628929015d..b960e9140f56 2023-10-20 armansito@google.com Add Graphite Vello Test jobs to CQ 2023-10-20 herb@google.com Add point_less_than_segment_in_x If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/skia-flutter-autoroll Please CC brianosman@google.com,jimgraham@google.com,rmistry@google.com,scroggo@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry 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 --- DEPS | 2 +- ci/licenses_golden/licenses_skia | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index bf302fc3612eb..7848724186d52 100644 --- a/DEPS +++ b/DEPS @@ -18,7 +18,7 @@ vars = { 'llvm_git': 'https://llvm.googlesource.com', # OCMock is for testing only so there is no google clone 'ocmock_git': 'https://github.com/erikdoe/ocmock.git', - 'skia_revision': 'de628929015d144edc1ec4a407601fc78da24db4', + 'skia_revision': 'b960e9140f56675c0a2f6a8664967a56bd3e5158', # WARNING: DO NOT EDIT canvaskit_cipd_instance MANUALLY # See `lib/web_ui/README.md` for how to roll CanvasKit to a new version. diff --git a/ci/licenses_golden/licenses_skia b/ci/licenses_golden/licenses_skia index 626adef770a01..bee8d62c68cc4 100644 --- a/ci/licenses_golden/licenses_skia +++ b/ci/licenses_golden/licenses_skia @@ -1,4 +1,4 @@ -Signature: cc293eec6887015255a7b33a10f2577b +Signature: 1c400dc7d1eeb2a8c37c54eca028db89 ==================================================================================================== LIBRARY: etc1 From 29168dd7502b3e8c9c9fa8bfbc5521b1821cecc6 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 20 Oct 2023 10:20:43 -0700 Subject: [PATCH 2/9] [Impeller] GPU Tracer for GLES. (#47080) Trace GPU execution time on GLES using GL_EXT_disjoint_timer_query. This requires a per-app opt in from the Android Manifest with the key `"io.flutter.embedding.android.EnableOpenGLGPUTracing` set to true. --- ci/licenses_golden/licenses_flutter | 4 + common/settings.h | 4 + .../backend/gles/playground_impl_gles.cc | 4 +- impeller/renderer/backend/gles/BUILD.gn | 3 + .../renderer/backend/gles/context_gles.cc | 17 ++-- impeller/renderer/backend/gles/context_gles.h | 13 ++- .../renderer/backend/gles/gpu_tracer_gles.cc | 88 +++++++++++++++++++ .../renderer/backend/gles/gpu_tracer_gles.h | 51 +++++++++++ .../renderer/backend/gles/proc_table_gles.h | 8 +- .../renderer/backend/gles/render_pass_gles.cc | 20 ++++- .../renderer/backend/gles/surface_gles.cc | 4 + .../gles/test/gpu_tracer_gles_unittests.cc | 50 +++++++++++ .../renderer/backend/gles/test/mock_gles.cc | 59 +++++++++++++ .../renderer/backend/metal/gpu_tracer_mtl.h | 11 +-- shell/common/switches.cc | 2 + shell/common/switches.h | 4 + .../android/android_context_gl_impeller.cc | 28 +++--- .../android/android_context_gl_impeller.h | 4 +- .../android_context_gl_impeller_unittests.cc | 6 +- .../android/android_context_vulkan_impeller.h | 2 +- .../engine/loader/FlutterLoader.java | 5 ++ .../platform/android/platform_view_android.cc | 13 ++- .../embedder/embedder_surface_gl_impeller.cc | 13 +-- 23 files changed, 364 insertions(+), 49 deletions(-) create mode 100644 impeller/renderer/backend/gles/gpu_tracer_gles.cc create mode 100644 impeller/renderer/backend/gles/gpu_tracer_gles.h create mode 100644 impeller/renderer/backend/gles/test/gpu_tracer_gles_unittests.cc diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 29cb1c5e034ab..e6e3308249404 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -2086,6 +2086,8 @@ ORIGIN: ../../../flutter/impeller/renderer/backend/gles/device_buffer_gles.h + . ORIGIN: ../../../flutter/impeller/renderer/backend/gles/formats_gles.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/backend/gles/formats_gles.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/backend/gles/gles.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/renderer/backend/gles/gpu_tracer_gles.cc + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/renderer/backend/gles/gpu_tracer_gles.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/backend/gles/handle_gles.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/backend/gles/handle_gles.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/renderer/backend/gles/pipeline_gles.cc + ../../../flutter/LICENSE @@ -4855,6 +4857,8 @@ FILE: ../../../flutter/impeller/renderer/backend/gles/device_buffer_gles.h FILE: ../../../flutter/impeller/renderer/backend/gles/formats_gles.cc FILE: ../../../flutter/impeller/renderer/backend/gles/formats_gles.h FILE: ../../../flutter/impeller/renderer/backend/gles/gles.h +FILE: ../../../flutter/impeller/renderer/backend/gles/gpu_tracer_gles.cc +FILE: ../../../flutter/impeller/renderer/backend/gles/gpu_tracer_gles.h FILE: ../../../flutter/impeller/renderer/backend/gles/handle_gles.cc FILE: ../../../flutter/impeller/renderer/backend/gles/handle_gles.h FILE: ../../../flutter/impeller/renderer/backend/gles/pipeline_gles.cc diff --git a/common/settings.h b/common/settings.h index af002c93006c4..00dc5aeaf8d04 100644 --- a/common/settings.h +++ b/common/settings.h @@ -228,6 +228,10 @@ struct Settings { // must be available to the application. bool enable_vulkan_validation = false; + // Enable GPU tracing in GLES backends. + // Some devices claim to support the required APIs but crash on their usage. + bool enable_opengl_gpu_tracing = false; + // Data set by platform-specific embedders for use in font initialization. uint32_t font_initialization_data = 0; diff --git a/impeller/playground/backend/gles/playground_impl_gles.cc b/impeller/playground/backend/gles/playground_impl_gles.cc index 6e5c3f11ab198..fbf64c44ba35c 100644 --- a/impeller/playground/backend/gles/playground_impl_gles.cc +++ b/impeller/playground/backend/gles/playground_impl_gles.cc @@ -112,8 +112,8 @@ std::shared_ptr PlaygroundImplGLES::GetContext() const { return nullptr; } - auto context = - ContextGLES::Create(std::move(gl), ShaderLibraryMappingsForPlayground()); + auto context = ContextGLES::Create( + std::move(gl), ShaderLibraryMappingsForPlayground(), true); if (!context) { FML_LOG(ERROR) << "Could not create context."; return nullptr; diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index 1d783c0f8c841..4188d25478250 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/impeller/renderer/backend/gles/BUILD.gn @@ -16,6 +16,7 @@ impeller_component("gles_unittests") { sources = [ "test/capabilities_unittests.cc", "test/formats_gles_unittests.cc", + "test/gpu_tracer_gles_unittests.cc", "test/mock_gles.cc", "test/mock_gles.h", "test/mock_gles_unittests.cc", @@ -51,6 +52,8 @@ impeller_component("gles") { "formats_gles.cc", "formats_gles.h", "gles.h", + "gpu_tracer_gles.cc", + "gpu_tracer_gles.h", "handle_gles.cc", "handle_gles.h", "pipeline_gles.cc", diff --git a/impeller/renderer/backend/gles/context_gles.cc b/impeller/renderer/backend/gles/context_gles.cc index 6896d6609777e..779255dffa6c4 100644 --- a/impeller/renderer/backend/gles/context_gles.cc +++ b/impeller/renderer/backend/gles/context_gles.cc @@ -3,23 +3,27 @@ // found in the LICENSE file. #include "impeller/renderer/backend/gles/context_gles.h" +#include #include "impeller/base/config.h" #include "impeller/base/validation.h" #include "impeller/renderer/backend/gles/command_buffer_gles.h" +#include "impeller/renderer/backend/gles/gpu_tracer_gles.h" namespace impeller { std::shared_ptr ContextGLES::Create( std::unique_ptr gl, - const std::vector>& shader_libraries) { + const std::vector>& shader_libraries, + bool enable_gpu_tracing) { return std::shared_ptr( - new ContextGLES(std::move(gl), shader_libraries)); + new ContextGLES(std::move(gl), shader_libraries, enable_gpu_tracing)); } -ContextGLES::ContextGLES(std::unique_ptr gl, - const std::vector>& - shader_libraries_mappings) { +ContextGLES::ContextGLES( + std::unique_ptr gl, + const std::vector>& shader_libraries_mappings, + bool enable_gpu_tracing) { reactor_ = std::make_shared(std::move(gl)); if (!reactor_->IsValid()) { VALIDATION_LOG << "Could not create valid reactor."; @@ -61,7 +65,8 @@ ContextGLES::ContextGLES(std::unique_ptr gl, std::shared_ptr(new SamplerLibraryGLES( device_capabilities_->SupportsDecalSamplerAddressMode())); } - + gpu_tracer_ = std::make_shared(GetReactor()->GetProcTable(), + enable_gpu_tracing); is_valid_ = true; } diff --git a/impeller/renderer/backend/gles/context_gles.h b/impeller/renderer/backend/gles/context_gles.h index 56da6c55a8092..0d097c3f18a87 100644 --- a/impeller/renderer/backend/gles/context_gles.h +++ b/impeller/renderer/backend/gles/context_gles.h @@ -4,10 +4,13 @@ #pragma once +#include +#include #include "flutter/fml/macros.h" #include "impeller/base/backend_cast.h" #include "impeller/renderer/backend/gles/allocator_gles.h" #include "impeller/renderer/backend/gles/capabilities_gles.h" +#include "impeller/renderer/backend/gles/gpu_tracer_gles.h" #include "impeller/renderer/backend/gles/pipeline_library_gles.h" #include "impeller/renderer/backend/gles/reactor_gles.h" #include "impeller/renderer/backend/gles/sampler_library_gles.h" @@ -23,7 +26,8 @@ class ContextGLES final : public Context, public: static std::shared_ptr Create( std::unique_ptr gl, - const std::vector>& shader_libraries); + const std::vector>& shader_libraries, + bool enable_gpu_tracing); // |Context| ~ContextGLES() override; @@ -38,12 +42,16 @@ class ContextGLES final : public Context, bool RemoveReactorWorker(ReactorGLES::WorkerID id); + std::shared_ptr GetGPUTracer() const { return gpu_tracer_; } + private: ReactorGLES::Ref reactor_; std::shared_ptr shader_library_; std::shared_ptr pipeline_library_; std::shared_ptr sampler_library_; std::shared_ptr resource_allocator_; + std::shared_ptr gpu_tracer_; + // Note: This is stored separately from the ProcTableGLES CapabilitiesGLES // in order to satisfy the Context::GetCapabilities signature which returns // a reference. @@ -52,7 +60,8 @@ class ContextGLES final : public Context, ContextGLES( std::unique_ptr gl, - const std::vector>& shader_libraries); + const std::vector>& shader_libraries, + bool enable_gpu_tracing); // |Context| std::string DescribeGpuModel() const override; diff --git a/impeller/renderer/backend/gles/gpu_tracer_gles.cc b/impeller/renderer/backend/gles/gpu_tracer_gles.cc new file mode 100644 index 0000000000000..6e1e167877b3d --- /dev/null +++ b/impeller/renderer/backend/gles/gpu_tracer_gles.cc @@ -0,0 +1,88 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "impeller/renderer/backend/gles/gpu_tracer_gles.h" +#include +#include "fml/trace_event.h" + +namespace impeller { + +GPUTracerGLES::GPUTracerGLES(const ProcTableGLES& gl, bool enable_tracing) { +#ifdef IMPELLER_DEBUG + auto desc = gl.GetDescription(); + enabled_ = + enable_tracing && desc->HasExtension("GL_EXT_disjoint_timer_query"); +#endif // IMPELLER_DEBUG +} + +void GPUTracerGLES::MarkFrameStart(const ProcTableGLES& gl) { + if (!enabled_ || active_frame_.has_value() || + std::this_thread::get_id() != raster_thread_) { + return; + } + + // At the beginning of a frame, check the status of all pending + // previous queries. + ProcessQueries(gl); + + uint32_t query = 0; + gl.GenQueriesEXT(1, &query); + if (query == 0) { + return; + } + + active_frame_ = query; + gl.BeginQueryEXT(GL_TIME_ELAPSED_EXT, query); +} + +void GPUTracerGLES::RecordRasterThread() { + raster_thread_ = std::this_thread::get_id(); +} + +void GPUTracerGLES::ProcessQueries(const ProcTableGLES& gl) { + // For reasons unknown to me, querying the state of more than + // one query object per frame causes crashes on a Pixel 6 pro. + // It does not crash on an S10. + while (!pending_traces_.empty()) { + auto query = pending_traces_.front(); + + // First check if the query is complete without blocking + // on the result. Incomplete results are left in the pending + // trace vector and will not be checked again for another + // frame. + GLuint available = GL_FALSE; + gl.GetQueryObjectuivEXT(query, GL_QUERY_RESULT_AVAILABLE_EXT, &available); + + if (available != GL_TRUE) { + // If a query is not available, then all subsequent queries will be + // unavailable. + return; + } + // Return the timer resolution in nanoseconds. + uint64_t duration = 0; + gl.GetQueryObjectui64vEXT(query, GL_QUERY_RESULT_EXT, &duration); + auto gpu_ms = duration / 1000000.0; + + FML_TRACE_COUNTER("flutter", "GPUTracer", + reinterpret_cast(this), // Trace Counter ID + "FrameTimeMS", gpu_ms); + gl.DeleteQueriesEXT(1, &query); + pending_traces_.pop_front(); + } +} + +void GPUTracerGLES::MarkFrameEnd(const ProcTableGLES& gl) { + if (!enabled_ || std::this_thread::get_id() != raster_thread_ || + !active_frame_.has_value()) { + return; + } + + auto query = active_frame_.value(); + gl.EndQueryEXT(GL_TIME_ELAPSED_EXT); + + pending_traces_.push_back(query); + active_frame_ = std::nullopt; +} + +} // namespace impeller diff --git a/impeller/renderer/backend/gles/gpu_tracer_gles.h b/impeller/renderer/backend/gles/gpu_tracer_gles.h new file mode 100644 index 0000000000000..8de6963fc6759 --- /dev/null +++ b/impeller/renderer/backend/gles/gpu_tracer_gles.h @@ -0,0 +1,51 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#pragma once + +#include +#include + +#include "impeller/renderer/backend/gles/proc_table_gles.h" + +namespace impeller { + +/// @brief Trace GPU execution times using GL_EXT_disjoint_timer_query on GLES. +/// +/// Note: there are a substantial number of GPUs where usage of the this API is +/// known to cause crashes. As a result, this functionality is disabled by +/// default and can only be enabled in debug/profile mode via a specific opt-in +/// flag that is exposed in the Android manifest. +/// +/// To enable, add the following metadata to the application's Android manifest: +/// +class GPUTracerGLES { + public: + GPUTracerGLES(const ProcTableGLES& gl, bool enable_tracing); + + ~GPUTracerGLES() = default; + + /// @brief Record the thread id of the raster thread. + void RecordRasterThread(); + + /// @brief Record the start of a frame workload, if one hasn't already been + /// started. + void MarkFrameStart(const ProcTableGLES& gl); + + /// @brief Record the end of a frame workload. + void MarkFrameEnd(const ProcTableGLES& gl); + + private: + void ProcessQueries(const ProcTableGLES& gl); + + std::deque pending_traces_; + std::optional active_frame_ = std::nullopt; + std::thread::id raster_thread_; + + bool enabled_ = false; +}; + +} // namespace impeller diff --git a/impeller/renderer/backend/gles/proc_table_gles.h b/impeller/renderer/backend/gles/proc_table_gles.h index bbf94a23ea985..643ddd413f3f9 100644 --- a/impeller/renderer/backend/gles/proc_table_gles.h +++ b/impeller/renderer/backend/gles/proc_table_gles.h @@ -196,7 +196,13 @@ struct GLProc { PROC(PushDebugGroupKHR); \ PROC(PopDebugGroupKHR); \ PROC(ObjectLabelKHR); \ - PROC(RenderbufferStorageMultisampleEXT); + PROC(RenderbufferStorageMultisampleEXT); \ + PROC(GenQueriesEXT); \ + PROC(DeleteQueriesEXT); \ + PROC(GetQueryObjectui64vEXT); \ + PROC(BeginQueryEXT); \ + PROC(EndQueryEXT); \ + PROC(GetQueryObjectuivEXT); enum class DebugResourceType { kTexture, diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index 02356fdd58748..a856feeeff1a1 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -7,8 +7,10 @@ #include "flutter/fml/trace_event.h" #include "fml/closure.h" #include "impeller/base/validation.h" +#include "impeller/renderer/backend/gles/context_gles.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" #include "impeller/renderer/backend/gles/formats_gles.h" +#include "impeller/renderer/backend/gles/gpu_tracer_gles.h" #include "impeller/renderer/backend/gles/pipeline_gles.h" #include "impeller/renderer/backend/gles/texture_gles.h" @@ -141,7 +143,8 @@ struct RenderPassData { const RenderPassData& pass_data, const std::shared_ptr& transients_allocator, const ReactorGLES& reactor, - const std::vector& commands) { + const std::vector& commands, + const std::shared_ptr& tracer) { TRACE_EVENT0("impeller", "RenderPassGLES::EncodeCommandsInReactor"); if (commands.empty()) { @@ -149,6 +152,9 @@ struct RenderPassData { } const auto& gl = reactor.GetProcTable(); +#ifdef IMPELLER_DEBUG + tracer->MarkFrameStart(gl); +#endif // IMPELLER_DEBUG fml::ScopedCleanupClosure pop_pass_debug_marker( [&gl]() { gl.PopDebugGroup(); }); @@ -492,6 +498,11 @@ struct RenderPassData { attachments.data() // size ); } +#ifdef IMPELLER_DEBUG + if (is_default_fbo) { + tracer->MarkFrameEnd(gl); + } +#endif // IMPELLER_DEBUG return true; } @@ -549,12 +560,13 @@ bool RenderPassGLES::OnEncodeCommands(const Context& context) const { } std::shared_ptr shared_this = shared_from_this(); + auto tracer = ContextGLES::Cast(context).GetGPUTracer(); return reactor_->AddOperation([pass_data, allocator = context.GetResourceAllocator(), - render_pass = std::move(shared_this)]( - const auto& reactor) { + render_pass = std::move(shared_this), + tracer](const auto& reactor) { auto result = EncodeCommandsInReactor(*pass_data, allocator, reactor, - render_pass->commands_); + render_pass->commands_, tracer); FML_CHECK(result) << "Must be able to encode GL commands without error."; }); } diff --git a/impeller/renderer/backend/gles/surface_gles.cc b/impeller/renderer/backend/gles/surface_gles.cc index a179b3798f3d9..9c817e6e382a7 100644 --- a/impeller/renderer/backend/gles/surface_gles.cc +++ b/impeller/renderer/backend/gles/surface_gles.cc @@ -60,6 +60,10 @@ std::unique_ptr SurfaceGLES::WrapFBO( render_target_desc.SetColorAttachment(color0, 0u); render_target_desc.SetStencilAttachment(stencil0); +#ifdef IMPELLER_DEBUG + gl_context.GetGPUTracer()->RecordRasterThread(); +#endif // IMPELLER_DEBUG + return std::unique_ptr( new SurfaceGLES(std::move(swap_callback), render_target_desc)); } diff --git a/impeller/renderer/backend/gles/test/gpu_tracer_gles_unittests.cc b/impeller/renderer/backend/gles/test/gpu_tracer_gles_unittests.cc new file mode 100644 index 0000000000000..d0579a2091b23 --- /dev/null +++ b/impeller/renderer/backend/gles/test/gpu_tracer_gles_unittests.cc @@ -0,0 +1,50 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/testing/testing.h" // IWYU pragma: keep +#include "gtest/gtest.h" +#include "impeller/renderer/backend/gles/gpu_tracer_gles.h" +#include "impeller/renderer/backend/gles/test/mock_gles.h" + +namespace impeller { +namespace testing { + +#ifdef IMPELLER_DEBUG +TEST(GPUTracerGLES, CanFormatFramebufferErrorMessage) { + auto const extensions = std::vector{ + reinterpret_cast("GL_KHR_debug"), // + reinterpret_cast("GL_EXT_disjoint_timer_query"), // + }; + auto mock_gles = MockGLES::Init(extensions); + auto tracer = + std::make_shared(mock_gles->GetProcTable(), true); + tracer->RecordRasterThread(); + tracer->MarkFrameStart(mock_gles->GetProcTable()); + tracer->MarkFrameEnd(mock_gles->GetProcTable()); + + auto calls = mock_gles->GetCapturedCalls(); + + std::vector expected = {"glGenQueriesEXT", "glBeginQueryEXT", + "glEndQueryEXT"}; + for (auto i = 0; i < 3; i++) { + EXPECT_EQ(calls[i], expected[i]); + } + + // Begin second frame, which prompts the tracer to query the result + // from the previous frame. + tracer->MarkFrameStart(mock_gles->GetProcTable()); + + calls = mock_gles->GetCapturedCalls(); + std::vector expected_b = {"glGetQueryObjectuivEXT", + "glGetQueryObjectui64vEXT", + "glDeleteQueriesEXT"}; + for (auto i = 0; i < 3; i++) { + EXPECT_EQ(calls[i], expected_b[i]); + } +} + +#endif // IMPELLER_DEBUG + +} // namespace testing +} // namespace impeller diff --git a/impeller/renderer/backend/gles/test/mock_gles.cc b/impeller/renderer/backend/gles/test/mock_gles.cc index 1abf7e8721d46..30ea1ae3eefc7 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/impeller/renderer/backend/gles/test/mock_gles.cc @@ -112,6 +112,53 @@ void mockPushDebugGroupKHR(GLenum source, static_assert(CheckSameSignature::value); +void mockGenQueriesEXT(GLsizei n, GLuint* ids) { + RecordGLCall("glGenQueriesEXT"); + for (auto i = 0; i < n; i++) { + ids[i] = i + 1; + } +} + +static_assert(CheckSameSignature::value); + +void mockBeginQueryEXT(GLenum target, GLuint id) { + RecordGLCall("glBeginQueryEXT"); +} + +static_assert(CheckSameSignature::value); + +void mockEndQueryEXT(GLuint id) { + RecordGLCall("glEndQueryEXT"); +} + +static_assert(CheckSameSignature::value); + +void mockGetQueryObjectuivEXT(GLuint id, GLenum target, GLuint* result) { + RecordGLCall("glGetQueryObjectuivEXT"); + *result = GL_TRUE; +} + +static_assert(CheckSameSignature::value); + +void mockGetQueryObjectui64vEXT(GLuint id, GLenum target, GLuint64* result) { + RecordGLCall("glGetQueryObjectui64vEXT"); + *result = 1000u; +} + +static_assert(CheckSameSignature::value); + +void mockDeleteQueriesEXT(GLsizei size, const GLuint* queries) { + RecordGLCall("glDeleteQueriesEXT"); +} + +static_assert(CheckSameSignature::value); + std::shared_ptr MockGLES::Init( const std::optional>& extensions) { // If we cannot obtain a lock, MockGLES is already being used elsewhere. @@ -136,6 +183,18 @@ const ProcTableGLES::Resolver kMockResolver = [](const char* name) { return reinterpret_cast(&mockGetIntegerv); } else if (strcmp(name, "glGetError") == 0) { return reinterpret_cast(&mockGetError); + } else if (strcmp(name, "glGenQueriesEXT") == 0) { + return reinterpret_cast(&mockGenQueriesEXT); + } else if (strcmp(name, "glBeginQueryEXT") == 0) { + return reinterpret_cast(&mockBeginQueryEXT); + } else if (strcmp(name, "glEndQueryEXT") == 0) { + return reinterpret_cast(&mockEndQueryEXT); + } else if (strcmp(name, "glDeleteQueriesEXT") == 0) { + return reinterpret_cast(&mockDeleteQueriesEXT); + } else if (strcmp(name, "glGetQueryObjectui64vEXT") == 0) { + return reinterpret_cast(mockGetQueryObjectui64vEXT); + } else if (strcmp(name, "glGetQueryObjectuivEXT") == 0) { + return reinterpret_cast(mockGetQueryObjectuivEXT); } else { return reinterpret_cast(&doNothing); } diff --git a/impeller/renderer/backend/metal/gpu_tracer_mtl.h b/impeller/renderer/backend/metal/gpu_tracer_mtl.h index bfa227ed10e81..ac2c3916d0207 100644 --- a/impeller/renderer/backend/metal/gpu_tracer_mtl.h +++ b/impeller/renderer/backend/metal/gpu_tracer_mtl.h @@ -17,9 +17,8 @@ namespace impeller { class ContextMTL; /// @brief Approximate the GPU frame time by computing a difference between the -/// smallest -/// GPUStartTime and largest GPUEndTime for all cmd buffers submitted in -/// a frame workload. +/// smallest GPUStartTime and largest GPUEndTime for all command buffers +/// submitted in a frame workload. class GPUTracerMTL : public std::enable_shared_from_this { public: GPUTracerMTL() = default; @@ -27,13 +26,11 @@ class GPUTracerMTL : public std::enable_shared_from_this { ~GPUTracerMTL() = default; /// @brief Record that the current frame has ended. Any additional cmd buffers - /// will be - /// attributed to the "next" frame. + /// will be attributed to the "next" frame. void MarkFrameEnd(); /// @brief Record the current cmd buffer GPU execution timestamps into an - /// aggregate - /// frame workload metric. + /// aggregate frame workload metric. void RecordCmdBuffer(id buffer); private: diff --git a/shell/common/switches.cc b/shell/common/switches.cc index e7c5666b147e2..8ed53d09ff13b 100644 --- a/shell/common/switches.cc +++ b/shell/common/switches.cc @@ -477,6 +477,8 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { settings.enable_vulkan_validation = command_line.HasOption(FlagForSwitch(Switch::EnableVulkanValidation)); + settings.enable_opengl_gpu_tracing = + command_line.HasOption(FlagForSwitch(Switch::EnableOpenGLGPUTracing)); settings.enable_embedder_api = command_line.HasOption(FlagForSwitch(Switch::EnableEmbedderAPI)); diff --git a/shell/common/switches.h b/shell/common/switches.h index c6048705f3121..ddc4b3c8827aa 100644 --- a/shell/common/switches.h +++ b/shell/common/switches.h @@ -275,6 +275,10 @@ DEF_SWITCH(EnableVulkanValidation, "Enable loading Vulkan validation layers. The layers must be " "available to the application and loadable. On non-Vulkan backends, " "this flag does nothing.") +DEF_SWITCH(EnableOpenGLGPUTracing, + "enable-opengl-gpu-tracing", + "Enable tracing of GPU execution time when using the Impeller " + "OpenGLES backend.") DEF_SWITCH(LeakVM, "leak-vm", "When the last shell shuts down, the shared VM is leaked by default " diff --git a/shell/platform/android/android_context_gl_impeller.cc b/shell/platform/android/android_context_gl_impeller.cc index 83c1bb4fedd91..8c9b80645d7ab 100644 --- a/shell/platform/android/android_context_gl_impeller.cc +++ b/shell/platform/android/android_context_gl_impeller.cc @@ -49,7 +49,8 @@ class AndroidContextGLImpeller::ReactorWorker final }; static std::shared_ptr CreateImpellerContext( - const std::shared_ptr& worker) { + const std::shared_ptr& worker, + bool enable_gpu_tracing) { auto proc_table = std::make_unique( impeller::egl::CreateProcAddressResolver()); @@ -59,19 +60,20 @@ static std::shared_ptr CreateImpellerContext( } std::vector> shader_mappings = { - std::make_shared(impeller_entity_shaders_gles_data, - impeller_entity_shaders_gles_length), - std::make_shared( - impeller_framebuffer_blend_shaders_gles_data, - impeller_framebuffer_blend_shaders_gles_length), + std::make_shared( + impeller_entity_shaders_gles_data, + impeller_entity_shaders_gles_length), + std::make_shared( + impeller_framebuffer_blend_shaders_gles_data, + impeller_framebuffer_blend_shaders_gles_length), #if IMPELLER_ENABLE_3D - std::make_shared(impeller_scene_shaders_gles_data, - impeller_scene_shaders_gles_length), + std::make_shared( + impeller_scene_shaders_gles_data, impeller_scene_shaders_gles_length), #endif // IMPELLER_ENABLE_3D }; - auto context = - impeller::ContextGLES::Create(std::move(proc_table), shader_mappings); + auto context = impeller::ContextGLES::Create( + std::move(proc_table), shader_mappings, enable_gpu_tracing); if (!context) { FML_LOG(ERROR) << "Could not create OpenGLES Impeller Context."; return nullptr; @@ -86,7 +88,8 @@ static std::shared_ptr CreateImpellerContext( } AndroidContextGLImpeller::AndroidContextGLImpeller( - std::unique_ptr display) + std::unique_ptr display, + bool enable_gpu_tracing) : AndroidContext(AndroidRenderingAPI::kOpenGLES), reactor_worker_(std::shared_ptr(new ReactorWorker())), display_(std::move(display)) { @@ -147,7 +150,8 @@ AndroidContextGLImpeller::AndroidContextGLImpeller( return; } - auto impeller_context = CreateImpellerContext(reactor_worker_); + auto impeller_context = + CreateImpellerContext(reactor_worker_, enable_gpu_tracing); if (!impeller_context) { FML_DLOG(ERROR) << "Could not create Impeller context."; diff --git a/shell/platform/android/android_context_gl_impeller.h b/shell/platform/android/android_context_gl_impeller.h index 90466db683a8a..234ff58d2c91e 100644 --- a/shell/platform/android/android_context_gl_impeller.h +++ b/shell/platform/android/android_context_gl_impeller.h @@ -13,8 +13,8 @@ namespace flutter { class AndroidContextGLImpeller : public AndroidContext { public: - explicit AndroidContextGLImpeller( - std::unique_ptr display); + AndroidContextGLImpeller(std::unique_ptr display, + bool enable_gpu_tracing); ~AndroidContextGLImpeller(); diff --git a/shell/platform/android/android_context_gl_impeller_unittests.cc b/shell/platform/android/android_context_gl_impeller_unittests.cc index 452c55fc76261..e8f52832d8f94 100644 --- a/shell/platform/android/android_context_gl_impeller_unittests.cc +++ b/shell/platform/android/android_context_gl_impeller_unittests.cc @@ -45,7 +45,8 @@ TEST(AndroidContextGLImpeller, MSAAFirstAttempt) { .WillOnce(Return(ByMove(std::move(second_result)))); ON_CALL(*display, ChooseConfig(_)) .WillByDefault(Return(ByMove(std::unique_ptr()))); - auto context = std::make_unique(std::move(display)); + auto context = + std::make_unique(std::move(display), true); ASSERT_TRUE(context); } @@ -76,7 +77,8 @@ TEST(AndroidContextGLImpeller, FallbackForEmulator) { .WillOnce(Return(ByMove(std::move(third_result)))); ON_CALL(*display, ChooseConfig(_)) .WillByDefault(Return(ByMove(std::unique_ptr()))); - auto context = std::make_unique(std::move(display)); + auto context = + std::make_unique(std::move(display), true); ASSERT_TRUE(context); } } // namespace testing diff --git a/shell/platform/android/android_context_vulkan_impeller.h b/shell/platform/android/android_context_vulkan_impeller.h index 02e9a305cd039..1800fe00bccc1 100644 --- a/shell/platform/android/android_context_vulkan_impeller.h +++ b/shell/platform/android/android_context_vulkan_impeller.h @@ -14,7 +14,7 @@ namespace flutter { class AndroidContextVulkanImpeller : public AndroidContext { public: - AndroidContextVulkanImpeller(bool enable_validation); + explicit AndroidContextVulkanImpeller(bool enable_validation); ~AndroidContextVulkanImpeller(); diff --git a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java index da114ae3978d7..1bbeefe9c0ae9 100644 --- a/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java +++ b/shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java @@ -45,6 +45,8 @@ public class FlutterLoader { "io.flutter.embedding.android.EnableVulkanValidation"; private static final String IMPELLER_BACKEND_META_DATA_KEY = "io.flutter.embedding.android.ImpellerBackend"; + private static final String IMPELLER_OPENGL_GPU_TRACING_DATA_KEY = + "io.flutter.embedding.android.EnableOpenGLGPUTracing"; private static final String DISABLE_IMAGE_READER_PLATFORM_VIEWS_KEY = "io.flutter.embedding.android.DisableImageReaderPlatformViews"; @@ -340,6 +342,9 @@ public void ensureInitializationComplete( ENABLE_VULKAN_VALIDATION_META_DATA_KEY, areValidationLayersOnByDefault())) { shellArgs.add("--enable-vulkan-validation"); } + if (metaData.getBoolean(IMPELLER_OPENGL_GPU_TRACING_DATA_KEY, false)) { + shellArgs.add("--enable-opengl-gpu-tracing"); + } String backend = metaData.getString(IMPELLER_BACKEND_META_DATA_KEY); if (backend != null) { shellArgs.add("--impeller-backend=" + backend); diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 485f00eb189e8..c4617c4b99cea 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -70,7 +70,8 @@ static std::shared_ptr CreateAndroidContext( uint8_t msaa_samples, bool enable_impeller, const std::optional& impeller_backend, - bool enable_vulkan_validation) { + bool enable_vulkan_validation, + bool enable_opengl_gpu_tracing) { if (use_software_rendering) { return std::make_shared(AndroidRenderingAPI::kSoftware); } @@ -90,7 +91,8 @@ static std::shared_ptr CreateAndroidContext( switch (backend) { case AndroidRenderingAPI::kOpenGLES: return std::make_unique( - std::make_unique()); + std::make_unique(), + enable_opengl_gpu_tracing); case AndroidRenderingAPI::kVulkan: return std::make_unique( enable_vulkan_validation); @@ -99,7 +101,8 @@ static std::shared_ptr CreateAndroidContext( enable_vulkan_validation); if (!vulkan_backend->IsValid()) { return std::make_unique( - std::make_unique()); + std::make_unique(), + enable_opengl_gpu_tracing); } return vulkan_backend; } @@ -131,7 +134,9 @@ PlatformViewAndroid::PlatformViewAndroid( msaa_samples, delegate.OnPlatformViewGetSettings().enable_impeller, delegate.OnPlatformViewGetSettings().impeller_backend, - delegate.OnPlatformViewGetSettings().enable_vulkan_validation)) {} + delegate.OnPlatformViewGetSettings().enable_vulkan_validation, + delegate.OnPlatformViewGetSettings().enable_opengl_gpu_tracing)) { +} PlatformViewAndroid::PlatformViewAndroid( PlatformView::Delegate& delegate, diff --git a/shell/platform/embedder/embedder_surface_gl_impeller.cc b/shell/platform/embedder/embedder_surface_gl_impeller.cc index 340725d2b9d3b..c061c091a912b 100644 --- a/shell/platform/embedder/embedder_surface_gl_impeller.cc +++ b/shell/platform/embedder/embedder_surface_gl_impeller.cc @@ -65,11 +65,12 @@ EmbedderSurfaceGLImpeller::EmbedderSurfaceGLImpeller( gl_dispatch_table_.gl_make_current_callback(); std::vector> shader_mappings = { - std::make_shared(impeller_entity_shaders_gles_data, - impeller_entity_shaders_gles_length), + std::make_shared( + impeller_entity_shaders_gles_data, + impeller_entity_shaders_gles_length), #if IMPELLER_ENABLE_3D - std::make_shared(impeller_scene_shaders_gles_data, - impeller_scene_shaders_gles_length), + std::make_shared( + impeller_scene_shaders_gles_data, impeller_scene_shaders_gles_length), #endif // IMPELLER_ENABLE_3D }; auto gl = std::make_unique( @@ -78,8 +79,8 @@ EmbedderSurfaceGLImpeller::EmbedderSurfaceGLImpeller( return; } - impeller_context_ = - impeller::ContextGLES::Create(std::move(gl), shader_mappings); + impeller_context_ = impeller::ContextGLES::Create( + std::move(gl), shader_mappings, /*enable_gpu_tracing=*/false); if (!impeller_context_) { FML_LOG(ERROR) << "Could not create Impeller context."; From 677d1ce5e5e07909b95539e6db3112daaac0d47c Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Fri, 20 Oct 2023 13:26:15 -0400 Subject: [PATCH 3/9] Roll Dart SDK from ba96a157a8eb to 53fee35b299f (1 revision) (#47165) https://dart.googlesource.com/sdk.git/+log/ba96a157a8eb..53fee35b299f 2023-10-20 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.3.0-48.0.dev If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/dart-sdk-flutter-engine Please CC dart-vm-team@google.com,jimgraham@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter Engine: 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 --- DEPS | 2 +- ci/licenses_golden/licenses_dart | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index 7848724186d52..e33fa9a039108 100644 --- a/DEPS +++ b/DEPS @@ -57,7 +57,7 @@ vars = { # Dart is: https://github.com/dart-lang/sdk/blob/main/DEPS # You can use //tools/dart/create_updated_flutter_deps.py to produce # updated revision list of existing dependencies. - 'dart_revision': 'ba96a157a8eba2baa6d1d34e160eabb99e5b3bb2', + 'dart_revision': '53fee35b299f57c786dc8474ede49353a0009ed4', # WARNING: DO NOT EDIT MANUALLY # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py diff --git a/ci/licenses_golden/licenses_dart b/ci/licenses_golden/licenses_dart index 587befb0a3cab..4b2787dc01161 100644 --- a/ci/licenses_golden/licenses_dart +++ b/ci/licenses_golden/licenses_dart @@ -1,4 +1,4 @@ -Signature: 7fb7754f9d3cde2c73ab74c5f0b5ea8d +Signature: ca89342367a3be73e64cd33c4455c2c8 ==================================================================================================== LIBRARY: dart From 98d3111f59650cc9884dea19b1aec33f28a48b1b Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 20 Oct 2023 10:30:16 -0700 Subject: [PATCH 4/9] [macOS] Eliminate extraneous loadView calls (#47166) Eliminate unnecessary calls to [NSViewController loadView]. To quote the [documentation][loadview] for this method: "Do not call this method. If you require this method to be called, access the view property." In several of the existing tests, we do read viewController.view, and the other tests pass without this line regardless. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../macos/framework/Source/AccessibilityBridgeMacTest.mm | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacTest.mm b/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacTest.mm index f499e80dc642a..1b3c10a00230e 100644 --- a/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacTest.mm +++ b/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMacTest.mm @@ -61,7 +61,6 @@ @implementation AccessibilityBridgeTestViewController TEST(AccessibilityBridgeMacTest, SendsAccessibilityCreateNotificationToWindowOfFlutterView) { FlutterViewController* viewController = CreateTestViewController(); FlutterEngine* engine = viewController.engine; - [viewController loadView]; NSWindow* expectedTarget = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600) styleMask:NSBorderlessWindowMask @@ -122,7 +121,6 @@ @implementation AccessibilityBridgeTestViewController TEST(AccessibilityBridgeMacTest, NonZeroRootNodeId) { FlutterViewController* viewController = CreateTestViewController(); FlutterEngine* engine = viewController.engine; - [viewController loadView]; NSWindow* expectedTarget = [[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 800, 600) styleMask:NSBorderlessWindowMask @@ -192,7 +190,7 @@ @implementation AccessibilityBridgeTestViewController TEST(AccessibilityBridgeMacTest, DoesNotSendAccessibilityCreateNotificationWhenHeadless) { FlutterViewController* viewController = CreateTestViewController(); FlutterEngine* engine = viewController.engine; - [viewController loadView]; + // Setting up bridge so that the AccessibilityBridgeMacDelegateSpy // can query semantics information from. engine.semanticsEnabled = YES; @@ -238,7 +236,6 @@ @implementation AccessibilityBridgeTestViewController TEST(AccessibilityBridgeMacTest, DoesNotSendAccessibilityCreateNotificationWhenNoWindow) { FlutterViewController* viewController = CreateTestViewController(); FlutterEngine* engine = viewController.engine; - [viewController loadView]; // Setting up bridge so that the AccessibilityBridgeMacDelegateSpy // can query semantics information from. From 00ff2c9a617695874b9ec4a7a16d2f4a2bde1382 Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Fri, 20 Oct 2023 13:57:18 -0400 Subject: [PATCH 5/9] Roll Skia from b960e9140f56 to 9ffd5ef9a9ed (3 revisions) (#47167) https://skia.googlesource.com/skia.git/+log/b960e9140f56..9ffd5ef9a9ed 2023-10-20 lehoangquyen@chromium.org GraphiteDawn: don't set active render encoder if BlitWithDraw fails. 2023-10-20 briansalomon@gmail.com [graphite] Make RecorderOptions::kDefaultRecorderBudget a static member 2023-10-20 johnstiles@google.com Remove staging flag SK_IMPROVE_RASTER_PIPELINE_PRECISION. If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/skia-flutter-autoroll Please CC brianosman@google.com,jimgraham@google.com,rmistry@google.com,scroggo@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry 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 --- DEPS | 2 +- ci/licenses_golden/licenses_skia | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index e33fa9a039108..a2b0c98eb78ac 100644 --- a/DEPS +++ b/DEPS @@ -18,7 +18,7 @@ vars = { 'llvm_git': 'https://llvm.googlesource.com', # OCMock is for testing only so there is no google clone 'ocmock_git': 'https://github.com/erikdoe/ocmock.git', - 'skia_revision': 'b960e9140f56675c0a2f6a8664967a56bd3e5158', + 'skia_revision': '9ffd5ef9a9ed8c7eea136be4764778089f04d9df', # WARNING: DO NOT EDIT canvaskit_cipd_instance MANUALLY # See `lib/web_ui/README.md` for how to roll CanvasKit to a new version. diff --git a/ci/licenses_golden/licenses_skia b/ci/licenses_golden/licenses_skia index bee8d62c68cc4..c4de22aad4976 100644 --- a/ci/licenses_golden/licenses_skia +++ b/ci/licenses_golden/licenses_skia @@ -1,4 +1,4 @@ -Signature: 1c400dc7d1eeb2a8c37c54eca028db89 +Signature: b56f7251840fccf9795705be0801d720 ==================================================================================================== LIBRARY: etc1 @@ -392,6 +392,7 @@ FILE: ../../../third_party/skia/relnotes/glbackendsemaphore.md FILE: ../../../third_party/skia/relnotes/grsurface-info.md FILE: ../../../third_party/skia/relnotes/mesh.md FILE: ../../../third_party/skia/relnotes/readbuffer-deserial.md +FILE: ../../../third_party/skia/relnotes/recorder-static-member.md FILE: ../../../third_party/skia/relnotes/typeface.md FILE: ../../../third_party/skia/relnotes/vk-directcontext.md FILE: ../../../third_party/skia/src/gpu/gpu_workaround_list.txt From b385c013066202ed5abeb39a611eb97db65251da Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 20 Oct 2023 11:27:19 -0700 Subject: [PATCH 6/9] Add option to save Impeller failure images in rendertests (#47142) The rendering tests currently always save the impeller failure images into a temporary directory in /tmp which is out of the way and might accumulate over time. The images are now only saved when `--save-impeller-failures` is specified on the command line and they are now saved into a local sub-directory with multiple runs saving into new sub-directories to keep the failure images from getting confused with each other over time. The new image directories are named `./impeller_failure_images/NNNN/*.png` --- .../testing/dl_rendering_unittests.cc | 153 +++++++++++++----- 1 file changed, 114 insertions(+), 39 deletions(-) diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 1704db6488361..26421f9e1efb3 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -13,6 +13,7 @@ #include "flutter/display_list/skia/dl_sk_dispatcher.h" #include "flutter/display_list/testing/dl_test_surface_provider.h" #include "flutter/display_list/utils/dl_comparable.h" +#include "flutter/fml/file.h" #include "flutter/fml/math.h" #include "flutter/testing/display_list_testing.h" #include "flutter/testing/testing.h" @@ -1102,8 +1103,10 @@ class TestParameters { class CanvasCompareTester { public: - static std::vector kTestBackends; - static std::string kTempDirectory; + static std::vector TestBackends; + static std::string ImpellerFailureImageDirectory; + static bool SaveImpellerFailureImages; + static std::vector ImpellerFailureImages; static std::unique_ptr GetProvider(BackendType type) { auto provider = DlSurfaceProvider::Create(type); @@ -1122,7 +1125,7 @@ class CanvasCompareTester { if (!provider) { return false; } - CanvasCompareTester::kTestBackends.push_back(type); + CanvasCompareTester::TestBackends.push_back(type); return true; } @@ -1130,7 +1133,7 @@ class CanvasCompareTester { static void RenderAll(const TestParameters& params, const BoundsTolerance& tolerance = DefaultTolerance) { - for (auto& back_end : kTestBackends) { + for (auto& back_end : TestBackends) { auto provider = GetProvider(back_end); RenderEnvironment env = RenderEnvironment::MakeN32(provider.get()); env.init_ref(kEmptySkSetup, params.sk_renderer(), // @@ -1142,8 +1145,8 @@ class CanvasCompareTester { "Impeller reference")) { std::string test_name = ::testing::UnitTest::GetInstance()->current_test_info()->name(); - impeller_result->write( - to_png_filename(test_name + " (Impeller reference)")); + save_to_png(impeller_result, test_name + " (Impeller reference)", + "base rendering was blank or out of bounds"); } } else { static OncePerBackendWarning warnings("No Impeller output tests"); @@ -2264,16 +2267,75 @@ class CanvasCompareTester { .with_diff_clip()); } - static std::string to_png_filename(const std::string& desc) { - if (kTempDirectory.length() == 0) { - kTempDirectory = fml::CreateTemporaryDirectory(); + enum class DirectoryStatus { + kExisted, + kCreated, + kFailed, + }; + + static DirectoryStatus CheckDir(const std::string& dir) { + auto ret = + fml::OpenDirectory(dir.c_str(), false, fml::FilePermission::kRead); + if (ret.is_valid()) { + return DirectoryStatus::kExisted; + } + ret = + fml::OpenDirectory(dir.c_str(), true, fml::FilePermission::kReadWrite); + if (ret.is_valid()) { + return DirectoryStatus::kCreated; } + FML_LOG(ERROR) << "Could not create directory (" << dir + << ") for impeller failure images" + << ", ret = " << ret.get() << ", errno = " << errno; + return DirectoryStatus::kFailed; + } - std::string ret = kTempDirectory + "/"; - for (const char& ch : desc) { - ret += (ch == ':' || ch == ' ') ? '_' : ch; + static void SetupImpellerFailureImageDirectory() { + std::string base_dir = "./impeller_failure_images"; + if (CheckDir(base_dir) == DirectoryStatus::kFailed) { + return; + } + for (int i = 0; i < 10000; i++) { + std::string sub_dir = std::to_string(i); + while (sub_dir.length() < 4) { + sub_dir = "0" + sub_dir; + } + std::string try_dir = base_dir + "/" + sub_dir; + switch (CheckDir(try_dir)) { + case DirectoryStatus::kExisted: + break; + case DirectoryStatus::kCreated: + ImpellerFailureImageDirectory = try_dir; + return; + case DirectoryStatus::kFailed: + return; + } } - return ret + ".png"; + FML_LOG(ERROR) << "Too many output directories for Impeller failure images"; + } + + static void save_to_png(const RenderResult* result, + const std::string& op_desc, + const std::string& reason) { + if (!SaveImpellerFailureImages) { + return; + } + if (ImpellerFailureImageDirectory.length() == 0) { + SetupImpellerFailureImageDirectory(); + if (ImpellerFailureImageDirectory.length() == 0) { + SaveImpellerFailureImages = false; + return; + } + } + + std::string filename = ImpellerFailureImageDirectory + "/"; + for (const char& ch : op_desc) { + filename += (ch == ':' || ch == ' ') ? '_' : ch; + } + filename = filename + ".png"; + result->write(filename); + ImpellerFailureImages.push_back(filename); + FML_LOG(ERROR) << reason << ": " << filename; } static void RenderWith(const TestParameters& testP, @@ -2352,23 +2414,17 @@ class CanvasCompareTester { env.ref_impeller_result(), imp_result.get(), false, imp_info + " (attribute should affect rendering)"); } - if (!success) { + if (SaveImpellerFailureImages && !success) { FML_LOG(ERROR) << "Impeller issue encountered for: " << *imp_job.MakeDisplayList(base_info); - std::string filename = to_png_filename(info + " (Impeller Output)"); - imp_result->write(filename); - FML_LOG(ERROR) << "output saved in: " << filename; - std::string src_filename = to_png_filename(info + " (Impeller Input)"); - env.ref_impeller_result()->write(src_filename); - FML_LOG(ERROR) << "compare to reference without attributes: " - << src_filename; - std::string sk_filename = to_png_filename(info + " (Skia Output)"); - sk_result->write(sk_filename); - FML_LOG(ERROR) << "and to Skia reference with attributes: " - << sk_filename; - std::string sk_src_filename = to_png_filename(info + " (Skia Input)"); - env.ref_sk_result()->write(sk_src_filename); - FML_LOG(ERROR) << "operating on Skia source image: " << sk_src_filename; + save_to_png(imp_result.get(), info + " (Impeller Result)", + "output saved in"); + save_to_png(env.ref_impeller_result(), info + " (Impeller Reference)", + "compare to reference without attributes"); + save_to_png(sk_result.get(), info + " (Skia Result)", + "and to Skia reference with attributes"); + save_to_png(env.ref_sk_result(), info + " (Skia Reference)", + "and to Skia reference without attributes"); } } @@ -2755,8 +2811,10 @@ class CanvasCompareTester { } }; -std::vector CanvasCompareTester::kTestBackends; -std::string CanvasCompareTester::kTempDirectory = ""; +std::vector CanvasCompareTester::TestBackends; +std::string CanvasCompareTester::ImpellerFailureImageDirectory = ""; +bool CanvasCompareTester::SaveImpellerFailureImages = false; +std::vector CanvasCompareTester::ImpellerFailureImages; BoundsTolerance CanvasCompareTester::DefaultTolerance = BoundsTolerance().addAbsolutePadding(1, 1); @@ -2790,6 +2848,10 @@ class DisplayListRenderingTestBase : public BaseT, for (auto p_arg = std::next(args.begin()); p_arg != args.end(); p_arg++) { std::string arg = *p_arg; bool enable = true; + if (arg == "--save-impeller-failures") { + CanvasCompareTester::SaveImpellerFailureImages = true; + continue; + } if (StartsWith(arg, "--no")) { enable = false; arg = "-" + arg.substr(4); @@ -2812,12 +2874,25 @@ class DisplayListRenderingTestBase : public BaseT, CanvasCompareTester::AddProvider(BackendType::kMetalBackend); } std::string providers = ""; - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { providers += " " + DlSurfaceProvider::BackendName(back_end); } FML_LOG(INFO) << "Running tests on [" << providers << " ]"; } + static void TearDownTestSuite() { + if (CanvasCompareTester::ImpellerFailureImages.size() > 0) { + FML_LOG(INFO); + FML_LOG(INFO) << CanvasCompareTester::ImpellerFailureImages.size() + << " images saved in " + << CanvasCompareTester::ImpellerFailureImageDirectory; + for (auto filename : CanvasCompareTester::ImpellerFailureImages) { + FML_LOG(INFO) << " " << filename; + } + FML_LOG(INFO); + } + } + private: FML_DISALLOW_COPY_AND_ASSIGN(DisplayListRenderingTestBase); }; @@ -3811,7 +3886,7 @@ TEST_F(DisplayListRendering, SaveLayerClippedContentStillFilters) { CaseParameters case_params("Filtered SaveLayer with clipped content"); BoundsTolerance tolerance = BoundsTolerance().addAbsolutePadding(6.0f, 6.0f); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); RenderEnvironment env = RenderEnvironment::MakeN32(provider.get()); env.init_ref(kEmptySkSetup, test_params.sk_renderer(), // @@ -3926,7 +4001,7 @@ TEST_F(DisplayListRendering, SaveLayerConsolidation) { bool same, bool rev_same, const std::string& desc1, const std::string& desc2) { - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto env = std::make_unique( provider.get(), PixelFormat::kN32PremulPixelFormat); @@ -4039,7 +4114,7 @@ TEST_F(DisplayListRendering, MatrixColorFilterModifyTransparencyCheck) { builder2.Restore(); auto display_list2 = builder2.Build(); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto env = std::make_unique( provider.get(), PixelFormat::kN32PremulPixelFormat); @@ -4110,7 +4185,7 @@ TEST_F(DisplayListRendering, MatrixColorFilterOpacityCommuteCheck) { builder2.Restore(); auto display_list2 = builder2.Build(); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto env = std::make_unique( provider.get(), PixelFormat::kN32PremulPixelFormat); @@ -4215,7 +4290,7 @@ TEST_F(DisplayListRendering, BlendColorFilterModifyTransparencyCheck) { builder2.Restore(); auto display_list2 = builder2.Build(); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto env = std::make_unique( provider.get(), PixelFormat::kN32PremulPixelFormat); @@ -4279,7 +4354,7 @@ TEST_F(DisplayListRendering, BlendColorFilterOpacityCommuteCheck) { builder2.Restore(); auto display_list2 = builder2.Build(); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto env = std::make_unique( provider.get(), PixelFormat::kN32PremulPixelFormat); @@ -4538,7 +4613,7 @@ class DisplayListNopTest : public DisplayListRendering { SkPaint sk_paint; sk_paint.setBlendMode(sk_mode); sk_paint.setColor(ToSk(color)); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto result_surface = provider->MakeOffscreenSurface( test_image->width(), test_image->height(), @@ -4601,7 +4676,7 @@ class DisplayListNopTest : public DisplayListRendering { sk_paint.setColor(ToSk(color)); sk_paint.setColorFilter(ToSk(color_filter)); sk_paint.setImageFilter(ToSk(image_filter)); - for (auto& back_end : CanvasCompareTester::kTestBackends) { + for (auto& back_end : CanvasCompareTester::TestBackends) { auto provider = CanvasCompareTester::GetProvider(back_end); auto result_surface = provider->MakeOffscreenSurface( w, h, DlSurfaceProvider::kN32PremulPixelFormat); From 8bd829420ae0bb436a57e501189031ffb4c03625 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 20 Oct 2023 11:30:33 -0700 Subject: [PATCH 7/9] Fix async image loading issues in skwasm. (#47117) This fixes https://github.com/flutter/flutter/issues/134045 There were a few different issues here: * We need to do our own message passing for rendering pictures. The async methods provided by emscripten have their own queue that can drain synchronously, so basically it's not guaranteed to be FIFO with other messages sent to the web worker or main thread. * When we drop frames, we should only drop intermediate frames, so that when the rendering flurry stops that the frame that is displayed is the last one that was actually requested. * We need to reset the GL context after lazy image creation, otherwise skia's renderer gets into a bad state for that frame. --- lib/web_ui/lib/src/engine/scene_view.dart | 51 +++++++++++++++++---- lib/web_ui/skwasm/image.cpp | 4 ++ lib/web_ui/skwasm/library_skwasm_support.js | 13 ++++++ lib/web_ui/skwasm/skwasm_support.h | 5 ++ lib/web_ui/skwasm/surface.cpp | 20 ++++---- lib/web_ui/skwasm/surface.h | 7 ++- lib/web_ui/test/engine/scene_view_test.dart | 33 ++++++++++++- 7 files changed, 107 insertions(+), 26 deletions(-) diff --git a/lib/web_ui/lib/src/engine/scene_view.dart b/lib/web_ui/lib/src/engine/scene_view.dart index f1c75c0939d25..41ac3fab96cd0 100644 --- a/lib/web_ui/lib/src/engine/scene_view.dart +++ b/lib/web_ui/lib/src/engine/scene_view.dart @@ -16,6 +16,20 @@ abstract class PictureRenderer { FutureOr renderPicture(ScenePicture picture); } +class _SceneRender { + _SceneRender(this.scene, this._completer) { + scene.beginRender(); + } + + final EngineScene scene; + final Completer _completer; + + void done() { + scene.endRender(); + _completer.complete(); + } +} + // This class builds a DOM tree that composites an `EngineScene`. class EngineSceneView { factory EngineSceneView(PictureRenderer pictureRenderer) { @@ -30,16 +44,38 @@ class EngineSceneView { List containers = []; - int queuedRenders = 0; - static const int kMaxQueuedRenders = 3; + _SceneRender? _currentRender; + _SceneRender? _nextRender; + + Future renderScene(EngineScene scene) { + if (_currentRender != null) { + // If a scene is already queued up, drop it and queue this one up instead + // so that the scene view always displays the most recently requested scene. + _nextRender?.done(); + final Completer completer = Completer(); + _nextRender = _SceneRender(scene, completer); + return completer.future; + } + final Completer completer = Completer(); + _currentRender = _SceneRender(scene, completer); + _kickRenderLoop(); + return completer.future; + } - Future renderScene(EngineScene scene) async { - if (queuedRenders >= kMaxQueuedRenders) { + Future _kickRenderLoop() async { + final _SceneRender current = _currentRender!; + await _renderScene(current.scene); + current.done(); + _currentRender = _nextRender; + _nextRender = null; + if (_currentRender == null) { return; + } else { + return _kickRenderLoop(); } - queuedRenders += 1; + } - scene.beginRender(); + Future _renderScene(EngineScene scene) async { final List slices = scene.rootLayer.slices; final Iterable> renderFutures = slices.map( (LayerSlice slice) async => switch (slice) { @@ -113,9 +149,6 @@ class EngineSceneView { sceneElement.removeChild(currentElement); currentElement = sibling; } - scene.endRender(); - - queuedRenders -= 1; } } diff --git a/lib/web_ui/skwasm/image.cpp b/lib/web_ui/skwasm/image.cpp index 0e7665d0722d2..def216398afa5 100644 --- a/lib/web_ui/skwasm/image.cpp +++ b/lib/web_ui/skwasm/image.cpp @@ -101,6 +101,10 @@ class TextureSourceImageGenerator : public GrExternalTextureGenerator { auto backendTexture = GrBackendTextures::MakeGL( fInfo.width(), fInfo.height(), mipmapped, glInfo); + + // In order to bind the image source to the texture, makeTexture has changed + // which texture is "in focus" for the WebGL context. + GrAsDirectContext(context)->resetContext(kTextureBinding_GrGLBackendState); return std::make_unique( backendTexture, glInfo.fID, emscripten_webgl_get_current_context()); } diff --git a/lib/web_ui/skwasm/library_skwasm_support.js b/lib/web_ui/skwasm/library_skwasm_support.js index 46467e507b60b..5e62614e009c4 100644 --- a/lib/web_ui/skwasm/library_skwasm_support.js +++ b/lib/web_ui/skwasm/library_skwasm_support.js @@ -27,6 +27,9 @@ mergeInto(LibraryManager.library, { return; } switch (skwasmMessage) { + case 'renderPicture': + _surface_renderPictureOnWorker(data.surface, data.picture, data.callbackId); + return; case 'onRenderComplete': _surface_onRenderComplete(data.surface, data.callbackId, data.imageBitmap); return; @@ -51,6 +54,14 @@ mergeInto(LibraryManager.library, { PThread.pthreads[threadId].addEventListener("message", eventListener); } }; + _skwasm_dispatchRenderPicture = function(threadId, surfaceHandle, pictureHandle, callbackId) { + PThread.pthreads[threadId].postMessage({ + skwasmMessage: 'renderPicture', + surface: surfaceHandle, + picture: pictureHandle, + callbackId, + }); + }; _skwasm_createOffscreenCanvas = function(width, height) { const canvas = new OffscreenCanvas(width, height); var contextAttributes = { @@ -114,6 +125,8 @@ mergeInto(LibraryManager.library, { skwasm_disposeAssociatedObjectOnThread__deps: ['$skwasm_support_setup'], skwasm_registerMessageListener: function() {}, skwasm_registerMessageListener__deps: ['$skwasm_support_setup'], + skwasm_dispatchRenderPicture: function() {}, + skwasm_dispatchRenderPicture__deps: ['$skwasm_support_setup'], skwasm_createOffscreenCanvas: function () {}, skwasm_createOffscreenCanvas__deps: ['$skwasm_support_setup'], skwasm_resizeCanvas: function () {}, diff --git a/lib/web_ui/skwasm/skwasm_support.h b/lib/web_ui/skwasm/skwasm_support.h index 21c790d6507eb..0c9cbaaa845c8 100644 --- a/lib/web_ui/skwasm/skwasm_support.h +++ b/lib/web_ui/skwasm/skwasm_support.h @@ -4,6 +4,7 @@ #include #include +#include "third_party/skia/include/core/SkPicture.h" namespace Skwasm { class Surface; @@ -19,6 +20,10 @@ extern SkwasmObject skwasm_getAssociatedObject(void* pointer); extern void skwasm_disposeAssociatedObjectOnThread(unsigned long threadId, void* pointer); extern void skwasm_registerMessageListener(pthread_t threadId); +extern void skwasm_dispatchRenderPicture(unsigned long threadId, + Skwasm::Surface* surface, + SkPicture* picture, + uint32_t callbackId); extern uint32_t skwasm_createOffscreenCanvas(int width, int height); extern void skwasm_resizeCanvas(uint32_t contextHandle, int width, int height); extern void skwasm_captureImageBitmap(Skwasm::Surface* surfaceHandle, diff --git a/lib/web_ui/skwasm/surface.cpp b/lib/web_ui/skwasm/surface.cpp index cebb4889faf2b..28b64bd25a328 100644 --- a/lib/web_ui/skwasm/surface.cpp +++ b/lib/web_ui/skwasm/surface.cpp @@ -43,9 +43,7 @@ uint32_t Surface::renderPicture(SkPicture* picture) { assert(emscripten_is_main_browser_thread()); uint32_t callbackId = ++_currentCallbackId; picture->ref(); - emscripten_dispatch_to_thread(_thread, EM_FUNC_SIG_VIII, - reinterpret_cast(fRenderPicture), - nullptr, this, picture, callbackId); + skwasm_dispatchRenderPicture(_thread, this, picture, callbackId); return callbackId; } @@ -138,7 +136,7 @@ void Surface::_recreateSurface() { } // Worker thread only -void Surface::_renderPicture(const SkPicture* picture, uint32_t callbackId) { +void Surface::renderPictureOnWorker(SkPicture* picture, uint32_t callbackId) { SkRect pictureRect = picture->cullRect(); SkIRect roundedOutRect; pictureRect.roundOut(&roundedOutRect); @@ -195,13 +193,6 @@ void Surface::fDispose(Surface* surface) { surface->_dispose(); } -void Surface::fRenderPicture(Surface* surface, - SkPicture* picture, - uint32_t callbackId) { - surface->_renderPicture(picture, callbackId); - picture->unref(); -} - void Surface::fOnRasterizeComplete(Surface* surface, SkData* imageData, uint32_t callbackId) { @@ -239,6 +230,13 @@ SKWASM_EXPORT uint32_t surface_renderPicture(Surface* surface, return surface->renderPicture(picture); } +SKWASM_EXPORT void surface_renderPictureOnWorker(Surface* surface, + SkPicture* picture, + uint32_t callbackId) { + surface->renderPictureOnWorker(picture, callbackId); + picture->unref(); +} + SKWASM_EXPORT uint32_t surface_rasterizeImage(Surface* surface, SkImage* image, ImageByteFormat format) { diff --git a/lib/web_ui/skwasm/surface.h b/lib/web_ui/skwasm/surface.h index c7cd137a00fb5..d99164831e523 100644 --- a/lib/web_ui/skwasm/surface.h +++ b/lib/web_ui/skwasm/surface.h @@ -70,13 +70,15 @@ class Surface { std::unique_ptr createTextureSourceWrapper( SkwasmObject textureSource); + // Worker thread + void renderPictureOnWorker(SkPicture* picture, uint32_t callbackId); + private: void _runWorker(); void _init(); void _dispose(); void _resizeCanvasToFit(int width, int height); void _recreateSurface(); - void _renderPicture(const SkPicture* picture, uint32_t callbackId); void _rasterizeImage(SkImage* image, ImageByteFormat format, uint32_t callbackId); @@ -99,9 +101,6 @@ class Surface { pthread_t _thread; static void fDispose(Surface* surface); - static void fRenderPicture(Surface* surface, - SkPicture* picture, - uint32_t callbackId); static void fOnRenderComplete(Surface* surface, uint32_t callbackId, SkwasmObject imageBitmap); diff --git a/lib/web_ui/test/engine/scene_view_test.dart b/lib/web_ui/test/engine/scene_view_test.dart index cea07e15294ab..2b83d1e53d40d 100644 --- a/lib/web_ui/test/engine/scene_view_test.dart +++ b/lib/web_ui/test/engine/scene_view_test.dart @@ -25,6 +25,7 @@ class StubPictureRenderer implements PictureRenderer { @override Future renderPicture(ScenePicture picture) async { + renderedPictures.add(picture); final ui.Rect cullRect = picture.cullRect; final DomImageBitmap bitmap = (await createImageBitmap( scratchCanvasElement, @@ -32,12 +33,16 @@ class StubPictureRenderer implements PictureRenderer { ).toDart)! as DomImageBitmap; return bitmap; } + + List renderedPictures = []; } void testMain() { late EngineSceneView sceneView; + late StubPictureRenderer stubPictureRenderer; setUp(() { - sceneView = EngineSceneView(StubPictureRenderer()); + stubPictureRenderer = StubPictureRenderer(); + sceneView = EngineSceneView(stubPictureRenderer); }); test('SceneView places canvas according to device-pixel ratio', () async { @@ -72,7 +77,7 @@ void testMain() { debugOverrideDevicePixelRatio(null); }); - test('SceneView places canvas according to device-pixel ratio', () async { + test('SceneView places platform view according to device-pixel ratio', () async { debugOverrideDevicePixelRatio(2.0); final PlatformView platformView = PlatformView( @@ -101,4 +106,28 @@ void testMain() { debugOverrideDevicePixelRatio(null); }); + + test('SceneView always renders most recent picture and skips intermediate pictures', () async { + final List pictures = []; + final List> renderFutures = >[]; + for (int i = 1; i < 20; i++) { + final StubPicture picture = StubPicture(const ui.Rect.fromLTWH( + 50, + 80, + 100, + 120, + )); + pictures.add(picture); + final EngineRootLayer rootLayer = EngineRootLayer(); + rootLayer.slices.add(PictureSlice(picture)); + final EngineScene scene = EngineScene(rootLayer); + renderFutures.add(sceneView.renderScene(scene)); + } + await Future.wait(renderFutures); + + // Should just render the first and last pictures and skip the one inbetween. + expect(stubPictureRenderer.renderedPictures.length, 2); + expect(stubPictureRenderer.renderedPictures.first, pictures.first); + expect(stubPictureRenderer.renderedPictures.last, pictures.last); + }); } From 471fbc58d88462f640379c915311577a622030a7 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 20 Oct 2023 15:17:36 -0400 Subject: [PATCH 8/9] [web] Support `flutterViewId` in platform view messages (#46891) - Accept a new `flutterViewId` field in platform view messages. - Keep transitory support for legacy platform view messages that don't contain `flutterViewId`. - Default view factories set `width:100%` and `height:100%`. --- .../lib/src/engine/platform_dispatcher.dart | 60 ++-- .../platform_views/content_manager.dart | 4 +- .../platform_views/message_handler.dart | 84 ++++-- lib/web_ui/lib/src/engine/util.dart | 22 ++ .../legacy_message_handler_test.dart | 265 ++++++++++++++++++ .../platform_views/message_handler_test.dart | 141 +++++++--- 6 files changed, 477 insertions(+), 99 deletions(-) create mode 100644 lib/web_ui/test/engine/platform_views/legacy_message_handler_test.dart diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index 51bc3a394cff5..5d667c51ccf28 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -21,6 +21,9 @@ ui.VoidCallback? scheduleFrameCallback; typedef HighContrastListener = void Function(bool enabled); typedef _KeyDataResponseCallback = void Function(bool handled); +const StandardMethodCodec standardCodec = StandardMethodCodec(); +const JSONMethodCodec jsonCodec = JSONMethodCodec(); + /// Determines if high contrast is enabled using media query 'forced-colors: active' for Windows class HighContrastSupport { static HighContrastSupport instance = HighContrastSupport(); @@ -129,13 +132,13 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { /// The current list of windows. @override - Iterable get views => viewData.values; - final Map viewData = {}; + Iterable get views => viewData.values; + final Map viewData = {}; /// Returns the [FlutterView] with the provided ID if one exists, or null /// otherwise. @override - ui.FlutterView? view({required int id}) => viewData[id]; + EngineFlutterView? view({required int id}) => viewData[id]; /// A map of opaque platform window identifiers to window configurations. /// @@ -470,8 +473,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { /// This should be in sync with shell/common/shell.cc case 'flutter/skia': - const MethodCodec codec = JSONMethodCodec(); - final MethodCall decoded = codec.decodeMethodCall(data); + final MethodCall decoded = jsonCodec.decodeMethodCall(data); switch (decoded.method) { case 'Skia.setResourceCacheMaxBytes': if (renderer is CanvasKitRenderer) { @@ -486,7 +488,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { // Also respond in HTML mode. Otherwise, apps would have to detect // CanvasKit vs HTML before invoking this method. replyToPlatformMessage( - callback, codec.encodeSuccessEnvelope([true])); + callback, jsonCodec.encodeSuccessEnvelope([true])); } return; @@ -496,8 +498,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { return; case 'flutter/platform': - const MethodCodec codec = JSONMethodCodec(); - final MethodCall decoded = codec.decodeMethodCall(data); + final MethodCall decoded = jsonCodec.decodeMethodCall(data); switch (decoded.method) { case 'SystemNavigator.pop': // TODO(a-wallen): As multi-window support expands, the pop call @@ -505,13 +506,13 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { // supported. implicitView!.browserHistory.exit().then((_) { replyToPlatformMessage( - callback, codec.encodeSuccessEnvelope(true)); + callback, jsonCodec.encodeSuccessEnvelope(true)); }); return; case 'HapticFeedback.vibrate': final String? type = decoded.arguments as String?; vibrate(_getHapticFeedbackDuration(type)); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; case 'SystemChrome.setApplicationSwitcherDescription': final Map arguments = decoded.arguments as Map; @@ -520,24 +521,24 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { final int primaryColor = arguments['primaryColor'] as int? ?? 0xFF000000; domDocument.title = label; setThemeColor(ui.Color(primaryColor)); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; case 'SystemChrome.setSystemUIOverlayStyle': final Map arguments = decoded.arguments as Map; final int? statusBarColor = arguments['statusBarColor'] as int?; setThemeColor(statusBarColor == null ? null : ui.Color(statusBarColor)); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; case 'SystemChrome.setPreferredOrientations': final List arguments = decoded.arguments as List; ScreenOrientation.instance.setPreferredOrientation(arguments).then((bool success) { replyToPlatformMessage( - callback, codec.encodeSuccessEnvelope(success)); + callback, jsonCodec.encodeSuccessEnvelope(success)); }); return; case 'SystemSound.play': // There are no default system sounds on web. - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; case 'Clipboard.setData': ClipboardMessageHandler().setDataMethodCall(decoded, callback); @@ -560,23 +561,21 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { return; case 'flutter/contextmenu': - const MethodCodec codec = JSONMethodCodec(); - final MethodCall decoded = codec.decodeMethodCall(data); + final MethodCall decoded = jsonCodec.decodeMethodCall(data); switch (decoded.method) { case 'enableContextMenu': implicitView!.contextMenu.enable(); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; case 'disableContextMenu': implicitView!.contextMenu.disable(); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); return; } return; case 'flutter/mousecursor': - const MethodCodec codec = StandardMethodCodec(); - final MethodCall decoded = codec.decodeMethodCall(data); + final MethodCall decoded = standardCodec.decodeMethodCall(data); final Map arguments = decoded.arguments as Map; switch (decoded.method) { case 'activateSystemCursor': @@ -585,15 +584,21 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { return; case 'flutter/web_test_e2e': - const MethodCodec codec = JSONMethodCodec(); replyToPlatformMessage( callback, - codec.encodeSuccessEnvelope( - _handleWebTestEnd2EndMessage(codec, data))); + jsonCodec.encodeSuccessEnvelope( + _handleWebTestEnd2EndMessage(jsonCodec, data))); return; case 'flutter/platform_views': - implicitView!.platformViewMessageHandler.handlePlatformViewCall(data, callback!); + final MethodCall(:String method, :dynamic arguments) = standardCodec.decodeMethodCall(data); + final int? flutterViewId = tryViewId(arguments); + if (flutterViewId == null) { + implicitView!.platformViewMessageHandler.handleLegacyPlatformViewCall(method, arguments, callback!); + return; + } + arguments as Map; + viewData[flutterViewId]!.platformViewMessageHandler.handlePlatformViewCall(method, arguments, callback!); return; case 'flutter/accessibility': @@ -609,8 +614,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { // supported. implicitView!.handleNavigationMessage(data).then((bool handled) { if (handled) { - const MethodCodec codec = JSONMethodCodec(); - replyToPlatformMessage(callback, codec.encodeSuccessEnvelope(true)); + replyToPlatformMessage(callback, jsonCodec.encodeSuccessEnvelope(true)); } else { callback?.call(null); } @@ -1350,7 +1354,7 @@ class ViewConfiguration { }); ViewConfiguration copyWith({ - ui.FlutterView? view, + EngineFlutterView? view, double? devicePixelRatio, ui.Rect? geometry, bool? visible, @@ -1375,7 +1379,7 @@ class ViewConfiguration { ); } - final ui.FlutterView? view; + final EngineFlutterView? view; final double devicePixelRatio; final ui.Rect geometry; final bool visible; diff --git a/lib/web_ui/lib/src/engine/platform_views/content_manager.dart b/lib/web_ui/lib/src/engine/platform_views/content_manager.dart index a9ed363417caf..0b0e35270ff66 100644 --- a/lib/web_ui/lib/src/engine/platform_views/content_manager.dart +++ b/lib/web_ui/lib/src/engine/platform_views/content_manager.dart @@ -249,5 +249,7 @@ DomElement _defaultFactory( }) { params!; params as Map; - return domDocument.createElement(params.readString('tagName')); + return domDocument.createElement(params.readString('tagName')) + ..style.width = '100%' + ..style.height = '100%'; } diff --git a/lib/web_ui/lib/src/engine/platform_views/message_handler.dart b/lib/web_ui/lib/src/engine/platform_views/message_handler.dart index 68c0fe340cac8..9e20b3674ada3 100644 --- a/lib/web_ui/lib/src/engine/platform_views/message_handler.dart +++ b/lib/web_ui/lib/src/engine/platform_views/message_handler.dart @@ -53,7 +53,7 @@ class PlatformViewMessageHandler { /// Handle a `create` Platform View message. /// - /// This will attempt to render the `contents` and of a Platform View, if its + /// This will attempt to render the `contents` of a Platform View, if its /// `viewType` has been registered previously. /// /// (See [PlatformViewManager.registerFactory] for more details.) @@ -63,37 +63,34 @@ class PlatformViewMessageHandler { /// If all goes well, this function will `callback` with an empty success envelope. /// In case of error, this will `callback` with an error envelope describing the error. void _createPlatformView( - MethodCall methodCall, - _PlatformMessageResponseCallback callback, - ) { - final Map args = methodCall.arguments as Map; - final int viewId = args.readInt('id'); - final String viewType = args.readString('viewType'); - final Object? params = args['params']; - - if (!_contentManager.knowsViewType(viewType)) { + _PlatformMessageResponseCallback callback, { + required int platformViewId, + required String platformViewType, + required Object? params, + }) { + if (!_contentManager.knowsViewType(platformViewType)) { callback(_codec.encodeErrorEnvelope( code: 'unregistered_view_type', message: 'A HtmlElementView widget is trying to create a platform view ' - 'with an unregistered type: <$viewType>.', + 'with an unregistered type: <$platformViewType>.', details: 'If you are the author of the PlatformView, make sure ' '`registerViewFactory` is invoked.', )); return; } - if (_contentManager.knowsViewId(viewId)) { + if (_contentManager.knowsViewId(platformViewId)) { callback(_codec.encodeErrorEnvelope( code: 'recreating_view', message: 'trying to create an already created view', - details: 'view id: $viewId', + details: 'view id: $platformViewId', )); return; } final DomElement content = _contentManager.renderContent( - viewType, - viewId, + platformViewType, + platformViewId, params, ); @@ -106,7 +103,7 @@ class PlatformViewMessageHandler { /// Handle a `dispose` Platform View message. /// /// This will clear the cached information that the framework has about a given - /// `viewId`, through the [_contentManager]. + /// `platformViewId`, through the [_contentManager]. /// /// Once that's done, the dispose call is delegated to the [_disposeHandler] /// function, so the active rendering backend can dispose of whatever resources @@ -114,34 +111,67 @@ class PlatformViewMessageHandler { /// /// This function should always `callback` with an empty success envelope. void _disposePlatformView( - MethodCall methodCall, - _PlatformMessageResponseCallback callback, - ) { - final int viewId = methodCall.arguments as int; - + _PlatformMessageResponseCallback callback, { + required int platformViewId, + }) { // The contentManager removes the slot and the contents from its internal // cache, and the DOM. - _contentManager.clearPlatformView(viewId); + _contentManager.clearPlatformView(platformViewId); callback(_codec.encodeSuccessEnvelope(null)); } + /// Handles legacy PlatformViewCalls that don't contain a Flutter View ID. + /// + /// This is transitional code to support the old platform view channel. As + /// soon as the framework code is updated to send the Flutter View ID, this + /// method can be removed. + void handleLegacyPlatformViewCall( + String method, + dynamic arguments, + _PlatformMessageResponseCallback callback, + ) { + switch (method) { + case 'create': + arguments as Map; + _createPlatformView( + callback, + platformViewId: arguments.readInt('id'), + platformViewType: arguments.readString('viewType'), + params: arguments['params'], + ); + return; + case 'dispose': + _disposePlatformView(callback, platformViewId: arguments as int); + return; + } + callback(null); + } + /// Handles a PlatformViewCall to the `flutter/platform_views` channel. /// /// This method handles two possible messages: /// * `create`: See [_createPlatformView] /// * `dispose`: See [_disposePlatformView] void handlePlatformViewCall( - ByteData? data, + String method, + Map arguments, _PlatformMessageResponseCallback callback, ) { - final MethodCall decoded = _codec.decodeMethodCall(data); - switch (decoded.method) { + switch (method) { case 'create': - _createPlatformView(decoded, callback); + _createPlatformView( + callback, + platformViewId: arguments.readInt('platformViewId'), + platformViewType: arguments.readString('platformViewType'), + params: arguments['params'], + ); return; case 'dispose': - _disposePlatformView(decoded, callback); + _disposePlatformView( + callback, + platformViewId: arguments.readInt('platformViewId'), + ); return; } callback(null); diff --git a/lib/web_ui/lib/src/engine/util.dart b/lib/web_ui/lib/src/engine/util.dart index bd4ad5b0ef333..d6581459b60c2 100644 --- a/lib/web_ui/lib/src/engine/util.dart +++ b/lib/web_ui/lib/src/engine/util.dart @@ -14,6 +14,7 @@ import 'package:ui/ui.dart' as ui; import 'browser_detection.dart'; import 'dom.dart'; import 'safe_browser_api.dart'; +import 'services.dart'; import 'vector_math.dart'; /// Generic callback signature, used by [_futurize]. @@ -627,6 +628,27 @@ extension JsonExtensions on Map { } } +/// Extracts view ID from the [MethodCall.arguments] map. +/// +/// Throws if the view ID is not present or if [arguments] is not a map. +int readViewId(Object? arguments) { + final int? viewId = tryViewId(arguments); + if (viewId == null) { + throw Exception('Could not find a `viewId` in the arguments: $arguments'); + } + return viewId; +} + +/// Extracts view ID from the [MethodCall.arguments] map. +/// +/// Returns null if the view ID is not present or if [arguments] is not a map. +int? tryViewId(Object? arguments) { + if (arguments is Map) { + return arguments.tryInt('viewId'); + } + return null; +} + /// Prints a list of bytes in hex format. /// /// Bytes are separated by one space and are padded on the left to always show diff --git a/lib/web_ui/test/engine/platform_views/legacy_message_handler_test.dart b/lib/web_ui/test/engine/platform_views/legacy_message_handler_test.dart new file mode 100644 index 0000000000000..2c9f47725438f --- /dev/null +++ b/lib/web_ui/test/engine/platform_views/legacy_message_handler_test.dart @@ -0,0 +1,265 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; +import 'dart:typed_data'; + +import 'package:test/bootstrap/browser.dart'; +import 'package:test/test.dart'; +import 'package:ui/src/engine.dart'; + +void main() { + internalBootstrapBrowserTest(() => testMain); +} + +const MethodCodec codec = StandardMethodCodec(); + +typedef PlatformViewFactoryCall = ({int viewId, Object? params}); + +void testMain() { + group('PlatformViewMessageHandler', () { + group('handlePlatformViewCall', () { + const String viewType = 'forTest'; + const int viewId = 6; + late PlatformViewManager contentManager; + late Completer completer; + + setUp(() { + contentManager = PlatformViewManager(); + completer = Completer(); + }); + + group('"create" message', () { + test('unregistered viewType, fails with descriptive exception', + () async { + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + final Map arguments = _getCreateArguments(viewType, viewId); + + messageHandler.handleLegacyPlatformViewCall('create', arguments, completer.complete); + + final ByteData? response = await completer.future; + try { + codec.decodeEnvelope(response!); + } on PlatformException catch (e) { + expect(e.code, 'unregistered_view_type'); + expect(e.message, contains(viewType)); + expect(e.details, contains('registerViewFactory')); + } + }); + + test('duplicate viewId, fails with descriptive exception', () async { + contentManager.registerFactory( + viewType, (int id) => createDomHTMLDivElement()); + contentManager.renderContent(viewType, viewId, null); + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + final Map arguments = _getCreateArguments(viewType, viewId); + + messageHandler.handleLegacyPlatformViewCall('create', arguments, completer.complete); + + final ByteData? response = await completer.future; + try { + codec.decodeEnvelope(response!); + } on PlatformException catch (e) { + expect(e.code, 'recreating_view'); + expect(e.details, contains('$viewId')); + } + }); + + test('returns a successEnvelope when the view is created normally', + () async { + contentManager.registerFactory( + viewType, (int id) => createDomHTMLDivElement()..id = 'success'); + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + final Map arguments = _getCreateArguments(viewType, viewId); + + messageHandler.handleLegacyPlatformViewCall('create', arguments, completer.complete); + + final ByteData? response = await completer.future; + expect(codec.decodeEnvelope(response!), isNull, + reason: + 'The response should be a success envelope, with null in it.'); + }); + + test('inserts the created view into the platformViewsContainer', + () async { + final DomElement platformViewsContainer = createDomElement('pv-container'); + contentManager.registerFactory( + viewType, (int id) => createDomHTMLDivElement()..id = 'success'); + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: platformViewsContainer, + contentManager: contentManager, + ); + final Map arguments = _getCreateArguments(viewType, viewId); + + messageHandler.handleLegacyPlatformViewCall('create', arguments, completer.complete); + + final ByteData? response = await completer.future; + + expect( + platformViewsContainer.children.single, + isNotNull, + reason: 'The container has a single child, the created view.', + ); + final DomElement platformView = platformViewsContainer.children.single; + expect( + platformView.querySelector('div#success'), + isNotNull, + reason: 'The element created by the factory should be present in the created view.', + ); + expect( + codec.decodeEnvelope(response!), + isNull, + reason: 'The response should be a success envelope, with null in it.', + ); + }); + + test('passes creation params to the factory', () async { + final List factoryCalls = []; + contentManager.registerFactory(viewType, (int viewId, {Object? params}) { + factoryCalls.add((viewId: viewId, params: params)); + return createDomHTMLDivElement(); + }); + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + + final List> completers = >[]; + + completers.add(Completer()); + messageHandler.handleLegacyPlatformViewCall( + 'create', + _getCreateArguments(viewType, 111), + completers.last.complete, + ); + + completers.add(Completer()); + messageHandler.handleLegacyPlatformViewCall( + 'create', + _getCreateArguments(viewType, 222, {'foo': 'bar'}), + completers.last.complete, + ); + + completers.add(Completer()); + messageHandler.handleLegacyPlatformViewCall( + 'create', + _getCreateArguments(viewType, 333, 'foobar'), + completers.last.complete, + ); + + completers.add(Completer()); + messageHandler.handleLegacyPlatformViewCall( + 'create', + _getCreateArguments(viewType, 444, [1, null, 'str']), + completers.last.complete, + ); + + final List responses = await Future.wait( + completers.map((Completer c) => c.future), + ); + + for (final ByteData? response in responses) { + expect( + codec.decodeEnvelope(response!), + isNull, + reason: 'The response should be a success envelope, with null in it.', + ); + } + + expect(factoryCalls, hasLength(4)); + expect(factoryCalls[0].viewId, 111); + expect(factoryCalls[0].params, isNull); + expect(factoryCalls[1].viewId, 222); + expect(factoryCalls[1].params, {'foo': 'bar'}); + expect(factoryCalls[2].viewId, 333); + expect(factoryCalls[2].params, 'foobar'); + expect(factoryCalls[3].viewId, 444); + expect(factoryCalls[3].params, [1, null, 'str']); + }); + + test('fails if the factory returns a non-DOM object', () async { + contentManager.registerFactory(viewType, (int viewId) { + // Return an object that's not a DOM element. + return Object(); + }); + + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + final Map arguments = _getCreateArguments(viewType, viewId); + + expect(() { + messageHandler.handleLegacyPlatformViewCall('create', arguments, (_) {}); + }, throwsA(isA())); + }); + }); + + group('"dispose" message', () { + late Completer viewIdCompleter; + + setUp(() { + viewIdCompleter = Completer(); + }); + + test('never fails, even for unknown viewIds', () async { + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: contentManager, + ); + + messageHandler.handleLegacyPlatformViewCall('dispose', viewId, completer.complete); + + final ByteData? response = await completer.future; + expect(codec.decodeEnvelope(response!), isNull, + reason: + 'The response should be a success envelope, with null in it.'); + }); + + test('never fails, even for unknown viewIds', () async { + final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( + platformViewsContainer: createDomElement('div'), + contentManager: _FakePlatformViewManager(viewIdCompleter.complete), + ); + + messageHandler.handleLegacyPlatformViewCall('dispose', viewId, completer.complete); + + final int disposedViewId = await viewIdCompleter.future; + expect(disposedViewId, viewId, + reason: + 'The viewId to dispose should be passed to the contentManager'); + }); + }); + }); + }); +} + +class _FakePlatformViewManager extends PlatformViewManager { + _FakePlatformViewManager(void Function(int) clearFunction) + : _clearPlatformView = clearFunction; + + final void Function(int) _clearPlatformView; + + @override + void clearPlatformView(int viewId) { + return _clearPlatformView(viewId); + } +} + +Map _getCreateArguments(String viewType, int viewId, [Object? params]) { + return { + 'id': viewId, + 'viewType': viewType, + if (params != null) 'params': params, + }; +} diff --git a/lib/web_ui/test/engine/platform_views/message_handler_test.dart b/lib/web_ui/test/engine/platform_views/message_handler_test.dart index e1b28b68b954d..344de91e04ae4 100644 --- a/lib/web_ui/test/engine/platform_views/message_handler_test.dart +++ b/lib/web_ui/test/engine/platform_views/message_handler_test.dart @@ -20,8 +20,8 @@ typedef PlatformViewFactoryCall = ({int viewId, Object? params}); void testMain() { group('PlatformViewMessageHandler', () { group('handlePlatformViewCall', () { - const String viewType = 'forTest'; - const int viewId = 6; + const String platformViewType = 'forTest'; + const int platformViewId = 6; late PlatformViewManager contentManager; late Completer completer; @@ -37,52 +37,64 @@ void testMain() { platformViewsContainer: createDomElement('div'), contentManager: contentManager, ); - final ByteData? message = _getCreateMessage(viewType, viewId); + final Map arguments = _getCreateArguments( + platformViewType: platformViewType, + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('create', arguments, completer.complete); final ByteData? response = await completer.future; try { codec.decodeEnvelope(response!); } on PlatformException catch (e) { expect(e.code, 'unregistered_view_type'); - expect(e.message, contains(viewType)); + expect(e.message, contains(platformViewType)); expect(e.details, contains('registerViewFactory')); } }); test('duplicate viewId, fails with descriptive exception', () async { contentManager.registerFactory( - viewType, (int id) => createDomHTMLDivElement()); - contentManager.renderContent(viewType, viewId, null); + platformViewType, (int id) => createDomHTMLDivElement()); + contentManager.renderContent(platformViewType, platformViewId, null); final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( platformViewsContainer: createDomElement('div'), contentManager: contentManager, ); - final ByteData? message = _getCreateMessage(viewType, viewId); + final Map arguments = _getCreateArguments( + platformViewType: platformViewType, + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('create', arguments, completer.complete); final ByteData? response = await completer.future; try { codec.decodeEnvelope(response!); } on PlatformException catch (e) { expect(e.code, 'recreating_view'); - expect(e.details, contains('$viewId')); + expect(e.details, contains('$platformViewId')); } }); test('returns a successEnvelope when the view is created normally', () async { contentManager.registerFactory( - viewType, (int id) => createDomHTMLDivElement()..id = 'success'); + platformViewType, (int id) => createDomHTMLDivElement()..id = 'success'); final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( platformViewsContainer: createDomElement('div'), contentManager: contentManager, ); - final ByteData? message = _getCreateMessage(viewType, viewId); + final Map arguments = _getCreateArguments( + platformViewType: platformViewType, + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('create', arguments, completer.complete); final ByteData? response = await completer.future; expect(codec.decodeEnvelope(response!), isNull, @@ -94,14 +106,18 @@ void testMain() { () async { final DomElement platformViewsContainer = createDomElement('pv-container'); contentManager.registerFactory( - viewType, (int id) => createDomHTMLDivElement()..id = 'success'); + platformViewType, (int id) => createDomHTMLDivElement()..id = 'success'); final PlatformViewMessageHandler messageHandler = PlatformViewMessageHandler( platformViewsContainer: platformViewsContainer, contentManager: contentManager, ); - final ByteData? message = _getCreateMessage(viewType, viewId); + final Map arguments = _getCreateArguments( + platformViewType: platformViewType, + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('create', arguments, completer.complete); final ByteData? response = await completer.future; @@ -125,7 +141,7 @@ void testMain() { test('passes creation params to the factory', () async { final List factoryCalls = []; - contentManager.registerFactory(viewType, (int viewId, {Object? params}) { + contentManager.registerFactory(platformViewType, (int viewId, {Object? params}) { factoryCalls.add((viewId: viewId, params: params)); return createDomHTMLDivElement(); }); @@ -138,25 +154,48 @@ void testMain() { completers.add(Completer()); messageHandler.handlePlatformViewCall( - _getCreateMessage(viewType, 111), + 'create', + _getCreateArguments( + platformViewType: platformViewType, + platformViewId: 111, + viewId: kImplicitViewId, + ), completers.last.complete, ); completers.add(Completer()); messageHandler.handlePlatformViewCall( - _getCreateMessage(viewType, 222, {'foo': 'bar'}), + 'create', + _getCreateArguments( + platformViewType: platformViewType, + platformViewId: 222, + viewId: kImplicitViewId, + params: {'foo': 'bar'}, + ), completers.last.complete, ); completers.add(Completer()); messageHandler.handlePlatformViewCall( - _getCreateMessage(viewType, 333, 'foobar'), + 'create', + _getCreateArguments( + platformViewType: platformViewType, + platformViewId: 333, + viewId: kImplicitViewId, + params: 'foobar', + ), completers.last.complete, ); completers.add(Completer()); messageHandler.handlePlatformViewCall( - _getCreateMessage(viewType, 444, [1, null, 'str']), + 'create', + _getCreateArguments( + platformViewType: platformViewType, + platformViewId: 444, + viewId: kImplicitViewId, + params: [1, null, 'str'], + ), completers.last.complete, ); @@ -184,7 +223,7 @@ void testMain() { }); test('fails if the factory returns a non-DOM object', () async { - contentManager.registerFactory(viewType, (int viewId) { + contentManager.registerFactory(platformViewType, (int viewId) { // Return an object that's not a DOM element. return Object(); }); @@ -193,10 +232,14 @@ void testMain() { platformViewsContainer: createDomElement('div'), contentManager: contentManager, ); - final ByteData? message = _getCreateMessage(viewType, viewId); + final Map arguments = _getCreateArguments( + platformViewType: platformViewType, + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); expect(() { - messageHandler.handlePlatformViewCall(message, (_) {}); + messageHandler.handlePlatformViewCall('create', arguments, (_) {}); }, throwsA(isA())); }); }); @@ -213,9 +256,12 @@ void testMain() { platformViewsContainer: createDomElement('div'), contentManager: contentManager, ); - final ByteData? message = _getDisposeMessage(viewId); + final Map arguments = _getDisposeArguments( + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('dispose', arguments, completer.complete); final ByteData? response = await completer.future; expect(codec.decodeEnvelope(response!), isNull, @@ -228,12 +274,15 @@ void testMain() { platformViewsContainer: createDomElement('div'), contentManager: _FakePlatformViewManager(viewIdCompleter.complete), ); - final ByteData? message = _getDisposeMessage(viewId); + final Map arguments = _getDisposeArguments( + platformViewId: platformViewId, + viewId: kImplicitViewId, + ); - messageHandler.handlePlatformViewCall(message, completer.complete); + messageHandler.handlePlatformViewCall('dispose', arguments, completer.complete); final int disposedViewId = await viewIdCompleter.future; - expect(disposedViewId, viewId, + expect(disposedViewId, platformViewId, reason: 'The viewId to dispose should be passed to the contentManager'); }); @@ -254,20 +303,26 @@ class _FakePlatformViewManager extends PlatformViewManager { } } -ByteData? _getCreateMessage(String viewType, int viewId, [Object? params]) { - return codec.encodeMethodCall(MethodCall( - 'create', - { - 'id': viewId, - 'viewType': viewType, - if (params != null) 'params': params, - }, - )); +Map _getCreateArguments({ + required String platformViewType, + required int platformViewId, + required int viewId, + Object? params, +}) { + return { + 'platformViewId': platformViewId, + 'platformViewType': platformViewType, + if (params != null) 'params': params, + 'viewId': viewId, + }; } -ByteData? _getDisposeMessage(int viewId) { - return codec.encodeMethodCall(MethodCall( - 'dispose', - viewId, - )); +Map _getDisposeArguments({ + required int platformViewId, + required int viewId, +}) { + return { + 'platformViewId': platformViewId, + 'viewId': viewId, + }; } From b27e1b38375b5907fdea08265d26e4eef05f56d9 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Fri, 20 Oct 2023 12:20:08 -0700 Subject: [PATCH 9/9] Add link support in web accessibility (#46117) fixes https://github.com/flutter/flutter/issues/134795 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- ci/licenses_golden/licenses_flutter | 2 + lib/web_ui/lib/src/engine.dart | 1 + lib/web_ui/lib/src/engine/semantics.dart | 1 + .../lib/src/engine/semantics/checkable.dart | 19 +-- .../lib/src/engine/semantics/dialog.dart | 13 +- .../lib/src/engine/semantics/focusable.dart | 6 +- .../lib/src/engine/semantics/image.dart | 8 +- .../src/engine/semantics/incrementable.dart | 2 +- .../src/engine/semantics/label_and_value.dart | 10 +- lib/web_ui/lib/src/engine/semantics/link.dart | 21 +++ .../lib/src/engine/semantics/live_region.dart | 4 +- .../src/engine/semantics/platform_view.dart | 5 +- .../lib/src/engine/semantics/scrollable.dart | 16 +- .../lib/src/engine/semantics/semantics.dart | 141 ++++++++++++------ .../lib/src/engine/semantics/tappable.dart | 40 ++--- .../lib/src/engine/semantics/text_field.dart | 21 ++- .../test/engine/semantics/semantics_test.dart | 71 ++++++++- 17 files changed, 260 insertions(+), 121 deletions(-) create mode 100644 lib/web_ui/lib/src/engine/semantics/link.dart diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index e6e3308249404..c01aba892418a 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -2720,6 +2720,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/focusable.dart + .. ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/image.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/incrementable.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/label_and_value.dart + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/link.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/live_region.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/platform_view.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/semantics/scrollable.dart + ../../../flutter/LICENSE @@ -5496,6 +5497,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/focusable.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/image.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/incrementable.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/label_and_value.dart +FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/link.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/live_region.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/platform_view.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/semantics/scrollable.dart diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index 1fda83ecc08fa..da7c3cdfddbe1 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -143,6 +143,7 @@ export 'engine/semantics/focusable.dart'; export 'engine/semantics/image.dart'; export 'engine/semantics/incrementable.dart'; export 'engine/semantics/label_and_value.dart'; +export 'engine/semantics/link.dart'; export 'engine/semantics/live_region.dart'; export 'engine/semantics/platform_view.dart'; export 'engine/semantics/scrollable.dart'; diff --git a/lib/web_ui/lib/src/engine/semantics.dart b/lib/web_ui/lib/src/engine/semantics.dart index d1d58ce4a688e..0ba3230ee96a1 100644 --- a/lib/web_ui/lib/src/engine/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics.dart @@ -8,6 +8,7 @@ export 'semantics/focusable.dart'; export 'semantics/image.dart'; export 'semantics/incrementable.dart'; export 'semantics/label_and_value.dart'; +export 'semantics/link.dart'; export 'semantics/live_region.dart'; export 'semantics/platform_view.dart'; export 'semantics/scrollable.dart'; diff --git a/lib/web_ui/lib/src/engine/semantics/checkable.dart b/lib/web_ui/lib/src/engine/semantics/checkable.dart index e6f5b2efcc223..d50f51c512c64 100644 --- a/lib/web_ui/lib/src/engine/semantics/checkable.dart +++ b/lib/web_ui/lib/src/engine/semantics/checkable.dart @@ -13,7 +13,6 @@ import 'package:ui/ui.dart' as ui; -import '../dom.dart'; import 'semantics.dart'; /// The specific type of checkable control. @@ -63,18 +62,18 @@ class Checkable extends PrimaryRoleManager { if (semanticsObject.isFlagsDirty) { switch (_kind) { case _CheckableKind.checkbox: - semanticsObject.setAriaRole('checkbox'); + setAriaRole('checkbox'); case _CheckableKind.radio: - semanticsObject.setAriaRole('radio'); + setAriaRole('radio'); case _CheckableKind.toggle: - semanticsObject.setAriaRole('switch'); + setAriaRole('switch'); } /// Adding disabled and aria-disabled attribute to notify the assistive /// technologies of disabled elements. _updateDisabledAttribute(); - semanticsObject.element.setAttribute( + setAttribute( 'aria-checked', (semanticsObject.hasFlag(ui.SemanticsFlag.isChecked) || semanticsObject.hasFlag(ui.SemanticsFlag.isToggled)) @@ -92,17 +91,15 @@ class Checkable extends PrimaryRoleManager { void _updateDisabledAttribute() { if (semanticsObject.enabledState() == EnabledState.disabled) { - final DomElement element = semanticsObject.element; - element - ..setAttribute('aria-disabled', 'true') - ..setAttribute('disabled', 'true'); + setAttribute('aria-disabled', 'true'); + setAttribute('disabled', 'true'); } else { _removeDisabledAttribute(); } } void _removeDisabledAttribute() { - final DomElement element = semanticsObject.element; - element..removeAttribute('aria-disabled')..removeAttribute('disabled'); + removeAttribute('aria-disabled'); + removeAttribute('disabled'); } } diff --git a/lib/web_ui/lib/src/engine/semantics/dialog.dart b/lib/web_ui/lib/src/engine/semantics/dialog.dart index 8155ba4838415..9f64e42e7acff 100644 --- a/lib/web_ui/lib/src/engine/semantics/dialog.dart +++ b/lib/web_ui/lib/src/engine/semantics/dialog.dart @@ -38,8 +38,8 @@ class Dialog extends PrimaryRoleManager { } return true; }()); - semanticsObject.element.setAttribute('aria-label', label ?? ''); - semanticsObject.setAriaRole('dialog'); + setAttribute('aria-label', label ?? ''); + setAriaRole('dialog'); } } @@ -51,8 +51,8 @@ class Dialog extends PrimaryRoleManager { return; } - semanticsObject.setAriaRole('dialog'); - semanticsObject.element.setAttribute( + setAriaRole('dialog'); + setAttribute( 'aria-describedby', routeName.semanticsObject.element.id, ); @@ -61,7 +61,10 @@ class Dialog extends PrimaryRoleManager { /// Supplies a description for the nearest ancestor [Dialog]. class RouteName extends RoleManager { - RouteName(SemanticsObject semanticsObject) : super(Role.routeName, semanticsObject); + RouteName( + SemanticsObject semanticsObject, + PrimaryRoleManager owner, + ) : super(Role.routeName, semanticsObject, owner); Dialog? _dialog; diff --git a/lib/web_ui/lib/src/engine/semantics/focusable.dart b/lib/web_ui/lib/src/engine/semantics/focusable.dart index 4b3285dbdf399..4caf56f3f3eac 100644 --- a/lib/web_ui/lib/src/engine/semantics/focusable.dart +++ b/lib/web_ui/lib/src/engine/semantics/focusable.dart @@ -28,9 +28,9 @@ import 'semantics.dart'; /// /// * https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets class Focusable extends RoleManager { - Focusable(SemanticsObject semanticsObject) + Focusable(SemanticsObject semanticsObject, PrimaryRoleManager owner) : _focusManager = AccessibilityFocusManager(semanticsObject.owner), - super(Role.focusable, semanticsObject); + super(Role.focusable, semanticsObject, owner); final AccessibilityFocusManager _focusManager; @@ -38,7 +38,7 @@ class Focusable extends RoleManager { void update() { if (semanticsObject.isFocusable) { if (!_focusManager.isManaging) { - _focusManager.manage(semanticsObject.id, semanticsObject.element); + _focusManager.manage(semanticsObject.id, owner.element); } _focusManager.changeFocus(semanticsObject.hasFocus && (!semanticsObject.hasEnabledState || semanticsObject.isEnabled)); } else { diff --git a/lib/web_ui/lib/src/engine/semantics/image.dart b/lib/web_ui/lib/src/engine/semantics/image.dart index 0f0eb298b0ad3..efe1d7cdb414b 100644 --- a/lib/web_ui/lib/src/engine/semantics/image.dart +++ b/lib/web_ui/lib/src/engine/semantics/image.dart @@ -49,14 +49,14 @@ class ImageRoleManager extends PrimaryRoleManager { ..height = '${semanticsObject.rect!.height}px'; } _auxiliaryImageElement!.style.fontSize = '6px'; - semanticsObject.element.append(_auxiliaryImageElement!); + append(_auxiliaryImageElement!); } _auxiliaryImageElement!.setAttribute('role', 'img'); _setLabel(_auxiliaryImageElement); } else if (semanticsObject.isVisualOnly) { - semanticsObject.setAriaRole('img'); - _setLabel(semanticsObject.element); + setAriaRole('img'); + _setLabel(element); _cleanUpAuxiliaryElement(); } else { _cleanUpAuxiliaryElement(); @@ -78,7 +78,7 @@ class ImageRoleManager extends PrimaryRoleManager { } void _cleanupElement() { - semanticsObject.element.removeAttribute('aria-label'); + removeAttribute('aria-label'); } @override diff --git a/lib/web_ui/lib/src/engine/semantics/incrementable.dart b/lib/web_ui/lib/src/engine/semantics/incrementable.dart index f1de98d026982..7ad693b628584 100644 --- a/lib/web_ui/lib/src/engine/semantics/incrementable.dart +++ b/lib/web_ui/lib/src/engine/semantics/incrementable.dart @@ -29,7 +29,7 @@ class Incrementable extends PrimaryRoleManager { addRouteName(); addLabelAndValue(); - semanticsObject.element.append(_element); + append(_element); _element.type = 'range'; _element.setAttribute('role', 'slider'); diff --git a/lib/web_ui/lib/src/engine/semantics/label_and_value.dart b/lib/web_ui/lib/src/engine/semantics/label_and_value.dart index 23421e13590d1..3a5f7300734fb 100644 --- a/lib/web_ui/lib/src/engine/semantics/label_and_value.dart +++ b/lib/web_ui/lib/src/engine/semantics/label_and_value.dart @@ -2,7 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import '../dom.dart'; import 'semantics.dart'; /// Renders [SemanticsObject.label] and/or [SemanticsObject.value] to the semantics DOM. @@ -26,8 +25,8 @@ import 'semantics.dart'; /// This role manager does not manage images and text fields. See /// [ImageRoleManager] and [TextField]. class LabelAndValue extends RoleManager { - LabelAndValue(SemanticsObject semanticsObject) - : super(Role.labelAndValue, semanticsObject); + LabelAndValue(SemanticsObject semanticsObject, PrimaryRoleManager owner) + : super(Role.labelAndValue, semanticsObject, owner); @override void update() { @@ -62,12 +61,11 @@ class LabelAndValue extends RoleManager { combinedValue.write(semanticsObject.value); } - semanticsObject.element - .setAttribute('aria-label', combinedValue.toString()); + owner.setAttribute('aria-label', combinedValue.toString()); } void _cleanUpDom() { - semanticsObject.element.removeAttribute('aria-label'); + owner.removeAttribute('aria-label'); } @override diff --git a/lib/web_ui/lib/src/engine/semantics/link.dart b/lib/web_ui/lib/src/engine/semantics/link.dart new file mode 100644 index 0000000000000..00dcdfcad54c5 --- /dev/null +++ b/lib/web_ui/lib/src/engine/semantics/link.dart @@ -0,0 +1,21 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import '../dom.dart'; +import '../semantics.dart'; + +/// Provides accessibility for links. +class Link extends PrimaryRoleManager { + Link(SemanticsObject semanticsObject) : super.withBasics(PrimaryRole.link, semanticsObject); + + @override + DomElement createElement() { + final DomElement element = domDocument.createElement('a'); + // TODO(chunhtai): Fill in the real link once the framework sends entire uri. + // https://github.com/flutter/flutter/issues/102535. + element.setAttribute('href', '#'); + element.style.display = 'block'; + return element; + } +} diff --git a/lib/web_ui/lib/src/engine/semantics/live_region.dart b/lib/web_ui/lib/src/engine/semantics/live_region.dart index c922cddd717b7..cea9c997e1d63 100644 --- a/lib/web_ui/lib/src/engine/semantics/live_region.dart +++ b/lib/web_ui/lib/src/engine/semantics/live_region.dart @@ -15,8 +15,8 @@ import 'semantics.dart'; /// label of the element. See [LabelAndValue]. If there is no label provided /// no content will be read. class LiveRegion extends RoleManager { - LiveRegion(SemanticsObject semanticsObject) - : super(Role.liveRegion, semanticsObject); + LiveRegion(SemanticsObject semanticsObject, PrimaryRoleManager owner) + : super(Role.liveRegion, semanticsObject, owner); String? _lastAnnouncement; diff --git a/lib/web_ui/lib/src/engine/semantics/platform_view.dart b/lib/web_ui/lib/src/engine/semantics/platform_view.dart index 80321fc77e03e..7502694390dc3 100644 --- a/lib/web_ui/lib/src/engine/semantics/platform_view.dart +++ b/lib/web_ui/lib/src/engine/semantics/platform_view.dart @@ -2,7 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import '../dom.dart'; import '../platform_views/slots.dart'; import 'semantics.dart'; @@ -30,13 +29,13 @@ class PlatformViewRoleManager extends PrimaryRoleManager { if (semanticsObject.isPlatformView) { if (semanticsObject.isPlatformViewIdDirty) { - semanticsObject.element.setAttribute( + setAttribute( 'aria-owns', getPlatformViewDomId(semanticsObject.platformViewId), ); } } else { - semanticsObject.element.removeAttribute('aria-owns'); + removeAttribute('aria-owns'); } } } diff --git a/lib/web_ui/lib/src/engine/semantics/scrollable.dart b/lib/web_ui/lib/src/engine/semantics/scrollable.dart index 82b9308816ab3..61f912304e1c0 100644 --- a/lib/web_ui/lib/src/engine/semantics/scrollable.dart +++ b/lib/web_ui/lib/src/engine/semantics/scrollable.dart @@ -30,7 +30,7 @@ class Scrollable extends PrimaryRoleManager { ..transformOrigin = '0 0 0' // Ignore pointer events since this is a dummy element. ..pointerEvents = 'none'; - semanticsObject.element.append(_scrollOverflowElement); + append(_scrollOverflowElement); } /// Disables browser-driven scrolling in the presence of pointer events. @@ -112,7 +112,7 @@ class Scrollable extends PrimaryRoleManager { // This is effective only in Chrome. Safari does not implement this // CSS property. In Safari the `PointerBinding` uses `preventDefault` // to prevent browser scrolling. - semanticsObject.element.style.touchAction = 'none'; + element.style.touchAction = 'none'; _gestureModeDidChange(); // Memoize the tear-off because Dart does not guarantee that two @@ -126,17 +126,17 @@ class Scrollable extends PrimaryRoleManager { _scrollListener = createDomEventListener((_) { _recomputeScrollPosition(); }); - semanticsObject.element.addEventListener('scroll', _scrollListener); + addEventListener('scroll', _scrollListener); } } /// The value of "scrollTop" or "scrollLeft", depending on the scroll axis. int get _domScrollPosition { if (semanticsObject.isVerticalScrollContainer) { - return semanticsObject.element.scrollTop.toInt(); + return element.scrollTop.toInt(); } else { assert(semanticsObject.isHorizontalScrollContainer); - return semanticsObject.element.scrollLeft.toInt(); + return element.scrollLeft.toInt(); } } @@ -153,7 +153,6 @@ class Scrollable extends PrimaryRoleManager { void _neutralizeDomScrollPosition() { // This value is arbitrary. const int canonicalNeutralScrollPosition = 10; - final DomElement element = semanticsObject.element; final ui.Rect? rect = semanticsObject.rect; if (rect == null) { printWarning('Warning! the rect attribute of semanticsObject is null'); @@ -197,7 +196,6 @@ class Scrollable extends PrimaryRoleManager { } void _gestureModeDidChange() { - final DomElement element = semanticsObject.element; switch (semanticsObject.owner.gestureMode) { case GestureMode.browserGestures: // overflow:scroll will cause the browser report "scroll" events when @@ -227,13 +225,13 @@ class Scrollable extends PrimaryRoleManager { @override void dispose() { super.dispose(); - final DomCSSStyleDeclaration style = semanticsObject.element.style; + final DomCSSStyleDeclaration style = element.style; assert(_gestureModeListener != null); style.removeProperty('overflowY'); style.removeProperty('overflowX'); style.removeProperty('touch-action'); if (_scrollListener != null) { - semanticsObject.element.removeEventListener('scroll', _scrollListener); + removeEventListener('scroll', _scrollListener); } semanticsObject.owner.removeGestureModeListener(_gestureModeListener); _gestureModeListener = null; diff --git a/lib/web_ui/lib/src/engine/semantics/semantics.dart b/lib/web_ui/lib/src/engine/semantics/semantics.dart index 316c867a5fef0..8a6ec7c0d35eb 100644 --- a/lib/web_ui/lib/src/engine/semantics/semantics.dart +++ b/lib/web_ui/lib/src/engine/semantics/semantics.dart @@ -24,6 +24,7 @@ import 'focusable.dart'; import 'image.dart'; import 'incrementable.dart'; import 'label_and_value.dart'; +import 'link.dart'; import 'live_region.dart'; import 'platform_view.dart'; import 'scrollable.dart'; @@ -382,6 +383,9 @@ enum PrimaryRole { /// /// Provides a label or a value. generic, + + /// Contains a link. + link, } /// Identifies one of the secondary [RoleManager]s of a [PrimaryRoleManager]. @@ -437,6 +441,8 @@ abstract class PrimaryRoleManager { /// management intereferes with the widget's functionality. PrimaryRoleManager.blank(this.role, this.semanticsObject); + late final DomElement element = _initElement(createElement(), semanticsObject); + /// The primary role identifier. final PrimaryRole role; @@ -453,29 +459,82 @@ abstract class PrimaryRoleManager { @visibleForTesting List get debugSecondaryRoles => _secondaryRoleManagers?.map((RoleManager manager) => manager.role).toList() ?? const []; + @protected + DomElement createElement() => domDocument.createElement('flt-semantics'); + + static DomElement _initElement(DomElement element, SemanticsObject semanticsObject) { + // DOM nodes created for semantics objects are positioned absolutely using + // transforms. + element.style.position = 'absolute'; + element.setAttribute('id', 'flt-semantic-node-${semanticsObject.id}'); + + // The root node has some properties that other nodes do not. + if (semanticsObject.id == 0 && !configuration.debugShowSemanticsNodes) { + // Make all semantics transparent. Use `filter` instead of `opacity` + // attribute because `filter` is stronger. `opacity` does not apply to + // some elements, particularly on iOS, such as the slider thumb and track. + // + // Use transparency instead of "visibility:hidden" or "display:none" + // so that a screen reader does not ignore these elements. + element.style.filter = 'opacity(0%)'; + + // Make text explicitly transparent to signal to the browser that no + // rasterization needs to be done. + element.style.color = 'rgba(0,0,0,0)'; + } + + // Make semantic elements visible for debugging by outlining them using a + // green border. Do not use `border` attribute because it affects layout + // (`outline` does not). + if (configuration.debugShowSemanticsNodes) { + element.style.outline = '1px solid green'; + } + return element; + } + + /// Sets the `role` ARIA attribute. + void setAriaRole(String ariaRoleName) { + setAttribute('role', ariaRoleName); + } + + /// Sets the `role` ARIA attribute. + void setAttribute(String name, Object value) { + element.setAttribute(name, value); + } + + void append(DomElement child) { + element.append(child); + } + + void removeAttribute(String name) => element.removeAttribute(name); + + void addEventListener(String type, DomEventListener? listener, [bool? useCapture]) => element.addEventListener(type, listener, useCapture); + + void removeEventListener(String type, DomEventListener? listener, [bool? useCapture]) => element.removeEventListener(type, listener, useCapture); + /// Adds generic focus management features. void addFocusManagement() { - addSecondaryRole(Focusable(semanticsObject)); + addSecondaryRole(Focusable(semanticsObject, this)); } /// Adds generic live region features. void addLiveRegion() { - addSecondaryRole(LiveRegion(semanticsObject)); + addSecondaryRole(LiveRegion(semanticsObject, this)); } /// Adds generic route name features. void addRouteName() { - addSecondaryRole(RouteName(semanticsObject)); + addSecondaryRole(RouteName(semanticsObject, this)); } /// Adds generic label features. void addLabelAndValue() { - addSecondaryRole(LabelAndValue(semanticsObject)); + addSecondaryRole(LabelAndValue(semanticsObject, this)); } /// Adds generic functionality for handling taps and clicks. void addTappable() { - addSecondaryRole(Tappable(semanticsObject)); + addSecondaryRole(Tappable(semanticsObject, this)); } /// Adds a secondary role to this primary role manager. @@ -525,7 +584,7 @@ abstract class PrimaryRoleManager { /// gesture mode changes. @mustCallSuper void dispose() { - semanticsObject.element.removeAttribute('role'); + removeAttribute('role'); _isDisposed = true; } } @@ -566,11 +625,11 @@ final class GenericRole extends PrimaryRoleManager { // Flutter renders into canvas, so the focus ring looks wrong. // - Read out the same label multiple times. if (semanticsObject.hasChildren) { - semanticsObject.setAriaRole('group'); + setAriaRole('group'); } else if (semanticsObject.hasFlag(ui.SemanticsFlag.isHeader)) { - semanticsObject.setAriaRole('heading'); + setAriaRole('heading'); } else { - semanticsObject.setAriaRole('text'); + setAriaRole('text'); } } } @@ -588,7 +647,7 @@ abstract class RoleManager { /// Initializes a secondary role for [semanticsObject]. /// /// A single role object manages exactly one [SemanticsObject]. - RoleManager(this.role, this.semanticsObject); + RoleManager(this.role, this.semanticsObject, this.owner); /// Role identifier. final Role role; @@ -596,6 +655,8 @@ abstract class RoleManager { /// The semantics object managed by this role. final SemanticsObject semanticsObject; + final PrimaryRoleManager owner; + /// Called immediately after the [semanticsObject] updates some of its fields. /// /// A concrete implementation of this method would typically use some of the @@ -627,34 +688,7 @@ abstract class RoleManager { /// information to the browser. class SemanticsObject { /// Creates a semantics tree node with the given [id] and [owner]. - SemanticsObject(this.id, this.owner) { - // DOM nodes created for semantics objects are positioned absolutely using - // transforms. - element.style.position = 'absolute'; - element.setAttribute('id', 'flt-semantic-node-$id'); - - // The root node has some properties that other nodes do not. - if (id == 0 && !configuration.debugShowSemanticsNodes) { - // Make all semantics transparent. Use `filter` instead of `opacity` - // attribute because `filter` is stronger. `opacity` does not apply to - // some elements, particularly on iOS, such as the slider thumb and track. - // - // Use transparency instead of "visibility:hidden" or "display:none" - // so that a screen reader does not ignore these elements. - element.style.filter = 'opacity(0%)'; - - // Make text explicitly transparent to signal to the browser that no - // rasterization needs to be done. - element.style.color = 'rgba(0,0,0,0)'; - } - - // Make semantic elements visible for debugging by outlining them using a - // green border. Do not use `border` attribute because it affects layout - // (`outline` does not). - if (configuration.debugShowSemanticsNodes) { - element.style.outline = '1px solid green'; - } - } + SemanticsObject(this.id, this.owner); /// See [ui.SemanticsUpdateBuilder.updateNode]. int get flags => _flags; @@ -981,9 +1015,6 @@ class SemanticsObject { /// Controls the semantics tree that this node participates in. final EngineSemanticsOwner owner; - /// The DOM element used to convey semantics information to the browser. - final DomElement element = domDocument.createElement('flt-semantics'); - /// Bitfield showing which fields have been updated but have not yet been /// applied to the DOM. /// @@ -996,6 +1027,9 @@ class SemanticsObject { /// Whether the field corresponding to the [fieldIndex] has been updated. bool _isDirty(int fieldIndex) => (_dirtyFields & fieldIndex) != 0; + /// The dom element of this semantics object. + DomElement get element => primaryRole!.element; + /// Returns the HTML element that contains the HTML elements of direct /// children of this object. /// @@ -1079,6 +1113,9 @@ class SemanticsObject { /// Whether this object represents an editable text field. bool get isTextField => hasFlag(ui.SemanticsFlag.isTextField); + /// Whether this object represents an editable text field. + bool get isLink => hasFlag(ui.SemanticsFlag.isLink); + /// Whether this object needs screen readers attention right away. bool get isLiveRegion => hasFlag(ui.SemanticsFlag.isLiveRegion) && @@ -1456,11 +1493,6 @@ class SemanticsObject { _currentChildrenInRenderOrder = childrenInRenderOrder; } - /// Sets the `role` ARIA attribute. - void setAriaRole(String ariaRoleName) { - element.setAttribute('role', ariaRoleName); - } - /// The primary role of this node. /// /// The primary role is assigned by [updateSelf] based on the combination of @@ -1485,6 +1517,8 @@ class SemanticsObject { return PrimaryRole.scrollable; } else if (scopesRoute) { return PrimaryRole.dialog; + } else if (isLink) { + return PrimaryRole.link; } else { return PrimaryRole.generic; } @@ -1500,6 +1534,7 @@ class SemanticsObject { PrimaryRole.dialog => Dialog(this), PrimaryRole.image => ImageRoleManager(this), PrimaryRole.platformView => PlatformViewRoleManager(this), + PrimaryRole.link => Link(this), PrimaryRole.generic => GenericRole(this), }; } @@ -1509,6 +1544,7 @@ class SemanticsObject { void _updateRoles() { PrimaryRoleManager? currentPrimaryRole = primaryRole; final PrimaryRole roleId = _getPrimaryRoleIdentifier(); + final DomElement? previousElement = primaryRole?.element; if (currentPrimaryRole != null) { if (currentPrimaryRole.role == roleId) { @@ -1535,6 +1571,19 @@ class SemanticsObject { primaryRole = currentPrimaryRole; currentPrimaryRole.update(); } + + // Reparent element. + if (previousElement != element) { + final DomElement? container = _childContainerElement; + if (container != null) { + element.append(container); + } + final DomElement? parent = previousElement?.parent; + if (parent != null) { + parent.insertBefore(element, previousElement); + previousElement!.remove(); + } + } } /// Whether the object represents an UI element with "increase" or "decrease" diff --git a/lib/web_ui/lib/src/engine/semantics/tappable.dart b/lib/web_ui/lib/src/engine/semantics/tappable.dart index 259a9c7d55669..cba0fafeb7650 100644 --- a/lib/web_ui/lib/src/engine/semantics/tappable.dart +++ b/lib/web_ui/lib/src/engine/semantics/tappable.dart @@ -11,7 +11,7 @@ import 'semantics.dart'; /// Sets the "button" ARIA role. class Button extends PrimaryRoleManager { Button(SemanticsObject semanticsObject) : super.withBasics(PrimaryRole.button, semanticsObject) { - semanticsObject.setAriaRole('button'); + setAriaRole('button'); } @override @@ -19,9 +19,9 @@ class Button extends PrimaryRoleManager { super.update(); if (semanticsObject.enabledState() == EnabledState.disabled) { - semanticsObject.element.setAttribute('aria-disabled', 'true'); + setAttribute('aria-disabled', 'true'); } else { - semanticsObject.element.removeAttribute('aria-disabled'); + removeAttribute('aria-disabled'); } } } @@ -33,26 +33,28 @@ class Button extends PrimaryRoleManager { /// the browser may not send us pointer events. In that mode we forward HTML /// click as [ui.SemanticsAction.tap]. class Tappable extends RoleManager { - Tappable(SemanticsObject semanticsObject) - : super(Role.tappable, semanticsObject); + Tappable(SemanticsObject semanticsObject, PrimaryRoleManager owner) + : super(Role.tappable, semanticsObject, owner); DomEventListener? _clickListener; @override void update() { - if (!semanticsObject.isTappable || semanticsObject.enabledState() == EnabledState.disabled) { - _stopListening(); - } else { - if (_clickListener == null) { - _clickListener = createDomEventListener((DomEvent event) { - if (semanticsObject.owner.gestureMode != GestureMode.browserGestures) { - return; - } - EnginePlatformDispatcher.instance.invokeOnSemanticsAction( - semanticsObject.id, ui.SemanticsAction.tap, null); - }); - semanticsObject.element.addEventListener('click', _clickListener); - } + if (_clickListener == null) { + _clickListener = createDomEventListener((DomEvent event) { + // Stop dom from reacting since it will be handled entirely on the + // flutter side. + event.preventDefault(); + if (!semanticsObject.isTappable || semanticsObject.enabledState() == EnabledState.disabled) { + return; + } + if (semanticsObject.owner.gestureMode != GestureMode.browserGestures) { + return; + } + EnginePlatformDispatcher.instance.invokeOnSemanticsAction( + semanticsObject.id, ui.SemanticsAction.tap, null); + }); + owner.addEventListener('click', _clickListener); } } @@ -61,7 +63,7 @@ class Tappable extends RoleManager { return; } - semanticsObject.element.removeEventListener('click', _clickListener); + owner.removeEventListener('click', _clickListener); _clickListener = null; } diff --git a/lib/web_ui/lib/src/engine/semantics/text_field.dart b/lib/web_ui/lib/src/engine/semantics/text_field.dart index 9d6a88a9ccb46..243e09cab6f8f 100644 --- a/lib/web_ui/lib/src/engine/semantics/text_field.dart +++ b/lib/web_ui/lib/src/engine/semantics/text_field.dart @@ -275,7 +275,7 @@ class TextField extends PrimaryRoleManager { ..left = '0' ..width = '${semanticsObject.rect!.width}px' ..height = '${semanticsObject.rect!.height}px'; - semanticsObject.element.append(activeEditableElement); + append(activeEditableElement); } void _setupDomElement() { @@ -336,22 +336,21 @@ class TextField extends PrimaryRoleManager { return; } - semanticsObject.element - ..setAttribute('role', 'textbox') - ..setAttribute('contenteditable', 'false') - ..setAttribute('tabindex', '0'); + setAttribute('role', 'textbox'); + setAttribute('contenteditable', 'false'); + setAttribute('tabindex', '0'); num? lastPointerDownOffsetX; num? lastPointerDownOffsetY; - semanticsObject.element.addEventListener('pointerdown', + addEventListener('pointerdown', createDomEventListener((DomEvent event) { final DomPointerEvent pointerEvent = event as DomPointerEvent; lastPointerDownOffsetX = pointerEvent.clientX; lastPointerDownOffsetY = pointerEvent.clientY; }), true); - semanticsObject.element.addEventListener('pointerup', + addEventListener('pointerup', createDomEventListener((DomEvent event) { final DomPointerEvent pointerEvent = event as DomPointerEvent; @@ -399,17 +398,17 @@ class TextField extends PrimaryRoleManager { // represent the same text field. It will confuse VoiceOver, so `role` needs to // be assigned and removed, based on whether or not editableElement exists. activeEditableElement.focus(); - semanticsObject.element.removeAttribute('role'); + removeAttribute('role'); activeEditableElement.addEventListener('blur', createDomEventListener((DomEvent event) { - semanticsObject.element.setAttribute('role', 'textbox'); + setAttribute('role', 'textbox'); activeEditableElement.remove(); SemanticsTextEditingStrategy._instance?.deactivate(this); // Focus on semantics element before removing the editable element, so that // the user can continue navigating the page with the assistive technology. - semanticsObject.element.focus(); + element.focus(); editableElement = null; })); } @@ -447,7 +446,7 @@ class TextField extends PrimaryRoleManager { } } - final DomElement element = editableElement ?? semanticsObject.element; + final DomElement element = editableElement ?? this.element; if (semanticsObject.hasLabel) { element.setAttribute( 'aria-label', diff --git a/lib/web_ui/test/engine/semantics/semantics_test.dart b/lib/web_ui/test/engine/semantics/semantics_test.dart index 3d4876b0432ac..88cc01db8ce40 100644 --- a/lib/web_ui/test/engine/semantics/semantics_test.dart +++ b/lib/web_ui/test/engine/semantics/semantics_test.dart @@ -92,6 +92,9 @@ void runSemanticsTests() { group('focusable', () { _testFocusable(); }); + group('link', () { + _testLink(); + }); } void _testRoleManagerLifecycle() { @@ -337,7 +340,11 @@ void _testEngineSemanticsOwner() { expect(placeholder.isConnected, isFalse); }); - void renderSemantics({String? label, String? tooltip}) { + void renderSemantics({String? label, String? tooltip, Set flags = const {}}) { + int flagValues = 0; + for (final ui.SemanticsFlag flag in flags) { + flagValues = flagValues | flag.index; + } final ui.SemanticsUpdateBuilder builder = ui.SemanticsUpdateBuilder(); updateNode( builder, @@ -351,6 +358,7 @@ void _testEngineSemanticsOwner() { id: 1, label: label ?? '', tooltip: tooltip ?? '', + flags: flagValues, transform: Matrix4.identity().toFloat64(), rect: const ui.Rect.fromLTRB(0, 0, 20, 20), ); @@ -404,6 +412,45 @@ void _testEngineSemanticsOwner() { semantics().semanticsEnabled = false; }); + test('can switch role', () async { + semantics().semanticsEnabled = true; + + // Create + renderSemantics(label: 'Hello'); + + Map tree = semantics().debugSemanticsTree!; + expect(tree.length, 2); + expect(tree[1]!.element.tagName.toLowerCase(), 'flt-semantics'); + expect(tree[1]!.id, 1); + expect(tree[1]!.label, 'Hello'); + final DomElement existingParent = tree[1]!.element.parent!; + + expectSemanticsTree(''' + + + + +'''); + + // Update + renderSemantics(label: 'Hello', flags: { ui.SemanticsFlag.isLink }); + + tree = semantics().debugSemanticsTree!; + expect(tree.length, 2); + expect(tree[1]!.id, 1); + expect(tree[1]!.label, 'Hello'); + expect(tree[1]!.element.tagName.toLowerCase(), 'a'); + expectSemanticsTree(''' + + + + +'''); + expect(existingParent, tree[1]!.element.parent); + + semantics().semanticsEnabled = false; + }); + test('tooltip is part of label', () async { semantics().semanticsEnabled = true; @@ -2892,6 +2939,28 @@ void _testFocusable() { }); } +void _testLink() { + test('nodes with link: true creates anchor tag', () { + semantics() + ..debugOverrideTimestampFunction(() => _testTime) + ..semanticsEnabled = true; + + SemanticsObject pumpSemantics() { + final SemanticsTester tester = SemanticsTester(semantics()); + tester.updateNode( + id: 0, + isLink: true, + rect: const ui.Rect.fromLTRB(0, 0, 100, 50), + ); + tester.apply(); + return tester.getSemanticsObject(0); + } + + final SemanticsObject object = pumpSemantics(); + expect(object.element.tagName.toLowerCase(), 'a'); + }); +} + /// A facade in front of [ui.SemanticsUpdateBuilder.updateNode] that /// supplies default values for semantics attributes. void updateNode(