Bug 238319 - Implement ServiceWorkerWindowClient.focus
Summary: Implement ServiceWorkerWindowClient.focus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 238364 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-03-24 03:08 PDT by youenn fablet
Modified: 2024-02-28 15:22 PST (History)
7 users (show)

See Also:


Attachments
WIP (31.10 KB, patch)
2022-03-24 03:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (62.81 KB, patch)
2022-03-25 03:59 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (62.84 KB, patch)
2022-03-25 05:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (65.58 KB, patch)
2022-03-26 06:11 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-03-24 03:08:34 PDT
<rdar://90616490>
Comment 1 youenn fablet 2022-03-24 03:12:59 PDT
Created attachment 455626 [details]
WIP
Comment 2 Ben Nham 2022-03-24 23:38:08 PDT
Comment on attachment 455626 [details]
WIP

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

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:247
> +

I did this a different way here: https://bugs.webkit.org/show_bug.cgi?id=238364

But maybe this much simpler implementation that you've done here is what we should be doing, as it doesn't require any changes to UserGestureIndicator.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:850
>      if (!m_uiDelegate)

Why not call UIClient::focus?
Comment 3 youenn fablet 2022-03-25 03:38:29 PDT
(In reply to Ben Nham from comment #2)
> Comment on attachment 455626 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455626&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:247
> > +
> 
> I did this a different way here:
> https://bugs.webkit.org/show_bug.cgi?id=238364
> 
> But maybe this much simpler implementation that you've done here is what we
> should be doing, as it doesn't require any changes to UserGestureIndicator.

Yeah, user gesture handling in non document is very loosely defined.
A service worker dedicated approach (based on events) seems ok for now.
We can revisit this later.

> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:850
> >      if (!m_uiDelegate)
> 
> Why not call UIClient::focus?

The WebPageProxy focus term has a strict meaning (used in setFocus and takeFocus)
which has nothing to do with making sure the view becomes key and visible.
This is a bit confusing. 

Hence why the different name that I reused from some automation code.
I am fine using another term though.
Comment 4 youenn fablet 2022-03-25 03:59:26 PDT
Created attachment 455744 [details]
Patch
Comment 5 youenn fablet 2022-03-25 05:01:44 PDT
Created attachment 455747 [details]
Patch
Comment 6 youenn fablet 2022-03-25 06:50:25 PDT
Comment on attachment 455747 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:840
> +    // FIXME: We probably need to call a delegate instead of doing this directly here.

This is the part where I am not exactly sure what we should do.
My guess is that we should let know the application we want to switch to that tab using a particular delegate (maybe with a default implementation if the delegate is not implemented).
This default implementation works for me on MacOS.
Comment 7 Brady Eidson 2022-03-25 12:24:25 PDT
Comment on attachment 455747 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:840
>> +    // FIXME: We probably need to call a delegate instead of doing this directly here.
> 
> This is the part where I am not exactly sure what we should do.
> My guess is that we should let know the application we want to switch to that tab using a particular delegate (maybe with a default implementation if the delegate is not implemented).
> This default implementation works for me on MacOS.

Let's add a new UI delegate method to call if implemented.
But if the client doesn't implement it, do the default thing.

R+ with that.
Comment 8 youenn fablet 2022-03-26 06:11:33 PDT
Created attachment 455842 [details]
Rebasing
Comment 9 EWS 2022-03-26 08:56:27 PDT
Committed r291938 (248909@main): <https://commits.webkit.org/248909@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455842 [details].
Comment 10 Ben Nham 2024-02-28 15:22:24 PST
*** Bug 238364 has been marked as a duplicate of this bug. ***