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

Fix: Blocks of multi-line inline elements are too large #18

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

igrep
Copy link
Contributor

@igrep igrep commented Jun 29, 2024

Problem

Blocks of multi-line inline elements can be broken even if they don't look collided with the ball in some cases.

Here's an example reproduced by the controlMode: "mouse" feature created in #15, and hard-coding visualizeBlocks: false (checkout my debug branch if you reproduce by yourself):

bba-too-large-block

As you can see, the larger block 12345678901234567890 got broken just as the 12345 got hit.

The source of the screenshot HTML is here.
<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="Content-Language" content="ja">
  </head>
  <body style="width: 8em; font-size: 50pt; font-family: monospace; line-break: anywhere">
    <span>12345678901234567890</span>
    <span>12345</span>
  </body>
</html>

Cause

This is because the blocks of elements are created from the rectangles returned by getBoundingClientRect, which returns only one rectangle from an element. getBoundingClientRect doesn't have a problem with most elements, each of which has only one bounding rectangle. But given a multi-line element, getBoundingClientRect returns an unexpectedly large rectangle including its rightmost bottom edge.

Solution

Use getClientRects instead of getBoundingClientRect to create blocks. It returns all rectangles of elements including multi-line elements.

NOTE

  • I recommend you to review with the ignore-spaces mode, and you'll see the changes in updateBall.ts are safe.
  • I tested this change only in the example case and my website's top page. Tell me if you think of any side-effect.
  • 👍Thanks for reading to the bottom line! This is the last pull request so far. I don't mean I'll stop contributing after this, but I won't do anymore for the time being. 🙇お忙しい中ありがとうございます!お疲れ様です🍵

Problem
====

Blocks of multi-line inline elements can be broken even if they don't look
collided with the ball in some cases.

Cause
----

This is because the blocks of elements are created from the rectangles
returned by `getBoundingClientRect`, which returns only one rectangle from
an element. `getBoundingClientRect` doesn't have a problem with most
elements, each of which has only one bounding rectangle. But given a
multi-line element, `getBoundingClientRect` returns an unexpectedly large
rectangle including its rightmost bottom edge.

Solution
====

Use `getClientRects` to create blocks instead of `getBoundingClientRect`.
It returns all rectangles of elements including multi-line elements.
Copy link
Owner

@canalun canalun left a comment

Choose a reason for hiding this comment

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

Thank you so much for finding this bug! It's really tricky and I overlooked it.
Many thanks!!!

👍Thanks for reading to the bottom line! This is the last pull request so far. I don't mean I'll stop contributing after this, but I won't do anymore for the time being. 🙇お忙しい中ありがとうございます!お疲れ様です🍵

I am so grateful that words can't express my appreciation...! This app improves a lot thanks to you, igrep-san.

I look forward to enjoying happy-coding with you somewhere again!!
Of course, you're always welcome to come back to this repo whenever you want :)

src/game/object/blocks.ts Outdated Show resolved Hide resolved
@canalun canalun merged commit 7e5e59e into canalun:main Jul 30, 2024
@igrep igrep deleted the fix-too-big-blocks branch July 30, 2024 04:16
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