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

Per-site Redirect Opt-out #687

Merged
merged 11 commits into from
Mar 5, 2019
Merged

Per-site Redirect Opt-out #687

merged 11 commits into from
Mar 5, 2019

Conversation

lidel
Copy link
Member

@lidel lidel commented Feb 20, 2019

This PR aims to:

Changes

Icon-based global redirect toggle

To avoid miss-clicks, confusion, and to disincentivise disabling global redirect, we moved global toggle from Browser Action menu to the utility icon row, and used custom "redirect" icon:

Enabled Disabled
on-2019-01-20--22-59-30 2019-01-20--22-59-51

When you start seeing the face above, you cannot unsee it.

Redirect opt-out per site

  • menu item in Active Tab section enables user to disable gateway redirect on current website
  • when clicked on regular site toggles redirect for current FQDN and all its subdomains
  • when clicked on /ipns// (DNSLink) website, toggles redirect for
  • preference changes triggers reload for current tab
  • DNSLink website reload changes between IPNS path and original URL

    peek 2019-02-20 16-05
    Redirect opt-out on DNSLink website loaded from local gateway restores original URL

  • Redirect preference applies not only to request with FQDN of the active tab, but also to all subresource requests that have it in originUrl (Firefox) or Referer header (Chrome). This means toggle is useful to restore functionality of complex websites such as d.tube (which uses IPFS subresources from various subdomains):

    peek 2019-02-20 16-08
    Redirect opt-out on regular website: functionality is restored thanks to automatic reload

Opt-out Test URLs

Use below to test the per-site opt-out:

  • http://docs.ipfs.io (DNSLink website at /ipns/docs.ipfs.io)
  • https://d.tube/#!/v/elenasteem/890wiapc (regular website with custom JS)
    • video won't load if redirect is active on initial load
    • disabling redirect for d.tube restores playback (note: additional toggle does not impact current video, as data is cached by the website)

Other

  • added subtle animation to utility icons
  • global redirect icon will enable integrations if clicked when in suspended state
  • menu items specific to the Active Tab are in a section marked with additional border
    (just a prototype, needs refinement)

TODO

  • fix tests
  • improve UX of "Active Tab" section in browser Action menu
  • minimize redirect-icon.js (100K right now)
  • expose per-site opt-outs as editable field on Preferences screen
    • mvp: one FQDN per line, sort lexicographically
  • remove requestId from caches in onCompleted || onErrorOccurred
  • move use of browser.* from browser action page to background page

looking for feedback and ideas!

Changes:

- moved global redirect toggle from Browser Action menu to the utility
  icon row, under "redirect" icon
- added animation to utility icons
- global redirect icon will enable integrations if clicked when in
  suspended state
- menu items specific to the Active Tab are marked with additional
  border (just a prototype, needs refinement)
- Redirect opt-out per site
  - new menu item in Active Tab section
  - when clicked on regular site toggles redirect for current FQDN and
    all its subdomains
  - when clicked on /ipns/<fqdn>/ (DNSLink) website, toggles redirect for <fqdn>
  - after redirect preference changes for current website, the tab is
    reloaded
  - DNSLink websites are reloaded to with URL change between IPNS path
    and original URL
  - redirect preference applies not only to requests to URLs with with FQDN
    of the active tab, but also to all subresource requests that have it
    in `originUrl` (Firefox) or `Referer` header (Chrome)
@ghost ghost assigned lidel Feb 20, 2019
@ghost ghost added the status/in-progress In progress label Feb 20, 2019
@fsdiogo
Copy link
Contributor

fsdiogo commented Feb 21, 2019

This is pretty cool @lidel 👌

In regard to the UI, I'd drop the dashed border as it gives a temporary feeling. Here's a suggestion.

One minor UI change: when you hover over the power or redirect toggle, the text always refers to when everything is turned on: Suspend all IPFS integrations and Stop all gateway redirections, even when it's already suspended or stopped. So I'd say to change to Toggle all IPFS integrations and Toggle all gateway redirections or something of the like!

The request for main page (request.type=main_request) is often optimized
early by preloading DNSLink etc.

We switch tests to one of subrequest types to ensure the test is not
impacted by main_test logic.
@lidel lidel requested review from olizilla and removed request for olizilla February 23, 2019 20:16
This removes dotted border and introduces familiar grandient
as sugegsted in
#687 (comment)
@lidel
Copy link
Member Author

lidel commented Feb 23, 2019

@fsdiogo thank you, I switched style to follow your suggestion, looks bit cleaner:

updates-2019-02-23--21-30-03

This adds a textarea for editing per-site opt-outs.
Array is converted into multi-line text.
Entries that are not valid FQDNs are dropped.
The list is sorted lexicographically.
@lidel
Copy link
Member Author

lidel commented Feb 24, 2019

To keep this PR simple, added a textarea on Preferences screen for editing opt-outs:

2019-02-24--02-11-30

  • Array is converted into multi-line text.
  • Entries that are not valid FQDNs are dropped.
  • The list is sorted lexicographically.
  • Added basic copy, needs to be reviewed

@lidel
Copy link
Member Author

lidel commented Feb 26, 2019

Created some GIFs for README:

Disable Gateway Redirect Per Website

Active Tab actions include option to opt-out current website from Gateway redirect of any IPFS subresources.
Disabling redirect for DNSLink website will restore original URL as well:

per-site-peek 2019-02-26 00-23

Disable Gateway Redirect (Global Toggle)

