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

Xenial domainpermissions #203

Merged
merged 48 commits into from Sep 27, 2019

Conversation

@balcy
Copy link
Collaborator

balcy commented Jun 23, 2019

This PR adds 2 new features:

  1. domain settings
    fixes #154
    fixes #147
    fixes #195

  2. domain permissions
    fixes #200

both are added for morph-browser and webapps

balcy added 30 commits Apr 27, 2019
extend DomainSettingsModel
the read methods now access the model data (no extra database queries)
…r and webapps)

make add / remove functions of DomainSettingsModel available in QML
move BrowserPage to the common folder, so that it can be used both for the browser and webapps
the user can select if the custom url scheme setting should be saved permanently (it is never saved for incognito mode)
navigationRequestDelegate for webapps was called twice on each request, remove one of the connections
use SortFilterModel (sort by domain without subdomain) to DomainSettingsPage
add domainWithoutSubdomain() to domain-settings-model
correct DownloadsPage
- adapt GeolocationPermission dialog so that the user can save the permission permanently
- save domain specific zoom factor (includes correction to the save default zoom dialog)
- add default zoom factor to DomainSettingsModel (for database cleanup of entries with default zoom factor)
adapt DomainSettingsPage
add methods to reset the databases for DomainSettings / DomainPermissions
add incognito flag to DomainPermissions
switch order: first handle custom schemes, than domain permissions
delete cache for morph-browser / webapps
add settings page link to chrome of webapps
in incognito mode links to be opened in a new incoginto window
@cibersheep

This comment has been minimized.

Copy link
Contributor

cibersheep commented Sep 16, 2019

About the design tweaks. I have some thoughts:

On the Allow/Block Dialog

imatge

I would choose a main action and color only that one.

Block Domain
About to access to domain XXX. Do you want to block this domain from now on?

[Block domain] (in red)
[Allow] (uncolored)
[Cancel] (uncolored)

Or the opposite, if you think the main action is to allow domains_

[Allow] (in green)
[Block domain] (uncolored)
[Cancel] (uncolored)

On Dialogs like this:
imatge

I would show a specific text in the button. More like:
This is happening
Do you want to [action]?
[Action]
[Cancel]

And reserve the "Ok" answer for informative texts:
This happened
This is what happened right now.
[Ok]

In the Domain Specific Settings:

imatge

  • I would go for a clear grey (Silk, even Porcelain)
  • Center the check boxes to the right with ore space (ListItems even)

In the Domain Specific Permissions:

imatge

  • Same with grey
  • I would show Not set, Blocked and Whitelisted in theme.palette.normal.backgroundText
  • I would use an exposed combo button to show the options
  • And use the Suru icons for the allowed and blocked icons :)

If you want me to help with any / all those, ping me

@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Sep 16, 2019

  • the background color has been adapted (commit 3 days ago)
    (the color is derived from the background, but relatively lighter or darker, depending on the theme, feel free to change / suggest the factor here)
  • the alignment of the checkboxes has been made (commit 3 days ago)
  • the custom url scheme dialog has been adapted (current commit)
    [the text for the checkbox does not include the domain, because it could be too long]
  • the blacklist/whitelist page has been adapted, so that the radio box labels are no longer bold and no longer green / red

for the "allow block" dialog I think neither allow, nor block is the default action:
Depending on the page and what the user wants, they have to take the decision.
And both selections result in a blacklist or whitelist entry in the database. (the same page might include content from desired and non-desired domains)

  • I think radio boxes for an enum with only 3 fixed values are easier to use than combo boxes
  • with the suru icons I'm not sure which one could be better, I've used color overlays (green / red) to make the list easier to read.
…of DomainPermissionsPage
@cibersheep

This comment has been minimized.

Copy link
Contributor

cibersheep commented Sep 17, 2019

Coool... let me try your changes and I get back to you.

Meanwhile:

  • Suru Icons you might use the ones named: "ok" for allowed, "cancel" for blocked. And they can be colored as you wish
  • About the Allow/Deny Dialog... I agree that is tricky. What about following the Restart menu style?
    Meaning: only one of the options is colored (red or green accordingly) the user still needs to choose (and not forcing block or allow as I suggested)

imatge

  • Radio buttons. The thing is I can't remember any where in the system we use them. We use ListItems for that:

ie.
GSM provider:
imatge

Lock screen after x seconds:
imatge

