From 8c465c95aa7acde2355b0ce0762987724f5f6811 Mon Sep 17 00:00:00 2001 From: BenJilks Date: Thu, 11 Jul 2024 20:12:51 +0100 Subject: [PATCH] LibWeb: Pick the closest hit test result, instead of the first When performing a hit test of type TextCursor, it would check if the position is around each fragment and not just inside it. This resulted in always selecting the first fragment checked. This commit computes the distance of each hit test result, and picks the closest one. --- .../Libraries/LibWeb/Painting/Paintable.h | 2 + .../LibWeb/Painting/PaintableBox.cpp | 86 +++++++++++++------ 2 files changed, 63 insertions(+), 25 deletions(-) diff --git a/Userland/Libraries/LibWeb/Painting/Paintable.h b/Userland/Libraries/LibWeb/Painting/Paintable.h index 57cee6e51e7..b7bd15df8e8 100644 --- a/Userland/Libraries/LibWeb/Painting/Paintable.h +++ b/Userland/Libraries/LibWeb/Painting/Paintable.h @@ -26,6 +26,8 @@ enum class PaintPhase { struct HitTestResult { JS::Handle paintable; int index_in_node { 0 }; + Optional vertical_distance {}; + Optional horizontal_distance {}; enum InternalPosition { None, diff --git a/Userland/Libraries/LibWeb/Painting/PaintableBox.cpp b/Userland/Libraries/LibWeb/Painting/PaintableBox.cpp index 4debff4fc1d..fd648c66280 100644 --- a/Userland/Libraries/LibWeb/Painting/PaintableBox.cpp +++ b/Userland/Libraries/LibWeb/Painting/PaintableBox.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -24,6 +25,7 @@ #include #include #include +#include namespace Web::Painting { @@ -843,11 +845,17 @@ Optional PaintableBox::hit_test(CSSPixelPoint position, HitTestTy { Optional result; (void)PaintableBox::hit_test(position, type, [&](HitTestResult candidate) { - VERIFY(!result.has_value()); - if (!candidate.paintable->visible_for_hit_testing()) - return TraversalDecision::Continue; - result = move(candidate); - return TraversalDecision::Break; + if (candidate.paintable->visible_for_hit_testing()) { + if (!result.has_value() + || candidate.vertical_distance.value_or(CSSPixels::max_integer_value) < result->vertical_distance.value_or(CSSPixels::max_integer_value) + || candidate.horizontal_distance.value_or(CSSPixels::max_integer_value) < result->horizontal_distance.value_or(CSSPixels::max_integer_value)) { + result = move(candidate); + } + } + + if (result.has_value() && (type == HitTestType::Exact || (result->vertical_distance == 0 && result->horizontal_distance == 0))) + return TraversalDecision::Break; + return TraversalDecision::Continue; }); return result; } @@ -872,7 +880,6 @@ TraversalDecision PaintableWithLines::hit_test(CSSPixelPoint position, HitTestTy return TraversalDecision::Break; } - Optional last_good_candidate; for (auto const& fragment : fragments()) { if (fragment.paintable().stacking_context()) continue; @@ -880,33 +887,62 @@ TraversalDecision PaintableWithLines::hit_test(CSSPixelPoint position, HitTestTy if (fragment_absolute_rect.contains(position_adjusted_by_scroll_offset)) { if (fragment.paintable().hit_test(position, type, callback) == TraversalDecision::Break) return TraversalDecision::Break; - HitTestResult hit_test_result { const_cast(fragment.paintable()), fragment.text_index_at(position_adjusted_by_scroll_offset.x()) }; + HitTestResult hit_test_result { const_cast(fragment.paintable()), fragment.text_index_at(position_adjusted_by_scroll_offset.x()), 0, 0 }; if (callback(hit_test_result) == TraversalDecision::Break) return TraversalDecision::Break; - } + } else if (type == HitTestType::TextCursor) { + auto const* common_ancestor_parent = [&]() -> DOM::Node const* { + auto selection = document().get_selection(); + if (!selection) + return nullptr; + auto range = selection->range(); + if (!range) + return nullptr; + auto common_ancestor = range->common_ancestor_container(); + if (common_ancestor->parent()) + return common_ancestor->parent(); + return common_ancestor; + }(); - // If we reached this point, the position is not within the fragment. However, the fragment start or end might be the place to place the cursor. - // This determines whether the fragment is a good candidate for the position. The last such good fragment is chosen. - // The best candidate is either the end of the line above, the beginning of the line below, or the beginning or end of the current line. - // We arbitrarily choose to consider the end of the line above and ignore the beginning of the line below. - // If we knew the direction of selection, we could make a better choice. - if (fragment_absolute_rect.bottom() - 1 <= position_adjusted_by_scroll_offset.y()) { // fully below the fragment - last_good_candidate = HitTestResult { const_cast(fragment.paintable()), fragment.start() + fragment.length() }; - } else if (fragment_absolute_rect.top() <= position_adjusted_by_scroll_offset.y()) { // vertically within the fragment - if (position_adjusted_by_scroll_offset.x() < fragment_absolute_rect.left()) { // left of the fragment - if (!last_good_candidate.has_value()) { // first fragment of the line - last_good_candidate = HitTestResult { const_cast(fragment.paintable()), fragment.start() }; + auto const* fragment_dom_node = fragment.layout_node().dom_node(); + if (common_ancestor_parent && fragment_dom_node && common_ancestor_parent->is_ancestor_of(*fragment_dom_node)) { + // If we reached this point, the position is not within the fragment. However, the fragment start or end might be + // the place to place the cursor. To determine the best place, we first find the closest fragment horizontally to + // the cursor. If we could not find one, then find for the closest vertically above the cursor. + // If we knew the direction of selection, we would look above if selecting upward. + if (fragment_absolute_rect.bottom() - 1 <= position_adjusted_by_scroll_offset.y()) { // fully below the fragment + HitTestResult hit_test_result { + .paintable = const_cast(fragment.paintable()), + .index_in_node = fragment.start() + fragment.length(), + .vertical_distance = position_adjusted_by_scroll_offset.y() - fragment_absolute_rect.bottom(), + }; + if (callback(hit_test_result) == TraversalDecision::Break) + return TraversalDecision::Break; + } else if (fragment_absolute_rect.top() <= position_adjusted_by_scroll_offset.y()) { // vertically within the fragment + if (position_adjusted_by_scroll_offset.x() < fragment_absolute_rect.left()) { + HitTestResult hit_test_result { + .paintable = const_cast(fragment.paintable()), + .index_in_node = fragment.start(), + .vertical_distance = 0, + .horizontal_distance = fragment_absolute_rect.left() - position_adjusted_by_scroll_offset.x(), + }; + if (callback(hit_test_result) == TraversalDecision::Break) + return TraversalDecision::Break; + } else if (position_adjusted_by_scroll_offset.x() > fragment_absolute_rect.right()) { + HitTestResult hit_test_result { + .paintable = const_cast(fragment.paintable()), + .index_in_node = fragment.start() + fragment.length(), + .vertical_distance = 0, + .horizontal_distance = position_adjusted_by_scroll_offset.x() - fragment_absolute_rect.right(), + }; + if (callback(hit_test_result) == TraversalDecision::Break) + return TraversalDecision::Break; + } } - } else { // right of the fragment - last_good_candidate = HitTestResult { const_cast(fragment.paintable()), fragment.start() + fragment.length() }; } } } - if (type == HitTestType::TextCursor && last_good_candidate.has_value()) { - if (callback(last_good_candidate.value()) == TraversalDecision::Break) - return TraversalDecision::Break; - } if (!stacking_context() && is_visible() && absolute_border_box_rect().contains(position_adjusted_by_scroll_offset.x(), position_adjusted_by_scroll_offset.y())) { if (callback(HitTestResult { const_cast(*this) }) == TraversalDecision::Break) return TraversalDecision::Break;