Skip to content

Commit

Permalink
LibWeb: Improve reliability of text selection
Browse files Browse the repository at this point in the history
This commit makes 2 changes, which make text selection work as expected
in most cases. Firstly, when performing a hit text of type `TextCursor`,
we only consider positions that are vertically within the fragment and
to the right of that fragment to be hits. Secondly, we now only
consider hits on nodes of type `Text` when extending the current
selection on mousemove.

This introduces a slight regression in that the start of a fragment is
no longer selected if the user clicks to the left of it.
  • Loading branch information
tcl3 committed Jun 5, 2024
1 parent c6e9f0e commit d2e9e05
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 11 deletions.
2 changes: 1 addition & 1 deletion Userland/Libraries/LibWeb/Page/EventHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ bool EventHandler::handle_mousemove(CSSPixelPoint position, CSSPixelPoint screen
}
if (m_in_mouse_selection) {
auto hit = paint_root()->hit_test(position, Painting::HitTestType::TextCursor);
if (start_index.has_value() && hit.has_value() && hit->dom_node()) {
if (start_index.has_value() && hit.has_value() && is<Web::DOM::Text>(hit->dom_node())) {
m_navigable->set_cursor_position(DOM::Position::create(realm, *hit->dom_node(), *start_index));
if (auto selection = document.get_selection()) {
auto anchor_node = selection->anchor_node();
Expand Down
14 changes: 4 additions & 10 deletions Userland/Libraries/LibWeb/Painting/PaintableBox.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -839,17 +839,11 @@ TraversalDecision PaintableWithLines::hit_test(CSSPixelPoint position, HitTestTy

// 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.
// The best candidate is the end of the end of the current line, if the position is to the right of the fragment.
// 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<Paintable&>(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<Paintable&>(fragment.paintable()), fragment.start() };
}
} else { // right of the fragment
if (fragment_absolute_rect.bottom() > position_adjusted_by_scroll_offset.y() && fragment_absolute_rect.top() <= position_adjusted_by_scroll_offset.y()) { // vertically within the fragment.
// FIXME: Selecting to the left of a fragment should select the start of that fragment.
if (position_adjusted_by_scroll_offset.x() >= fragment_absolute_rect.right()) { // to the right of the fragment
last_good_candidate = HitTestResult { const_cast<Paintable&>(fragment.paintable()), fragment.start() + fragment.length() };
}
}
Expand Down

0 comments on commit d2e9e05

Please sign in to comment.