Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb: Implement Range's extension method #1417

Merged

Conversation

An-n-ya
Copy link
Contributor

@An-n-ya An-n-ya commented Sep 17, 2024

This patch implements Range::getClientRects and Range::getBoundingClientRect. Since the rects returned by invoking getClientRects can be accessed without adding them to the Selection, ViewportPaintable::recompute_selection_states has been updated to accept a Range as a parameter, rather than acquiring it through the Document's Selection.

With this change, the following tests now pass:

Note: The test range-bounding-client-rect-with-display-contents still fails due to an issue with Element::getClientRects, which will be addressed in a future commit.

Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing the branch locally and found that it breaks selection on pages where it used to work. you can reproduce the problem by trying to select a text using mouse on a page as simple as <p>Hello</p>.

Besides that issue the change seems good and thank you for making progress on it!

This patch implements `Range::getClientRects` and
`Range::getBoundingClientRect`. Since the rects returned by invoking
getClientRects can be accessed without adding them to the Selection,
`ViewportPaintable::recompute_selection_states` has been updated to
accept a Range as a parameter, rather than acquiring it through the
Document's Selection.

With this change, the following tests now pass:

- wpt[css/cssom-view/range-bounding-client-rect-with-nested-text.html]
- wpt[css/cssom-view/DOMRectList.html]

Note: The test
"css/cssom-view/range-bounding-client-rect-with-display-contents.html"
still fails due to an issue with Element::getClientRects, which will
be addressed in a future commit.
Comment on lines +306 to +311
// 3. Mark the nodes included in range selected.
for (auto* node = start_container; node && node != end_container->next_in_pre_order(); node = node->next_in_pre_order()) {
if (auto* paintable = node->paintable())
paintable->set_selected(true);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing the branch locally and found that it breaks selection on pages where it used to work. you can reproduce the problem by trying to select a text using mouse on a page as simple as <p>Hello</p>.

Besides that issue the change seems good and thank you for making progress on it!

The problem is caused by incorrectly iterating over the range's included nodes, which is addressed here.

@kalenikaliaksandr kalenikaliaksandr merged commit 75c7dbc into LadybirdBrowser:master Sep 20, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants