[PEPC] Add new flag PermissionElementDialogPositioning
This new flags enables positioning the permission dialog over the
permission element, instead of in the center of the web view.
Bug: 1462930
Change-Id: I9f29f02c2668e5682e231d48ff04dd7f0c5794e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5125716
Reviewed-by: Thomas Nguyen <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Commit-Queue: Andy Paicu <[email protected]>
Reviewed-by: Dominik Röttsches <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1270104}
diff --git a/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc b/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc
index be2f366..2d9ac55 100644
--- a/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc
+++ b/chrome/browser/ui/views/permissions/embedded_permission_prompt.cc
@@ -159,7 +159,9 @@
PrioritizeAndMergeNewVariant(current_request_variant, type);
}
- raw_ptr<EmbeddedPermissionPromptBaseView> prompt_view = nullptr;
+ RebuildRequests();
+
+ EmbeddedPermissionPromptBaseView* prompt_view = nullptr;
switch (embedded_prompt_variant_) {
case Variant::kAsk:
@@ -207,7 +209,6 @@
}
if (prompt_view) {
- RebuildRequests();
prompt_view_tracker_.SetView(prompt_view);
content_scrim_widget_ =
EmbeddedPermissionPromptContentScrimView::CreateScrimWidget(
diff --git a/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.cc b/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.cc
index b131b8c..0f004f4a 100644
--- a/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.cc
+++ b/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.cc
@@ -13,11 +13,13 @@
#include "chrome/browser/ui/views/chrome_widget_sublevel.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "components/vector_icons/vector_icons.h"
+#include "content/public/common/content_features.h"
#include "ui/base/interaction/element_identifier.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/base/models/image_model.h"
#include "ui/base/ui_base_features.h"
+#include "ui/gfx/geometry/rect.h"
#include "ui/gfx/paint_vector_icon.h"
#include "ui/views/bubble/bubble_frame_view.h"
#include "ui/views/controls/button/md_text_button.h"
@@ -81,6 +83,10 @@
browser_(browser),
delegate_(delegate) {
SetProperty(views::kElementIdentifierKey, kMainViewId);
+
+ CHECK_GT(delegate_->Requests().size(), 0u);
+ element_rect_ = delegate_->Requests()[0]->GetAnchorElementPosition().value_or(
+ gfx::Rect());
}
EmbeddedPermissionPromptBaseView::~EmbeddedPermissionPromptBaseView() = default;
@@ -176,7 +182,6 @@
void EmbeddedPermissionPromptBaseView::ShowWidget() {
GetWidget()->Show();
-
SizeToContents();
}
@@ -184,7 +189,12 @@
SetAnchorView(widget->GetContentsView());
set_parent_window(
platform_util::GetViewForWindow(browser_->window()->GetNativeWindow()));
- SetArrow(views::BubbleBorder::Arrow::FLOAT);
+
+ if (ShouldOverrideBubbleBounds()) {
+ SetArrow(views::BubbleBorder::Arrow::BOTTOM_LEFT);
+ } else {
+ SetArrow(views::BubbleBorder::Arrow::FLOAT);
+ }
}
bool EmbeddedPermissionPromptBaseView::ShouldShowCloseButton() const {
@@ -286,5 +296,54 @@
buttons_container.AddChildView(std::move(button_view));
}
+gfx::Rect EmbeddedPermissionPromptBaseView::GetBubbleBounds() {
+ if (!ShouldOverrideBubbleBounds()) {
+ return views::BubbleDialogDelegateView::GetBubbleBounds();
+ }
+
+ gfx::Rect default_bounds = views::BubbleDialogDelegateView::GetBubbleBounds();
+
+ content::WebContents* web_contents =
+ delegate_->GetPermissionPromptDelegate()->GetAssociatedWebContents();
+
+ gfx::Rect container_bounds = web_contents->GetContainerBounds();
+
+ // First, attempt to position the prompt below the PEPC, if it would not
+ // overflow the container bounds.
+ gfx::Rect prompt_bounds(
+ default_bounds.x() + element_rect_.bottom_center().x() -
+ default_bounds.width() / 2,
+ default_bounds.y() + element_rect_.bottom_center().y() +
+ default_bounds.height(),
+ default_bounds.width(), default_bounds.height());
+
+ if (container_bounds.Contains(prompt_bounds)) {
+ return prompt_bounds;
+ }
+
+ // Second, attempt to position the prompt above the PEPC, if it would not
+ // overflow the container bounds.
+ prompt_bounds =
+ gfx::Rect(default_bounds.x() + element_rect_.top_center().x() -
+ default_bounds.width() / 2,
+ default_bounds.y() + element_rect_.top_center().y(),
+ default_bounds.width(), default_bounds.height());
+
+ if (container_bounds.Contains(prompt_bounds)) {
+ return prompt_bounds;
+ }
+
+ // Otherwise, place it in the middle of the container bounds.
+ return gfx::Rect(
+ container_bounds.CenterPoint().x() - default_bounds.width() / 2,
+ container_bounds.CenterPoint().y() - default_bounds.height() / 2,
+ default_bounds.width(), default_bounds.height());
+}
+
+bool EmbeddedPermissionPromptBaseView::ShouldOverrideBubbleBounds() const {
+ return features::kPermissionElementDialogPositioning.Get() &&
+ !element_rect_.IsEmpty();
+}
+
BEGIN_METADATA(EmbeddedPermissionPromptBaseView)
END_METADATA
diff --git a/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.h b/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.h
index 13a5af9..8e06667 100644
--- a/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.h
+++ b/chrome/browser/ui/views/permissions/embedded_permission_prompt_base_view.h
@@ -122,8 +122,11 @@
void AddButton(views::View& buttons_container,
const ButtonConfiguration& button);
std::unique_ptr<views::FlexLayoutView> CreateLoadingIcon();
+ gfx::Rect GetBubbleBounds() override;
+ bool ShouldOverrideBubbleBounds() const;
const raw_ptr<Browser> browser_;
+ gfx::Rect element_rect_;
base::WeakPtr<EmbeddedPermissionPromptViewDelegate> delegate_;
};
diff --git a/chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc b/chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
index 1b3a02b..c0dcfed 100644
--- a/chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
+++ b/chrome/browser/ui/views/permissions/embedded_permission_prompt_interactive_uitest.cc
@@ -251,9 +251,11 @@
}));
}
+ protected:
+ base::test::ScopedFeatureList feature_list_;
+
private:
std::unique_ptr<net::EmbeddedTestServer> https_server_;
- base::test::ScopedFeatureList feature_list_;
};
// Failing on Windows, though manual testing of the same flow does not reproduce
@@ -263,11 +265,15 @@
#define MAYBE_BasicFlowCamera DISABLED_BasicFlowCamera
#define MAYBE_BasicFlowCameraMicrophone DISABLED_BasicFlowCameraMicrophone
#define MAYBE_TestPartialPermissionsLabels DISABLED_TestPartialPermissionsLabels
+#define MAYBE_TestPermissionElementDialogPositioning \
+ DISABLED_TestPermissionElementDialogPositioning
#else
#define MAYBE_BasicFlowMicrophone BasicFlowMicrophone
#define MAYBE_BasicFlowCamera BasicFlowCamera
#define MAYBE_BasicFlowCameraMicrophone BasicFlowCameraMicrophone
#define MAYBE_TestPartialPermissionsLabels TestPartialPermissionsLabels
+#define MAYBE_TestPermissionElementDialogPositioning \
+ TestPermissionElementDialogPositioning
#endif
IN_PROC_BROWSER_TEST_F(EmbeddedPermissionPromptInteractiveTest,
MAYBE_BasicFlowMicrophone) {
@@ -334,3 +340,55 @@
u"You previously didn't allow microphone on a.test:" +
base::UTF8ToUTF16(GetOrigin().port()));
}
+
+class EmbeddedPermissionPromptPositioningInteractiveTest
+ : public EmbeddedPermissionPromptInteractiveTest {
+ public:
+ EmbeddedPermissionPromptPositioningInteractiveTest() {
+ feature_list_.Reset();
+ feature_list_.InitWithFeaturesAndParameters(
+ {
+ {features::kPermissionElement,
+ {{"PermissionElementDialogPositioning", "true"}}},
+ {permissions::features::kOneTimePermission, {}},
+ {blink::features::kDisablePepcSecurityForTesting, {}},
+ },
+ {});
+ }
+};
+
+IN_PROC_BROWSER_TEST_F(EmbeddedPermissionPromptPositioningInteractiveTest,
+ MAYBE_TestPermissionElementDialogPositioning) {
+ RunTestSequence(InstrumentTab(kWebContentsElementId),
+ NavigateWebContents(kWebContentsElementId, GetURL()));
+
+ // Click on multiple elements in order from left to right, and ensure that
+ // dialog moves with each click
+ int previous_x = 0;
+ struct ElementAction {
+ std::string element_name;
+ ui::ElementIdentifier button_identifier;
+ };
+ std::vector<ElementAction> element_actions = {
+ {"microphone", EmbeddedPermissionPromptAskView::kAllowId},
+ {"camera", EmbeddedPermissionPromptAskView::kAllowId},
+ {"camera-microphone",
+ EmbeddedPermissionPromptPreviouslyGrantedView::kStopAllowingId},
+ };
+
+ for (const auto& element_action : element_actions) {
+ RunTestSequence(ClickOnPEPCElement(element_action.element_name),
+ InAnyContext(WaitForShow(
+ EmbeddedPermissionPromptBaseView::kMainViewId)),
+ FlushEvents(),
+ InAnyContext(CheckView(
+ EmbeddedPermissionPromptBaseView::kMainViewId,
+ [&previous_x](views::View* view) {
+ gfx::Rect bounds = view->GetBoundsInScreen();
+ previous_x = bounds.x();
+ return bounds.x();
+ },
+ testing::Gt(previous_x))),
+ PushPEPCPromptButton(element_action.button_identifier));
+ }
+}
diff --git a/components/permissions/permission_request.cc b/components/permissions/permission_request.cc
index 58fa158..3900210b 100644
--- a/components/permissions/permission_request.cc
+++ b/components/permissions/permission_request.cc
@@ -180,6 +180,10 @@
return data_.embedded_permission_element_initiated;
}
+std::optional<gfx::Rect> PermissionRequest::GetAnchorElementPosition() const {
+ return data_.anchor_element_position;
+}
+
#if !BUILDFLAG(IS_ANDROID)
bool PermissionRequest::IsConfirmationChipSupported() {
diff --git a/components/permissions/permission_request.h b/components/permissions/permission_request.h
index 1b65018..641feea 100644
--- a/components/permissions/permission_request.h
+++ b/components/permissions/permission_request.h
@@ -151,6 +151,9 @@
// element.
bool IsEmbeddedPermissionElementInitiated() const;
+ // Returns the position of the element that caused the prompt to open.
+ std::optional<gfx::Rect> GetAnchorElementPosition() const;
+
// Returns true if the request has two origins and should use the two origin
// prompt. Returns false otherwise.
bool ShouldUseTwoOriginPrompt() const;
diff --git a/components/permissions/permission_request_data.cc b/components/permissions/permission_request_data.cc
index 434f586..f724c59 100644
--- a/components/permissions/permission_request_data.cc
+++ b/components/permissions/permission_request_data.cc
@@ -20,6 +20,7 @@
embedded_permission_element_initiated(
request_description.embedded_permission_element_initiated),
requesting_origin(canonical_requesting_origin),
+ anchor_element_position(request_description.anchor_element_position),
requested_audio_capture_device_ids(
request_description.requested_audio_capture_device_ids),
requested_video_capture_device_ids(
diff --git a/content/public/common/content_features.cc b/content/public/common/content_features.cc
index 9855650a..5f4c73d 100644
--- a/content/public/common/content_features.cc
+++ b/content/public/common/content_features.cc
@@ -675,6 +675,11 @@
"PermissionElement",
base::FEATURE_DISABLED_BY_DEFAULT);
+// Enables different positioning of the permission dialog, so that it's placed
+// near the permission element, if possible.
+constexpr base::FeatureParam<bool> kPermissionElementDialogPositioning{
+ &kPermissionElement, "PermissionElementDialogPositioning", false};
+
// Enables Persistent Origin Trials. It causes tokens for an origin to be stored
// and persisted for the next navigation. This way, an origin trial can affect
// things before receiving the response, for instance it can affect the next
diff --git a/content/public/common/content_features.h b/content/public/common/content_features.h
index ec544c1..aacbc65 100644
--- a/content/public/common/content_features.h
+++ b/content/public/common/content_features.h
@@ -166,6 +166,8 @@
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFeaturePolicyHeader);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPepperCrossOriginRedirectRestriction);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPermissionElement);
+CONTENT_EXPORT extern const base::FeatureParam<bool>
+ kPermissionElementDialogPositioning;
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPersistentOriginTrials);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrefetchNewLimits);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kPrefetchRedirects);
diff --git a/third_party/blink/renderer/core/html/html_permission_element.cc b/third_party/blink/renderer/core/html/html_permission_element.cc
index 550ab19..6b3bf1c 100644
--- a/third_party/blink/renderer/core/html/html_permission_element.cc
+++ b/third_party/blink/renderer/core/html/html_permission_element.cc
@@ -17,6 +17,7 @@
#include "third_party/blink/renderer/core/events/mouse_event.h"
#include "third_party/blink/renderer/core/execution_context/execution_context.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
+#include "third_party/blink/renderer/core/geometry/dom_rect.h"
#include "third_party/blink/renderer/core/html/html_div_element.h"
#include "third_party/blink/renderer/core/html/html_span_element.h"
#include "third_party/blink/renderer/core/html/shadow/shadow_element_names.h"
@@ -367,7 +368,7 @@
auto descriptor = EmbeddedPermissionRequestDescriptor::New();
// TODO(crbug.com/1462930): Send element position to browser and use the
// rect to calculate expected prompt position in screen coordinates.
- descriptor->element_position = gfx::Rect(0, 0, 0, 0);
+ descriptor->element_position = GetBoundingClientRect()->ToEnclosingRect();
descriptor->permissions = mojo::Clone(permission_descriptors_);
GetPermissionService()->RequestPageEmbeddedPermission(
std::move(descriptor),
