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

LadyBird menuShadow drawing random box #23937

Open
tmkendall opened this issue Apr 11, 2024 · 8 comments
Open

LadyBird menuShadow drawing random box #23937

tmkendall opened this issue Apr 11, 2024 · 8 comments
Labels
bug Something isn't working has-repro We have a way to reproduce this bug. reduction-of-web-content Issue has a simplified reduction based on real-world web content. student project web compatibility

Comments

@tmkendall
Copy link

Hello, I am a student at the university of Utah working on ladybird for my final project in CS 4560 Web Browser Internals. While going through our Universities computing website we noticed that the browser will randomly draw rectangles in both corners
Chrome:
image

LadyBird:
image

Our group minimized the bug to some simple html:
image

My group wants to resolve this bug and want to know if this feasible for people not very familiar with the code. We know that it is due to the menu shadows so we've been looking through the CSS of that. So any comments and help would be awesome! Thank you!

@SuntzuDragon
Copy link

SuntzuDragon commented Apr 11, 2024

To add to this, the relevant bit of styling that is being rendered incorrectly is:

.menu-shadows {
	box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6);
}

@awesomekling
Copy link
Member

All right, so your simplified reduction is basically this:

<style>    
div {                                                     
    box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6); 
}
</style><div>

This should be a pretty self-contained bug, I would think!

I would start looking at paint_box_shadow in ShadowPainting.cpp

(cc @MacDue and @kalenikaliaksandr who have worked on this code in the past)

@AtkinsSJ AtkinsSJ added bug Something isn't working web compatibility has-repro We have a way to reproduce this bug. reduction-of-web-content Issue has a simplified reduction based on real-world web content. labels Apr 12, 2024
@tmkendall
Copy link
Author

tmkendall commented Apr 18, 2024

Follow up to this question, we have narrowed it down more and are very confused. When we put a image or a paragraph tag inside the div with the box shadow then the boxes go away but when it's just the div or header tags we still get these boxes.
LadyBird:
image
Google Chrome:
image

Our code testing out different cases:

<style>
.testing {
    box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6);
}

div {
  margin-bottom: 100px;
}
</style>

<div style="box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6)"></div>
<div class="testing"><h2>HEADER TEST</h2></div>
<div style="box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6)"><p>This is a p tag</p><p>This is a p tag</p><p>This is a p tag</p></div>
<div style="box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6)"><img src="https://www.cs.utah.edu/wp-content/uploads/2023/01/Kahlert-School-of-Computing_combined-hrz-1.svg"></div>

Any help would be awesome and much appreciated, we're also wondering if this is going to be above our paygrade and if we should look for something else. Thank you!

@pavpanchekha
Copy link

I tested this too:

<div style="box-shadow: 0 4px 10px -10px rgba(0, 0, 0, 0.6)">Hello</div>

In this case there is the bug. So my guess is that the issue occurs only for block boxes (the <div>) that contain inline content (text or image).

@pavpanchekha
Copy link

From experimenting a bit, the bug only seems to occur when the blur radius, spread radius, and font-height match up in some strange combination. I think figuring out the dependence between those might help suggest where the bug occurs.

@SuntzuDragon
Copy link

SuntzuDragon commented Apr 18, 2024

It seems like there is unexpected behavior when one of the last two of the four length values (blur-radius and spread-radius) are negative. On Chrome it is just a blank page, but on Ladybird it looks like this:

image

Minimized problem shown above:

<style>
    div {
        margin-bottom: 300px;
    }
</style>

<div style="box-shadow: 0px 0px 10px -10px rgba(0, 0, 0, 0.6)"></div>
<div style="box-shadow: 0px 0px -10px 10px rgba(0, 0, 0, 0.6)"></div>

But when both values are positive or both are negative, it seems to match the expected behavior with both Chrome and Ladybird showing a blank page.

@SuntzuDragon
Copy link

It looks like this bug might be happening because the parameter BordersData being passed into paint_box_shadow has widths of all zeros. If we check for this and return early from painting, the glitchy boxes no longer show up. And normal box shadows seem to be preserved.

// void paint_box_shadow(PaintContext& context, ....
dbgln("BordersData:\tT: {}\tB: {}\tL: {}\tR: {}", borders_data.top.width, 
borders_data.bottom.width, borders_data.left.width, borders_data.right.width);
if (borders_data.top.width == 0 && borders_data.bottom.width == 0 && borders_data.left.width == 0 && borders_data.right.width == 0) {
    return;
}
// for (auto& box_shadow_data.....

image
image

@SuntzuDragon
Copy link

Adding these two max checks prevents the shadowbox corners from swapping strangely:

auto expansion = spread_distance - (blur_radius * 2);
DevicePixelRect inner_bounding_rect = {
    device_content_rect.x() + offset_x - expansion,
    device_content_rect.y() + offset_y - expansion,
    max(0, (device_content_rect.width() + 2 * expansion).value()),
    max(0, (device_content_rect.height() + 2 * expansion).value())
};

Enlarged version showing corners are no longer being swapped:

image

SuntzuDragon added a commit to SuntzuDragon/serenity that referenced this issue Apr 19, 2024
Add checks to prevent negative width and height in inner_bounding_rect
within get_outer_box_shadow_configuration.
This fixes issues with shadow corner swapping
due to incorrect bounding box dimensions.

Fixes: SerenityOS#23937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has-repro We have a way to reproduce this bug. reduction-of-web-content Issue has a simplified reduction based on real-world web content. student project web compatibility
Projects
None yet
Development

No branches or pull requests

5 participants