Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consuming/clearing items from the launchQueue #92

Open
hybridherbst opened this issue Apr 11, 2023 · 22 comments
Open

Consuming/clearing items from the launchQueue #92

hybridherbst opened this issue Apr 11, 2023 · 22 comments
Labels

Comments

@hybridherbst
Copy link

hybridherbst commented Apr 11, 2023

I'm running into an issue with using launchQueue and setConsumer - the launchQueue is not cleared on subsequent page refresh when items are consumed and there doesn't seem to be a way to persistently remove items from the queue.

Consider the following scenario:

  1. you have a "viewer app".
  2. user double-clicks a file supported by the app; app is launched with one item in launch queue.
  3. launch queue is processed, file is displayed.
  4. user uses in-app UI to select another file to view.
  5. user refreshes the page (F5)
  6. unexpectedly, the old file displays again, because the app receives another launchQueue with items (despite them having been processed in an earlier launch).
  7. as far as I can see there's no way to differentiate between "user has refreshed the page but doesn't want the launchQueue to be applied since it's already displaying another file" and "user has double-clicked the same item again and gets a new window instance with the same launch queue, which should be processed".

Suggestions appreciated - not sure if I'm missing something. I'd like to declare a launchQueue item as "used" and not have it show up again on page refresh.

EDIT: seems that the launchQueue also has no timestamp or similar for when the item was added to the queue (which would help to differentiate with files that have been dropped later etc.). So this is also not useful to differentiate if a launch queue item is actually expected to be processed or is "outdated" / "used in the past".

@fallaciousreasoning
Copy link
Collaborator

@hybridherbst this sounds like a browser specific bug - would you mind filing a bug against the specific browser (I imagine you're using Chromium?).

https://bugs.chromium.org/p/chromium/issues/entry

@hybridherbst
Copy link
Author

@fallaciousreasoning as I wasn't able to find anything related to the desired behaviour when refreshing a window that had a launchQueue, I'm not certain that this is a bug. What is the intended behaviour and where can I read more about that?

@fallaciousreasoning
Copy link
Collaborator

I think that items in the launch queue should only be processed once. I don't know if this is specified anywhere but it was definitely my original intention (it may have changed, but I'd be surprised).

The spec seems to imply that only unconsumed LaunchParams should be passed to the Launch Consumer:
https://wicg.github.io/web-app-launch/#setconsumer-method

@alancutter
Copy link
Collaborator

alancutter commented Apr 12, 2023

This behaviour ought to be clarified in the spec.

The behaviour you're seeing in Chrome comes from:

  // Reloads have the last sent launch params re-sent as they may contain live
  // file handles that should persist across reloads.
  if (last_sent_queued_launch_params_ &&
      handle->GetReloadType() != content::ReloadType::NONE) {
    SendLaunchParams(*last_sent_queued_launch_params_, handle->GetURL());
    return;
  }

This is intended for the case where you open a file in the app via its file_handlers and then refresh the page. Without this the app would lose the file handle.
@evanstade

Your issue raises a very good point that the app may have changed which file it is working on in the meantime and the old LaunchParams has gone stale.

@alancutter alancutter reopened this Apr 12, 2023
@alancutter alancutter added the bug label Apr 12, 2023
@evanstade
Copy link

evanstade commented Apr 12, 2023

as far as I can see there's no way to differentiate between "user has refreshed the page but doesn't want the launchQueue to be applied since it's already displaying another file" and "user has double-clicked the same item again and gets a new window instance with the same launch queue, which should be processed".

There is of course case 3, which is what this behavior is intended to address: "user didn't start displaying another file, has refreshed the page but expects the same file to still be open"

Perhaps the app's action URL should include something to indicate the launch action such as "?src=fileHandler" and this should be stripped when the user takes some action such as opening a new file? Then on refresh, the page knows to ignore launch params.

It also seems like the app already ought to be using history.pushState() when the new file is opened. If not, I'd give that a try. Chrome might be able to automatically handle the case where pushState has been called. Since I don't work in this area any more, +@dmurph

@dmurph
Copy link
Collaborator

dmurph commented Apr 14, 2023

