Skip to content

Commit

Permalink
[Mac a11y] Anchor the context menu to the center of the first oultine
Browse files Browse the repository at this point in the history
rect of multiline elements.

When a user opens a context menu via a keyboard shortcut,
EventHandler::ShowNonLocatedContextMenu is called, which finds the
center coordinate of the target element and opens a context menu there.
When the target element is a multiline link, the center of that
element's bounding box is often not on the link itself, which is a
strange visual experience.

This CL makes ShowNonLocatedContextMenu take the center coordinate of
the first outline rect of the element. The outline rect is elsewhere
used to show the focus highlight of multiline links. This ensures that
the context menu is anchored to the link text itself.

AX-Relnotes: Context menus opened on multiline links are visually
anchored to the link text.

Bug: 1121180
Change-Id: If6c3877e8abed9016a2d204e777142f27ca5e27f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412906
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Abigail Klein <abigailbklein@google.com>
Cr-Commit-Position: refs/heads/master@{#809050}
  • Loading branch information
Abigail Klein authored and Commit Bot committed Sep 21, 2020
1 parent 480a9b9 commit d41f8e4
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 7 deletions.
21 changes: 16 additions & 5 deletions content/browser/accessibility/accessibility_action_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest, ShowContextMenu) {
IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
ShowContextMenuOnMultilineElement) {
LoadInitialAccessibilityTreeFromHtml(R"HTML(
<div style='width: 10em'>This is some text.
<a href='www.google.com'>This is a multiline link.</a></div>
<a style="line-height: 16px" href='www.google.com'>
This is a <br><br><br><br>multiline link.</a>
)HTML");

BrowserAccessibility* target_node =
Expand All @@ -560,10 +560,15 @@ IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,

UntrustworthyContextMenuParams context_menu_params =
context_menu_filter->get_params();
EXPECT_EQ(base::ASCIIToUTF16("This is a multiline link."),
context_menu_params.link_text);
std::string link_text = base::UTF16ToUTF8(context_menu_params.link_text);
base::ReplaceChars(link_text, "\n", "\\n", &link_text);
EXPECT_EQ("This is a\\n\\n\\n\\nmultiline link.", link_text);
EXPECT_EQ(ui::MenuSourceType::MENU_SOURCE_KEYBOARD,
context_menu_params.source_type);
// Expect the context menu to open on the same line as the first line of link
// text. Check that the y coordinate of the context menu is near the line
// height.
EXPECT_NEAR(16, context_menu_params.y, 15);
}

IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
Expand Down Expand Up @@ -596,14 +601,17 @@ IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
EXPECT_EQ(base::ASCIIToUTF16("Offscreen"), context_menu_params.link_text);
EXPECT_EQ(ui::MenuSourceType::MENU_SOURCE_KEYBOARD,
context_menu_params.source_type);
// Expect the context menu point to be 0, 0.
EXPECT_EQ(0, context_menu_params.x);
EXPECT_EQ(0, context_menu_params.y);
}

IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
ShowContextMenuOnObscuredElement) {
LoadInitialAccessibilityTreeFromHtml(R"HTML(
<a href='www.google.com'>Obscured</a>
<div style="position: absolute; height: 100px; width: 100px; top: 0px;
left: 0px; background-color:red;"></div>
left: 0px; background-color:red; line-height: 16px"></div>
)HTML");

BrowserAccessibility* target_node =
Expand All @@ -628,6 +636,9 @@ IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
EXPECT_EQ(base::ASCIIToUTF16("Obscured"), context_menu_params.link_text);
EXPECT_EQ(ui::MenuSourceType::MENU_SOURCE_KEYBOARD,
context_menu_params.source_type);
// Expect the context menu to open on the same line as the link text. Check
// that the y coordinate of the context menu is near the line height.
EXPECT_NEAR(16, context_menu_params.y, 15);
}

IN_PROC_BROWSER_TEST_F(AccessibilityActionBrowserTest,
Expand Down
19 changes: 17 additions & 2 deletions third_party/blink/renderer/core/input/event_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2130,7 +2130,7 @@ WebInputEventResult EventHandler::ShowNonLocatedContextMenu(
// Intersect the selection rect and the visible bounds of focused_element.
if (focused_element) {
IntRect clipped_rect = view->ViewportToFrame(
focused_element->VisibleBoundsInVisualViewport());
GetFocusedElementRectForNonLocatedContextMenu(focused_element));
left = std::max(clipped_rect.X(), left);
top = std::max(clipped_rect.Y(), top);
right = std::min(clipped_rect.MaxX(), right);
Expand All @@ -2147,7 +2147,8 @@ WebInputEventResult EventHandler::ShowNonLocatedContextMenu(
view->ConvertToRootFrame(selection_rect.Center());
}
} else if (focused_element) {
IntRect clipped_rect = focused_element->VisibleBoundsInVisualViewport();
IntRect clipped_rect =
GetFocusedElementRectForNonLocatedContextMenu(focused_element);
location_in_root_frame =
visual_viewport.ViewportToRootFrame(clipped_rect.Center());
} else {
Expand Down Expand Up @@ -2195,6 +2196,20 @@ WebInputEventResult EventHandler::ShowNonLocatedContextMenu(
return SendContextMenuEvent(mouse_event, focused_element);
}

IntRect EventHandler::GetFocusedElementRectForNonLocatedContextMenu(
Element* focused_element) {
IntRect clipped_rect = focused_element->VisibleBoundsInVisualViewport();
// The bounding rect of multiline elements may include points that are
// not within the element. Intersect the clipped rect with the first
// outline rect to ensure that the selection rect only includes visible
// points within the focused element.
Vector<IntRect> outline_rects =
focused_element->OutlineRectsInVisualViewport();
if (outline_rects.size() > 1)
clipped_rect.Intersect(outline_rects[0]);
return clipped_rect;
}

void EventHandler::ScheduleHoverStateUpdate() {
// TODO(https://crbug.com/668758): Use a normal BeginFrame update for this.
if (!hover_timer_.IsActive() &&
Expand Down
3 changes: 3 additions & 0 deletions third_party/blink/renderer/core/input/event_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ class CORE_EXPORT EventHandler final : public GarbageCollected<EventHandler> {
const HitTestRequest& request,
const WebMouseEvent& mev);

IntRect GetFocusedElementRectForNonLocatedContextMenu(
Element* focused_element);

// NOTE: If adding a new field to this class please ensure that it is
// cleared in |EventHandler::clear()|.

Expand Down

0 comments on commit d41f8e4

Please sign in to comment.