@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Sep 17, 2019

  • icons: ah they really look much better, especially in the dark mode (would you keep dialog-question-symbolic for "NotSet" ?

  • Allow / Deny dialog: Let's say the default would be allow (green), because most pages the user navigates to would be allowed. But if "block" is grey then, the user might not know that he will no longer be able to access the page from then.
    If we really have to avoid having a "positive" and "negative" button in one dialog, I'd prefer "Allow" grey, and "Block" red. But I like the current version most, because the colors reflect the status in the permissions page.

  • the radio boxes: the examples from the settings page are selected items from a list, but here we select an option of a single list item.
    While we already highlight the list item (the domain), the "sublist" item would need another color again to see the selcted item. Also, the selected item would have a tick on the right, that looks like the symbol to whitelisted items. I would still vote for those here, if many users complain we can change them : )

@cibersheep

This comment has been minimized.

Copy link
Contributor

cibersheep commented Sep 17, 2019

Thank you for your work, I have tested the updates you did :)

  • The info icon for Not set could work until I can design something specific (if you like)
  • For the Allow / Deny Dialog, I know that is a bit special but I really think we should be quite meticulous with the core apps therefore I would go with the top as a red block button
  • And for the radio buttons: I see what you mean. What about leaving it like it is for now and come back to it in a later revision?
…hout color

add title to AlertDialog for blocked domains
@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Sep 17, 2019

* The info icon for Not set could work until I can design something specific (if you like)

ok thanks, for now I'm fine with the icon

* For the Allow / Deny Dialog, I know that is a bit special but I really think we should be quite meticulous with the core apps therefore I would go with the top as a red block button

ok I tried to change that

* And for the radio buttons: I see what you mean. What about leaving it like it is for now and come back to it in a later revision?

it is not carved in stone : )

@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Sep 18, 2019

@UniversalSuperBox ok ready for merge ?

src/app/GeolocationPermissionRequest.qml Outdated Show resolved Hide resolved
src/app/AllowCustomSchemesDialog.qml Show resolved Hide resolved
src/app/AllowOrBlockDomainDialog.qml Outdated Show resolved Hide resolved
balcy added 3 commits Sep 20, 2019
@balcy balcy requested a review from UniversalSuperBox Sep 26, 2019
Copy link
Member

UniversalSuperBox left a comment

Here we go.

@UniversalSuperBox UniversalSuperBox merged commit 5c2a239 into xenial Sep 27, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
OTA-11 automation moved this from In progress to QA Sep 27, 2019
@balcy balcy deleted the xenial_-_domainpermissions branch Sep 29, 2019
@giiba

This comment has been minimized.

Copy link

giiba commented Sep 30, 2019

Nice work folks!

I've been testing the webapp and site specific zoom settings and it is really nice to have as it solves some of the problems that required more complicted solutions for zoom settings.

One curiosity: is there a way to apply zoom settings earlier in the rendering process? At the moment the pages are rendered then displayed, and then everything shifts afterwards as the content is fit to the zoom setting. This makes for a slightly awkward user experience as after a page loads things start moving before settling down.

@giiba

This comment has been minimized.

Copy link

giiba commented Sep 30, 2019

One thing I discovered this morning:

If a website asks for permission to location information the dialog won't let me disallow and remember the choice. If I check the 'remember decision' option the deny button is disabled.

Is there a reason for this? There are sites that ask it every page load, and I would like to just block the request permanently, as opposed to having to choose every time.

screenshot20190930_111828902

@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Sep 30, 2019

yeah good point. At that time it was implemented like a permission that can be revoked later (just a boolean flag "allow location access" in the database).
So permanent blocking was not thought of there, and would require a new field in the database.
[edit: actually "allow permanently" was renamed to "remember decision" later]

@giiba

This comment has been minimized.

Copy link

giiba commented Oct 1, 2019

Well it doesn't hinder the functionality, as Morph(stable) asks every time anyways on a site like that. Really the problem is the website.

@balcy

This comment has been minimized.

Copy link
Collaborator Author

balcy commented Oct 2, 2019

One curiosity: is there a way to apply zoom settings earlier in the rendering process? At the moment the pages are rendered then displayed, and then everything shifts afterwards as the content is fit to the zoom setting. This makes for a slightly awkward user experience as after a page loads things start moving before settling down.

it might be possible to use WebEngineLoadRequest.LoadStartedStatus instead of WebEngineLoadRequest.LoadSucceededStatus, but we have to make sure that we still detect whenever the current domain changes. (WebViewImpl.qml, 879)

@giiba

This comment has been minimized.

Copy link

giiba commented Oct 3, 2019

I played a bit with WebEngineView.LoadStartedStatus but it doesn't seem to trigger. If the current LoadSucceededStatus is replaced the zoom is never applied, and if I add a fourth 'if' under onLoadingChanged for LoadStartedStatus (and copy+paste from LoadSucceededStatus) there is no difference in behaviour.

Might be futile, I don't know enough, and it really isn't a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OTA-11
  
QA
4 participants
You can’t perform that action at this time.