Bug 230431 - RenderListMarker::imageChanged RenderBox image handling
Summary: RenderListMarker::imageChanged RenderBox image handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-17 15:38 PDT by Gabriel Nava Marino
Modified: 2021-09-22 10:20 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.23 KB, patch)
2021-09-17 16:02 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2021-09-20 19:15 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2021-09-21 12:00 PDT, Gabriel Nava Marino
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2021-09-21 13:36 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2021-09-21 15:49 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2021-09-17 15:38:53 PDT
RenderListMarker is a RenderBox. RenderBox is a RenderElement which can have image updates from style changes (i.e. RenderStyle has maskBoxImage).

Thus the following comment is outdated and no longer applies:
    // A list marker can't have a background or border image, so no need to call the base class method.
We should call the base class imageChanged method, which includes additional logic for handling RenderBox image updates, and handle image changes appropriately in RenderListMarker.
Comment 1 Gabriel Nava Marino 2021-09-17 16:02:23 PDT
Created attachment 438528 [details]
Patch
Comment 2 Darin Adler 2021-09-17 17:10:56 PDT
Comment on attachment 438528 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438528&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

Why no tests? Seems like we can and should make a test for whatever problem this caused.

> Source/WebCore/rendering/RenderListMarker.cpp:1803
> +    if (m_image) {
> +        if (o == m_image->data()) {

Let’s not nest this so much:

    if (m_image && o == m_image->data()) {
        ...
Comment 3 Gabriel Nava Marino 2021-09-20 19:15:25 PDT
Created attachment 438770 [details]
Patch
Comment 4 Gabriel Nava Marino 2021-09-20 19:16:25 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 438528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438528&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests (OOPS!).
> 
> Why no tests? Seems like we can and should make a test for whatever problem
> this caused.

I have added a new test to help catch this issue.

> 
> > Source/WebCore/rendering/RenderListMarker.cpp:1803
> > +    if (m_image) {
> > +        if (o == m_image->data()) {
> 
> Let’s not nest this so much:
> 
>     if (m_image && o == m_image->data()) {
>         ...

I have updated the patch to remove the unnecessary level of nesting.

Thank you for your feedback Darin!
Comment 5 Gabriel Nava Marino 2021-09-21 12:00:49 PDT
Created attachment 438844 [details]
Patch
Comment 6 Gabriel Nava Marino 2021-09-21 13:36:35 PDT
Created attachment 438848 [details]
Patch
Comment 7 Gabriel Nava Marino 2021-09-21 15:49:21 PDT
Created attachment 438873 [details]
Patch
Comment 8 EWS 2021-09-22 10:19:42 PDT
Committed r282880 (242009@main): <https://commits.webkit.org/242009@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438873 [details].
Comment 9 Radar WebKit Bug Importer 2021-09-22 10:20:22 PDT
<rdar://problem/83406263>