Bug 249930

Summary: Don't add empty rects during addFocusRingRects in RenderInline
Product: WebKit Reporter: Ahmad Saleem <ahmad.saleem792>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
GitHub Patch none

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.