Interesting. Are these file handles allowed to be saved in IndexedDB yet? @a-sully. If so - then I would imagine we should probably remove this behavior.

If not - I wonder if there is a way to differentiate the reload from the first run, so they can be used or filtered.

@alancutter
Copy link
Collaborator

alancutter commented Apr 14, 2023

Could the file handle be made available for the duration of the page session somehow rather than being lost on refresh?

MDN says:

A page session lasts as long as the tab or the browser is open, and survives over page reloads and restores.

@hybridherbst
Copy link
Author

hybridherbst commented Apr 14, 2023

@dmurph yes, file handles can be stored in IndexedDB, that's what I'm doing in the application here for "dropped" files as well.

That being said, the permissions for that are less permissive than launchQueue - I haven't found a clear pattern in Chrome but basically launchQueue is always allowed to access the fileHandle ("must" allow as per the spec, which makes sense) while file handles in IndexedDB have some heuristic for when the browser denies permission (and requires a user prompt again).

Maybe a file handle that was passed in via launchqueue could have better permissions when being retrieved from IndexedDB?

(Haven't gotten to trying out the history.pushState workaround mentioned above - but that is not an ideal state I think)

@dmurph
Copy link
Collaborator

dmurph commented Apr 14, 2023

Transferable navigable? https://html.spec.whatwg.org/multipage/document-sequences.html#traversable-navigable

I think there are thoughts about allowing these file handles to stay alive longer, in which case we should probably remove the behavior of having them re-populate on reload. I agree with @evanstade that the launch url should probably include some sort of metadata to communicate this came from a launch, and then that can be modified right away to disambiguate with a reload.

There is the other edge case that if we DO remove this from the queue, and the action url is reloaded, what does the website do? They have to handle that too. So there is always going to be some sort of reload edge case here.

@a-sully
Copy link

a-sully commented Apr 14, 2023

I'm less familiar with the specifics of the launch queue, but I can chime in on the FileSystemHandle bits of this discussion

That being said, the permissions for that are less permissive than launchQueue - I haven't found a clear pattern in Chrome but basically launchQueue is always allowed to access the fileHandle ("must" allow as per the spec, which makes sense) while file handles in IndexedDB have some heuristic for when the browser denies permission (and requires a user prompt again).

Correct, having a FileSystemHandle != having permission to access said handle

Currently, Chrome clears all File System permission state when you close the last tab for the site. Refreshing the page is considered a navigation, so this wipes permission state. File Handling initially grants "readwrite" permission to the file, but after that, it's treated just like any other FileSystemHandle that you could otherwise get from a picker or drag-and-drop.

Unsurprisingly, this is not a popular behavior WICG/file-system-access#297. Though we're actively working on making these permissions more persistent, so stay tuned!

@hybridherbst
Copy link
Author

Diving deeper into this.

I was able to use history.pushState to basically mark the launchQueue as "done" 👍 and move the handle into indexedDb instead. This solves the immediate problem but still leaves open questions, e.g.

  • should I have to do this
  • do I get the same elevated handle permissions (that launchQueue guarantees) if the handle is deserialized from indexedDb - I guess yes, but I'm not sure.

@a-sully thanks for the additional info! It doesn't quite match what I'm seeing (e.g. refresh does not always wipe permissions; PWAs seem to handle permissions differently; history.state is always empty when any field in it is a handle); would it be better if I open a clarifying question in the file-system-access repo instead of continuing here?

@dmurph I'm not exactly sure I understand what traversable navigables are/how they relate, where can I read more about that? (I tried implementing back/forward but since the fileHandle doesn't seem to persist in the state not sure how to handle that)

@a-sully
Copy link

a-sully commented Apr 14, 2023

@a-sully thanks for the additional info! It doesn't quite match what I'm seeing (e.g. refresh does not always wipe permissions; PWAs seem to handle permissions differently; history.state is always empty when any field in it is a handle); would it be better if I open a clarifying question in the file-system-access repo instead of continuing here?

Out of curiosity, are there other open tabs for the same origin when you're trying this? Permissions are only cleared when the last tab for the origin is closed

