SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.7k stars 3.19k forks source link

LadyBird menuShadow drawing random box #23937

Open tmkendall opened 7 months ago

tmkendall commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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)

tmkendall commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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