A handy toggle to disable all gateway redirects while keeping all other features enabled:

redirect

Suspend IPFS Extension

The "power" icon can be used to temporarily suspend all IPFS integrations
(redirects, API status, content scripts, protocol handlers etc).
Useful during testing. Extension can be re-enabled with a single click:

screenshot of suspend toggle

Added ipfsPathValidator.isRedirectPageActionsContext with tests:
Per-site toggle won't be shown on internal pages and non-IPNS urls
loaded from local gateway (confusing to users)

Removed redundant variables in Browser Action context and made UI less
jittery.
@lidel lidel force-pushed the feat/redirect-toggle-per-website branch from e2b2fe3 to 82cae05 Compare February 26, 2019 13:44
@lidel lidel requested review from a team, hugomrdias and cwaring February 26, 2019 13:46
@lidel
Copy link
Member Author

lidel commented Feb 26, 2019

Ok, I think this has been brewing for long enough.
Would appreciate review/sanity check or at least some 👍 or 👎 on:

@lidel lidel marked this pull request as ready for review February 26, 2019 14:04
@lidel lidel changed the title [WIP] Per-site Redirect Opt-out Per-site Redirect Opt-out Feb 26, 2019
@olizilla
Copy link
Member

@lidel in general I'm +1 on the idea. If we borrow from the UI language of IPFS Web UI, then we'd use a small caps label like:

companion-active-tab

Which i think works pretty well

@olizilla
Copy link
Member

@ericronne I know you have no context for this, but it's would be a useful exercise to talk you through it and get your thoughts.

@hugomrdias
Copy link
Member

hugomrdias commented Feb 26, 2019

@lidel i made a prototype which packs lots of ideas out of this PR scope

https://codesandbox.io/s/14y8r7qjo3

but basically i like toggles with explicit label and tooltip explaining everything like i'm 5. I prefer label texts that don't change with state.


[@lidel] This is awesome! I extracted this to separate issue – see #689 (comment)

@ericronne
Copy link

@olizilla yes, would love a walkthrough! Looks like an interesting workstream; would love to get some context before attempting to lend any sage (ha) advice …

Copy link

@ericronne ericronne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a brand-newbie to IPFS, i don't yet grok much about this tool. Once i've taken the time to learn more, i'll open a new issue with broader design suggestions. For now, i would enclose the arrow icon with a circle, to better coordinate with the other two buttons. Then maybe move it to the front of the row. Otherwise i wonder if users might think that clicking it performs some kind of swapping function on the two functions that it lies between (f that makes any sense). Thanks!

image

It was raised during the review that labels should not change.

This changes the UI of redirect toggles from dynamic label
to static label + @material/switch
We already have it as regular menu item, and the switch glyph seems to
confuse people about its purpose, so let's remove UI cruft and hide the
icon toggle for now.

Another small tweak is to fade out global toggle instead of hiding it
in offline mode, which removes unnecessary jitter from UI.
@lidel
Copy link
Member Author

lidel commented Mar 4, 2019

Thank you for the feedback!

Just pushed some tweaks, here is the summary of changes:

  • Menu sections are now separated with webui-like header, as suggested by @olizilla
  • Applied suggestion from @hugomrdias: labels do not change and the state is represented using toggle switch (@material/switch)
    • I applied this change to Pin/Unpin as well (it swapped label before)
  • Removed "redirect toggle" icon for now, and replaced it with "Gateway Redirect" menu item
    • The icon confused a lot of people and @ericronne raised good points about placement and its general shape. We can revisit the idea with updated SVG in the future.
  • Switched to fading-out instead of hiding some inactive elements to reduce UI jitter.
regular website dnslink website offline mode
1-github com-2019-03-04--19-16-33 1-dnslink-site-2019-03-04--19-18-32 1-dnslink-site-off-2019-03-04--19-19-56

@hugomrdias
Copy link
Member

looks awesome !!!

I would move active tab up and tools down and change "Gateway Redirect" to "Redirect to gateway".

This way both global and tab redirect are together in the UI to create a visual connection between them.

Another option is to change "Redirect on docs.ipfs.io" to "Gateway Redirect on docs.ipfs.io" but this one makes it bigger and with big origins it might get weird.

@lidel
Copy link
Member Author

lidel commented Mar 4, 2019

@hugomrdias 👌

regular website dnslink website offline mode
github-2019-03-05--00-32-02 ipfs-on-2019-03-05--00-32-32 ipfs-off-2019-03-05--00-33-03

I believe it is ready for a test ride in Beta channel.
Will merge this as-is, we can tweak copy/ux in separate PRs.

Update: Ready for testing: v2.7.5.748 (Beta)
UX discussion continues in: Design: New UI for IPFS Companion #689

@lidel lidel merged commit 678c714 into master Mar 5, 2019
@ghost ghost removed the status/in-progress In progress label Mar 5, 2019
@lidel lidel deleted the feat/redirect-toggle-per-website branch March 5, 2019 00:06
lidel added a commit that referenced this pull request Mar 12, 2019
When we introduced option to opt-out from redirect per site (#687), it came with a side effect of removing IPFS context actions. 

This PR (aka Show IPFS Actions on DNSLink Sites):

- Adds context actions on DNSLink sites (when redirect is disabled)
- Adds a bunch of tests
- Tweaks behavior of pin/unpin via browser action menu
- Works around missing dnslink resolver under js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirection toggle in popup menu Redirect opt-out per website
5 participants