@hybridherbst
Copy link
Author

It might be that I get a bit confused because of the extra permissions that an item has when it was in the launchQueue even when the launch queue isn't processed (so a file handle from indexedDb does indeed seem to have guaranteed permissions when it's also in the launch queue). I'll test this more!

@dmurph
Copy link
Collaborator

dmurph commented Apr 14, 2023

I was thinking that one thing that could help is to propose a change to the permission lifetime of this handle to all "traversable navigables". But - would like to avoid doing that if possible :)

@alancutter
Copy link
Collaborator

I'd be keen for the reload scenario to be catered to by something more specific to file handles, having this re-enqueue behaviour in launch_handler impacts all the other kinds of launches too e.g. share target and link capturing.

@evanstade
Copy link

having this re-enqueue behaviour in launch_handler impacts all the other kinds of launches too e.g. share target and link capturing

Is this not desirable? I tend to think that after you are done with the share action, the app should navigate to a different page, so reloading wouldn't re-enqueue the launch params. However if you refreshed before the share action was completed, you probably want the launch items to be enqueued again.

@dmurph
Copy link
Collaborator

dmurph commented Apr 17, 2023

This seems like it's likely to cause more confusion than help, as it's impossible for the developer to know whether this was a launch or a reload - the use would expect the UX to be different here. E.g. in an editor, you wouldn't expect the editor to reload the file on reload, you expect to resume your editing session (Where you may have made changes). Also not re-queueing is much simpler to think about when consuming the API (at least in my opinion).

But - if we do conclude this behavior is wanted, then perhaps we can:

  1. Have developers opt-in through an options struct passed to the setConsumer method.
  2. Specify on the launchParams that this was re-sent due to reload.

I chatted with some T&S folks and this was the conclusion:

Currently, for other one-time permission models, a reload or navigation to same origin doesn't cause permission to expire, maybe can we use the same kind of lifetime for FSA handles’ ACLs as well?

I believe if that happens, then we can remove this behavior without affecting use-cases?

Edit:
Although, it looks like technically this can all be handled by having a custom url argument on the file handler case that is removed, and then detected later to detect reload. So - perhaps we don't need to spend our extremely limited cycles on this API if it works as-is. BUT I do think making sure that file handle permissions are preserved like other one-time permissions is a good thing to do. @a-sully do you know if this is how it works today?

Edit 2:
I now wonder if this is closer to something like when users submit session data & do a POST request - when we reload, there is a warning that this will re-submit data. That might be a better model here for a reload on launch - we could be displaying something like "Reloading this page will re-open .txt, are you sure you want to do that?"

@evanstade
Copy link

E.g. in an editor, you wouldn't expect the editor to reload the file on reload, you expect to resume your editing session (Where you may have made changes).

This is true, but changes are lost either way (with re-enqueue or no re-enqueue). If you had made changes and they weren't auto-saved, a good editor would have a beforeunload handler that pops up a dialog and warns you.

Making FSA permissions more persistent has been a long process. It's in progress, but doesn't work yet.

we could be displaying something like "Reloading this page will re-open .txt, are you sure you want to do that?"

I think the POST warning is because POST can have side effects which are possibly not desirable. In this example, reopening the .txt file doesn't really have a bad side effect. (Also, this interstitial is a pretty awful crutch that it would be better to avoid repeating.)

@alancutter
Copy link
Collaborator

Semantically I'm not a fan of the current reload+reenqueue behaviour because a reload isn't an app launch, it's a navigation within the same site session. I think the semantic mismatch is a root cause of the original issue raised.

@davepermen
Copy link

@evanstade in my case it directly causes a side-effect (a file upload and processing). and now (gladly still in the local debug phase) it never stops adding the file again and again.

there needs to be a way to clear items.

@dmurph
Copy link
Collaborator

dmurph commented Apr 8, 2024

Now that the permission for storing a handle in IDB has launched in m122, we should remove this behavior.

@dmurph
Copy link
Collaborator

dmurph commented Apr 8, 2024

https://developer.chrome.com/blog/persistent-permissions-for-the-file-system-access-api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants