sailfishos / sailfish-components-webview

Mozilla Public License 2.0
13 stars 23 forks source link

Allow clients to customize the appearance and behavior of popups #104

Closed chriadam closed 3 years ago

chriadam commented 3 years ago

@rainemak @llewelld @okodron @atatarov Here is a proof of concept which at least works, although there are definitely some open discussion points which need further thought:

1) What if the client doesn't want a specific popup to be a dialog which is pushed to the pagestack, but instead some floating overlay item? If this is wanted, then we will need to extend the popupComponents dictionary to include some hint about the component type (e.g. "Dialog" vs "Item") which the PopupOpener.qml will use when determining how to load and show the component).

2) Currently we don't enforce that the custom component being loaded will extend the related interface type - mostly because I wanted to keep the ability to lazily-load the components from URL rather than compiling everything up front. Maybe this isn't a problem, but it's something to be aware of..

3) I wasn't too sure about the interplay between the "remember" value which is used for some cases (e.g. AuthDialogInterface and LocationDialogInterface) and the "preventDialogs" value which is exposed in some other cases. I tried to maintain the existing behaviour, but I might have messed something up - I guess that part requires careful review.

chriadam commented 3 years ago

After some discussion with Raine today: yes, we want to support item-derived (rather than dialog-derived) popups, so I need to add the type hint to the dictionary. I also need to add a signal (which is forwarded to the webview and emitted if the client has provided a custom components dictionary) which notifies the client when a popup has been instantiated (and what type of message triggered it).

We still need to consider e.g. should we be exposing the current embedlite message strings ("embed:alert" etc) either as keys in the dictionary or as parameter in the signal, or should we instead use some well-defined strings which identify each popup component type, instead?

chriadam commented 3 years ago

Implemented the above requirements (supports item-derived custom popups, added aboutToOpenPopup signal), and also rebased on top of current master. There were a few conflicts, but nothing too dramatic. Some feedback would be appreciated.

chriadam commented 3 years ago

@mkenttala review also appreciated, with regards to the way I resolved the merge conflict - hopefully I didn't break anything? From testing, the delayed popup load still seems to work as required, but perhaps I am wrong.

mkenttala commented 3 years ago

@mkenttala review also appreciated, with regards to the way I resolved the merge conflict - hopefully I didn't break anything? From testing, the delayed popup load still seems to work as required, but perhaps I am wrong.

LGTM code wise, not been able to test yet as I seem to be having some build/dependency issues.

chriadam commented 3 years ago

Updated as per feedback from Raine. Now clients specify a PopupProvider instance (which has well-defined properties for each of the popup types), rather than a raw JSON dict.

chriadam commented 3 years ago

Fixed as per some review comments, and rebased.

chriadam commented 3 years ago

Should I wait for https://github.com/sailfishos/sailfish-components-webview/pull/98 to be merged, and then rebase this PR on top (and fix up etc)?

rainemak commented 3 years ago

No need to wait for #98

chriadam commented 3 years ago

Rebased. I had to resolve a merge conflict related to the loginSaved() signal - @llewelld can you check that I didn't break anything? The dialog no longer emits the loginSaved() signal, but only the PopupOpener emits it (if the dialog is accepted).

llewelld commented 3 years ago

Rebased. I had to resolve a merge conflict related to the loginSaved() signal - @llewelld can you check that I didn't break anything? The dialog no longer emits the loginSaved() signal, but only the PopupOpener emits it (if the dialog is accepted).

I tested and the loginSaved() signal is still working for what it's needed for after the rebase; thanks for flagging it.

atatarov commented 3 years ago

Thank you! It looks good. One question about some dialogs. There are these

  1. https://github.com/sailfishos/sailfish-browser/pull/815/files Now it handle only sailfish-browser, but it could be provide also for WebView.
  2. https://github.com/sailfishos/sailfish-components-webview/pull/98/files I can see the same things for LocationDialog, PopupBlockedDialog and WebRtcPermissionDialog, it could be as a separate dialog as one layer above UserPrompt, for ex PermissionRequestDialog with "TextSwitch" and much more, it would be good to have it in qmldir, I guess.
rainemak commented 3 years ago

Thank you! It looks good. One question about some dialogs. There are these

1. https://github.com/sailfishos/sailfish-browser/pull/815/files
   Now it handle only sailfish-browser, but it could be provide also for WebView.

2. https://github.com/sailfishos/sailfish-components-webview/pull/98/files
   I can see the same things for LocationDialog, PopupBlockedDialog and WebRtcPermissionDialog, it could be as a separate dialog as one layer above UserPrompt, for ex PermissionRequestDialog with "TextSwitch" and much more, it would be good to have it in qmldir, I guess.

Let's create a new task about PopupBlocker thingie and do not block this one with it. Similarly for WebRTC work that is not yet landed, let's create new tasks and handle that alignment separately.

chriadam commented 3 years ago

Created JOLLA-173 for the popup blocker task. For the other one (implementing some PermissionPopupInterface type) I think that can be done once we have some concrete sub-types (i.e. after MR#98 is merged also).

rainemak commented 3 years ago

Rebase after https://github.com/sailfishos/sailfish-components-webview/pull/111