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

MV3 Beta 3.0.0.1114: regression in Brave UX #1251

Closed
Tracked by #1152
lidel opened this issue Aug 2, 2023 · 5 comments · Fixed by #1256
Closed
Tracked by #1152

MV3 Beta 3.0.0.1114: regression in Brave UX #1251

lidel opened this issue Aug 2, 2023 · 5 comments · Fixed by #1256
Assignees

Comments

@lidel
Copy link
Member

lidel commented Aug 2, 2023

@whizzzkid I've found two bugs in Brave:

Problem in MV3 Beta 3.0.0.1114

When built-in node is used in Brave:

Companion disables "My Node" button:

2023-08-02_20-11

And redirect happens to http:// URI instead of native ipfs:// and ipns:// :

2023-08-02_20-11_1

Expected behavior

  • Opening WebUI after clicking "My node" should work.
  • When user is redirected to native URI when built-in Brave node type is used:

    2023-08-02_20-12

@lidel lidel added need/triage Needs initial labeling and prioritization mv3-beta-bugs labels Aug 2, 2023
@whizzzkid whizzzkid self-assigned this Aug 2, 2023
@whizzzkid
Copy link
Contributor

@lidel I think that's because the redirect uri being generated for brave is not ipfs:// or ipns:// hence the redirect happens to http:// the pattens are strictly based on the redirect-uri

I think I can have fix for this.

@whizzzkid whizzzkid removed the need/triage Needs initial labeling and prioritization label Aug 3, 2023
@whizzzkid
Copy link
Contributor

@lidel

Issue 1: My Node Links, it seems to work for me. I'm not sure what broke or fixed it.

Screenshot 2023-08-14 at 4 18 13 AM

Issue 2: not redirecting to native ipns:// or ipfs://

Screenshot 2023-08-14 at 4 19 55 AM

The simple fix I was thinking was reverting to the old blocking API like in FF, but that's not the case here, brave does not implement the blocking api in MV3, yet, the specific rules to native protocols seems to be missing, It will need some more work. I can work on it as an enhancement after initial MV3? or will that be a blocker?

@lidel
Copy link
Member Author

lidel commented Aug 14, 2023

@whizzzkid thanks for looking into this. It feels like a blocker for publishing to stable channel, some details, and ideas below.

On why we may want to fix this before mV3 ships

Brave uses the package we publish to Chrome Web Store, so by shipping MV3 without fixing this, we introduce an unnecessary regression in Brave, which aims to be the state of the art for native handlers like ipfs://.

We would be undoing years of progress: we have a browser which supports native handler, but we make it act like regular Google Chrome or Firefox. We also alienate existing users who had native protocol for years now, and if someone was using something like wallet over it, they may lose access to it unless they type ipfs://cid manually.

Not the end of the world, but imo easier to fix now,
rather than to deal with fallout and responding to issues here and in brave.

Potential implementations paths

I don't know if/when a blocking API in MV3 will happen in Brave. We could ask Brave to hardcode and allow blocking API from MV2 for Companion, but no ETA on this. Imo not worth asking, as it is not a stable foundation we should rely upon going forward anyway.

See if we can use declarative API.
If not, perhaps we could fix the address bar in Brave in less invasive ways.

One idea, introduce an additional listener in Brave that looks at chrome.webNavigation events of the top level documents (URL in address bar), and when a resource with http:// URL of the local gateway is loaded, update the tab and replace it with ipfs:// equivalent (so only ipfs:// shows in browsing history).
Bit hacky, but reload will happen so fast end user will not notice. The benefit of this approach is future-proof, and will work no matter how broken declarative API in MV3 is :)

@whizzzkid
Copy link
Contributor

@lidel or, what if I add an additional listner if we're in brave which redirects:

originURI: ^https?://localhost:<port>/(ipfs|ipns)/(<blah>)
redirectURI: \\1://\2

since all rules are relative to localhost, adding a default redirect to protocol handler should be ok?

@whizzzkid
Copy link
Contributor

fixed in #1270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants