Bug 249930 - Don't add empty rects during addFocusRingRects in RenderInline
Summary: Don't add empty rects during addFocusRingRects in RenderInline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-12-28 18:30 PST by Ahmad Saleem
Modified: 2023-05-14 06:48 PDT (History)
4 users (show)

See Also:


Attachments
GitHub Patch (779.72 KB, image/png)
2023-05-11 04:15 PDT, Ahmad Saleem
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ahmad Saleem 2022-12-28 18:30:05 PST
Hi Team,

While going through Blink's commit, I came across another failing test case in Safari 16.2 & STP160:

Test Case - https://jsfiddle.net/1gvyf5er/1/show

^ Chrome Canary 111 does not show extra focus outline to small rectangle and similar is case with Firefox Nightly 110.

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/2cf30f0fb85c90f6ae813264aa0d0dcc9a85ab58

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderInline.cpp#912

Just wanted to raise since I was not able to find current bug for this.

Thanks!
Comment 1 Radar WebKit Bug Importer 2023-01-04 18:31:17 PST
<rdar://problem/103897005>
Comment 2 Ahmad Saleem 2023-05-11 04:15:15 PDT
Created attachment 466314 [details]
GitHub Patch

I manage to do merge and compile this but I think Line 930 is problematic since it does not fix the failing test case, which I have attached here on local build.

Just wanted to share what works so if anyone can highlight mistake, I can fix it or if they can fix it, they can take it up.
Comment 3 zalan 2023-05-12 21:03:06 PDT
Seems like a good merge.
Comment 4 zalan 2023-05-12 21:13:12 PDT
(In reply to zalan from comment #3)
> Seems like a good merge.
oops wrong bugzilla. please ignore.
Comment 5 zalan 2023-05-12 21:14:06 PDT
(In reply to Ahmad Saleem from comment #2)
> Created attachment 466314 [details]
> GitHub Patch
> 
> I manage to do merge and compile this but I think Line 930 is problematic
> since it does not fix the failing test case, which I have attached here on
> local build.
> 
> Just wanted to share what works so if anyone can highlight mistake, I can
> fix it or if they can fix it, they can take it up.
WebKit has addRect() and not operator(). just replace it and it will start working.
Comment 6 Ahmad Saleem 2023-05-13 02:48:11 PDT
(In reply to zalan from comment #5)
> (In reply to Ahmad Saleem from comment #2)
> > Created attachment 466314 [details]
> > GitHub Patch
> > 
> > I manage to do merge and compile this but I think Line 930 is problematic
> > since it does not fix the failing test case, which I have attached here on
> > local build.
> > 
> > Just wanted to share what works so if anyone can highlight mistake, I can
> > fix it or if they can fix it, they can take it up.
> WebKit has addRect() and not operator(). just replace it and it will start
> working.

Worked!!! (Test case also fixed - test in Minibrowser). Going to draft PR first and once I will fix all bugs, I will do ask for review. :-)
Comment 7 Ahmad Saleem 2023-05-13 02:50:19 PDT
(In Draft) PR - https://github.com/WebKit/WebKit/pull/13853
Comment 8 EWS 2023-05-14 06:48:38 PDT
Committed 264058@main (9db465cd1226): <https://commits.webkit.org/264058@main>

Reviewed commits have been landed. Closing PR #13853 and removing active labels.