From cda1d886dfdf287fab23fd3906fdb8d826ffb715 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Mon, 18 Dec 2023 21:19:10 +0100 Subject: [PATCH] LibWeb: Fix not working Element::scroll_an_element_into_view() Fixes following mistakes: - "scrolling box" for a document is not `scrollable_overflow_rect()` but size of viewport (initial containing block, like spec says). - comparing edges of "scrolling box" with edges of target element does not make any sense because "scrolling box" edges are relative to page while result of `get_bounding_client_rect()` is relative to viewport. --- .../Text/expected/scroll-into-view-end.txt | 1 + .../Text/expected/scroll-into-view-start.txt | 1 + .../Text/input/scroll-into-view-end.html | 29 +++++++++ .../Text/input/scroll-into-view-start.html | 29 +++++++++ Userland/Libraries/LibWeb/DOM/Element.cpp | 60 +++++++++---------- 5 files changed, 87 insertions(+), 33 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/scroll-into-view-end.txt create mode 100644 Tests/LibWeb/Text/expected/scroll-into-view-start.txt create mode 100644 Tests/LibWeb/Text/input/scroll-into-view-end.html create mode 100644 Tests/LibWeb/Text/input/scroll-into-view-start.html diff --git a/Tests/LibWeb/Text/expected/scroll-into-view-end.txt b/Tests/LibWeb/Text/expected/scroll-into-view-end.txt new file mode 100644 index 0000000000..22d327db36 --- /dev/null +++ b/Tests/LibWeb/Text/expected/scroll-into-view-end.txt @@ -0,0 +1 @@ + The page has been scrolled to y: 600 \ No newline at end of file diff --git a/Tests/LibWeb/Text/expected/scroll-into-view-start.txt b/Tests/LibWeb/Text/expected/scroll-into-view-start.txt new file mode 100644 index 0000000000..3f2a33798e --- /dev/null +++ b/Tests/LibWeb/Text/expected/scroll-into-view-start.txt @@ -0,0 +1 @@ + The page has been scrolled to y: 1000 \ No newline at end of file diff --git a/Tests/LibWeb/Text/input/scroll-into-view-end.html b/Tests/LibWeb/Text/input/scroll-into-view-end.html new file mode 100644 index 0000000000..3eb4ff54c8 --- /dev/null +++ b/Tests/LibWeb/Text/input/scroll-into-view-end.html @@ -0,0 +1,29 @@ + + +
+
+
+ diff --git a/Tests/LibWeb/Text/input/scroll-into-view-start.html b/Tests/LibWeb/Text/input/scroll-into-view-start.html new file mode 100644 index 0000000000..7fd6908687 --- /dev/null +++ b/Tests/LibWeb/Text/input/scroll-into-view-start.html @@ -0,0 +1,29 @@ + + +
+
+
+ diff --git a/Userland/Libraries/LibWeb/DOM/Element.cpp b/Userland/Libraries/LibWeb/DOM/Element.cpp index c2a8a5c210..39046cf877 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.cpp +++ b/Userland/Libraries/LibWeb/DOM/Element.cpp @@ -1534,13 +1534,19 @@ static ErrorOr scroll_an_element_into_view(DOM::Element& target, Bindings: } for (auto& scrollable_node : scrollable_nodes) { + if (!scrollable_node.is_document()) { + // FIXME: Add support for scrolling boxes other than the viewport. + continue; + } + // 1. If the Document associated with target is not same origin with the Document // associated with the element or viewport associated with scrolling box, terminate these steps. if (target.document().origin() != scrollable_node.document().origin()) { break; } - CSSPixelRect scrolling_box = *scrollable_node.paintable_box()->scrollable_overflow_rect(); + // NOTE: For a viewport scrolling box is initial containing block + CSSPixelRect scrolling_box = scrollable_node.document().viewport_rect(); // 2. Let target bounding border box be the box represented by the return value of invoking Element’s // getBoundingClientRect(), if target is an Element, or Range’s getBoundingClientRect(), @@ -1583,21 +1589,17 @@ static ErrorOr scroll_an_element_into_view(DOM::Element& target, Bindings: CSSPixels scrolling_box_width = scrolling_box_edge_d - scrolling_box_edge_c; // 11. Let position be the scroll position scrolling box would have by following these steps: - auto position = [&]() -> CSSPixelRect { - auto result_edge_a = scrolling_box_edge_a; - auto result_edge_b = scrolling_box_edge_b; - auto result_edge_c = scrolling_box_edge_c; - auto result_edge_d = scrolling_box_edge_d; + auto position = [&]() -> CSSPixelPoint { + CSSPixels x = 0; + CSSPixels y = 0; // 1. If block is "start", then align element edge A with scrolling box edge A. if (block == Bindings::ScrollLogicalPosition::Start) { - result_edge_a = element_edge_a; - result_edge_d = scrolling_box_edge_a + scrolling_box_height; + y = element_edge_a; } // 2. Otherwise, if block is "end", then align element edge B with scrolling box edge B. else if (block == Bindings::ScrollLogicalPosition::End) { - result_edge_b = element_edge_b; - result_edge_a = scrolling_box_edge_b - scrolling_box_height; + y = element_edge_a + element_height - scrolling_box_height; } // 3. Otherwise, if block is "center", then align the center of target bounding border box with the center of scrolling box in scrolling box’s block flow direction. else if (block == Bindings::ScrollLogicalPosition::Center) { @@ -1606,54 +1608,43 @@ static ErrorOr scroll_an_element_into_view(DOM::Element& target, Bindings: // 4. Otherwise, block is "nearest": else { // If element edge A and element edge B are both outside scrolling box edge A and scrolling box edge B - if (element_edge_a >= scrolling_box_edge_a && element_edge_b <= scrolling_box_edge_b) { + if (element_edge_a <= 0 && element_edge_b >= scrolling_box_height) { // Do nothing. } // If element edge A is outside scrolling box edge A and element height is less than scrolling box height // If element edge B is outside scrolling box edge B and element height is greater than scrolling box height - if ((element_edge_a >= scrolling_box_edge_a && element_height < scrolling_box_height) - || (element_edge_b <= scrolling_box_edge_b && element_height > scrolling_box_height)) { + else if ((element_edge_a <= 0 && element_height < scrolling_box_height) || (element_edge_b >= scrolling_box_height && element_height > scrolling_box_height)) { // Align element edge A with scrolling box edge A. - result_edge_a = element_edge_a; - result_edge_b = scrolling_box_edge_a + scrolling_box_height; + y = element_edge_a; } // If element edge A is outside scrolling box edge A and element height is greater than scrolling box height // If element edge B is outside scrolling box edge B and element height is less than scrolling box height - if ((element_edge_a <= scrolling_box_edge_a && element_height > scrolling_box_height) - || (element_edge_b >= scrolling_box_edge_b && element_height < scrolling_box_height)) { + else if ((element_edge_b >= scrolling_box_height && element_height < scrolling_box_height) || (element_edge_a <= 0 && element_height > scrolling_box_height)) { // Align element edge B with scrolling box edge B. - result_edge_b = element_edge_b; - result_edge_a = scrolling_box_edge_b - scrolling_box_height; + y = element_edge_a + element_height - scrolling_box_height; } } if (inline_ == Bindings::ScrollLogicalPosition::Nearest) { // If element edge C and element edge D are both outside scrolling box edge C and scrolling box edge D - if (element_edge_c >= scrolling_box_edge_c && element_edge_d <= scrolling_box_edge_d) { + if (element_edge_c <= 0 && element_edge_d >= scrolling_box_width) { // Do nothing. } // If element edge C is outside scrolling box edge C and element width is less than scrolling box width // If element edge D is outside scrolling box edge D and element width is greater than scrolling box width - if ((element_edge_c >= scrolling_box_edge_c && element_width < scrolling_box_width) - || (element_edge_d <= scrolling_box_edge_d && element_width > scrolling_box_width)) { + else if ((element_edge_c <= 0 && element_width < scrolling_box_width) || (element_edge_d >= scrolling_box_width && element_width > scrolling_box_width)) { // Align element edge C with scrolling box edge C. - result_edge_c = element_edge_c; - result_edge_d = scrolling_box_edge_c + scrolling_box_width; + x = element_edge_c; } - // If element edge C is outside scrolling box edge C and element width is greater than scrolling box width // If element edge D is outside scrolling box edge D and element width is less than scrolling box width - if ((element_edge_c <= scrolling_box_edge_c && element_width > scrolling_box_width) - || (element_edge_d >= scrolling_box_edge_d && element_width < scrolling_box_width)) { + else if ((element_edge_d >= scrolling_box_width && element_width < scrolling_box_width) || (element_edge_c <= 0 && element_width > scrolling_box_width)) { // Align element edge D with scrolling box edge D. - result_edge_d = element_edge_d; - result_edge_c = scrolling_box_edge_d - scrolling_box_width; + x = element_edge_d + element_width - scrolling_box_width; } } - auto result_width = result_edge_d - result_edge_c; - auto result_height = result_edge_b - result_edge_a; - return { result_edge_c, result_edge_a, result_width, result_height }; + return CSSPixelPoint { x, y }; }(); // FIXME: 12. If position is the same as scrolling box’s current scroll position, and scrolling box does not @@ -1670,7 +1661,10 @@ static ErrorOr scroll_an_element_into_view(DOM::Element& target, Bindings: // AD-HOC: auto& page = document.navigable()->traversable_navigable()->page(); - page.client().page_did_request_scroll_to(position.location()); + // NOTE: Since calculated position is relative to the viewport, we need to add the viewport's position to it + // before passing to page_did_request_scroll_to() that expects a position relative to the page. + position.set_y(position.y() + scrolling_box.y()); + page.client().page_did_request_scroll_to(position); } // If scrolling box is associated with an element else {