Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

PromptFeature refactoring #10323

Closed
Mugurell opened this issue May 24, 2021 · 7 comments
Closed

PromptFeature refactoring #10323

Mugurell opened this issue May 24, 2021 · 7 comments
Labels
<prompts> Component: feature-prompts 🔬 Research Problems without a solution that need research.

Comments

@Mugurell
Copy link
Contributor

Mugurell commented May 24, 2021

Following #8989 and #10273 it was seen that there are some situations which can be better handled.

Most of these situations are to the activePrompt - what that should represent and how should that be updated. Currently it can be initialized or cleared without a new Prompt being displayed / dismissed and can remain cleared with other Prompts displayed.

Things to consider:

  • unify dialogs and pickers under a single interface
  • keep a list of prompts (and pickers) / have a way of getting all of them in PromptFeature in the order shown on screen
  • have an easy way to map Prompts to PromptRequests and reverse
    • so that when one such Prompt is consumed by the user the PromptRequest for which it was shown can be removed form the Session and can be sent to GV to be consumed.
  • decide what should happen from an UX standpoint if the user is actively interacting (what should this mean?) with a Prompt and a new PromptRequest comes
    Currently all Prompts are modals with new Prompts stacked on top of each other (not sure how common is to get more Prompts but it definitely happens)
  • since now prompts are consumed by uid maybe consuming them by tab can be deprecated.

┆Issue is synchronized with this Jira Task

@Mugurell Mugurell added the <prompts> Component: feature-prompts label May 24, 2021
@jonalmeida
Copy link
Contributor

I noticed that the constructor of the PromptFeature is larger than my screen now. 😄 That's usually a sign that the feature surface could possibly be split into several parts where needed.

cc: @Amejia481 @gabrielluong who have worked on the feature and may have ideas too.

@jonalmeida jonalmeida added the 🔬 Research Problems without a solution that need research. label May 31, 2021
@gabrielluong
Copy link
Member

@grigoryk You were working on some prompt refactoring in #9752, perhaps you can share a draft or vision for this work even if it's incomplete.

@Amejia481
Copy link
Contributor

Amejia481 commented Jun 1, 2021

I noticed that the constructor of the PromptFeature is larger than my screen now. 😄 That's usually a sign that the feature surface could possibly be split into several parts where needed.

cc: @Amejia481 @gabrielluong who have worked on the feature and may have ideas too.

Yes, completely agree, I think one first step is to put all the logins/credit cards callbacks into a delegate, we could add a delegate similar to ShareDelegate.

@Amejia481
Copy link
Contributor

I additionally we want to remove this function, and any similar, as we only want to delete by UID and be 100% sure that we are removing the right prompt.

@Mugurell
Copy link
Contributor Author

Mugurell commented Jun 3, 2021

Regarding future refactorings was talking with Arturo about how desktop treats this http://jsfiddle.net/pahund/e9fv3j1m/show (enter some credentials, press submit, wait 5 seconds):

  • Two prompts are shown at the same time and the user can interact with any of them but
  • The login prompt is part of the browser
  • The alert is web content

This distinction exists now in the app also (though not really enforced), there are Select*Prompts implemented as Views and then all the others prompts implemented as modal Dialogs.
I first though they should be grouped under the same interface to be easier to cache/queue them but maybe it makes more sense to keep them separated and similarly to desktop:

  • allow having two prompts if of different types at the same time on the screen
  • otherwise queue all prompt requests and show them one by one only after the previous being consumed

Don't know where this would leave the FilePicker. Maybe it makes more sense to treat it as a queued prompt though having it opening another app to pick files some time after the user action might seem too intrusive. Could this be one PromptRequest that should just be dropped and not queued if another Prompt (as a Dialog) is shown?

@grigoryk
Copy link
Contributor

grigoryk commented Jun 3, 2021

Here's my quick stab at refactoring this a while back. It's largely incomplete, but perhaps still useful: grigoryk@bf806c2 - I meant to come back to it before we need it for the autofill work, but alas.

The idea in that refactor is to admit that there are fundamentally two types of prompts - those that originate from web content, and those that originate from the "chrome", the application itself. You'll notice that we already have this split, basically, but it's all spaghetti'd together.

So, the refactor introduce two types - WebPromptRequest and AppPromptRequest, and propagates this distinction. E.g.: there are now two distinct classes responsible for handling specifics - AppPromptFeature and WebPromptFeature, with the parent PromptFeature responsible for a) delegating appropriately, and b) providing any shared functionality.

Regarding queuing, my understanding is that we 1) should never display multiple prompts, and 2) should never display prompts from any tab that isn't currently selected (and being actively viewed). I think this applies to all types of prompts that we have? If there are certain prompts/scenarios where this doesn't apply, let's capture that in the types we introduce here.

@gabrielluong
Copy link
Member

Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1796356

Change performed by the Move to Bugzilla add-on.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
<prompts> Component: feature-prompts 🔬 Research Problems without a solution that need research.
Projects
None yet
Development

No branches or pull requests

5 participants