Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #5292: Add "Force Paste" item in menu to bypass onpaste handlers #5293

Merged
merged 1 commit into from
May 10, 2022

Conversation

kylehickinson
Copy link
Collaborator

Summary of Changes

This pull request fixes #5292

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan

Screenshots:

image

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@kylehickinson kylehickinson requested a review from a team April 26, 2022 20:31

"use strict";

Object.defineProperty(window.__firefox__, "forcePaste", {

Choose a reason for hiding this comment

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

do we need to add this stuff to window? Seems like it would be better to just keep the script in a const somewhere and execute it directly?

Choose a reason for hiding this comment

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

I guess we are doing this in other places, but it doesn't really seem ideal to me. What do you think about a follow up to call the JS directly for this kind of stuff? I'm not sure if ios has resource paks in chromium, but if it does we can still manage these as js files, but read the value as a string (sync read from memory, not disk). I don't think it's terrible though to just embed the JS in some consts somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it was for namespacing and keeping everything under one variable since prior to iOS 14 content world sandboxes (and still for a bunch of these scripts) they are visible to the webpage. No resource pak's but the reason they're like this is because they're webpack'd and injected via the WebKit APIs

We can technically get rid of window.__firefox__ if we want but we possibly would want to namespace things for non-sandboxed scripts

Choose a reason for hiding this comment

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

Well, I guess I'm wondering why they need to by part of window at all. Can we not just execute the JS directly instead of calling a method that we add to window?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I know they don't need to be a a part of window at all

Choose a reason for hiding this comment

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

I don't think we need to do anything in this PR, but I do think we should a follow-up to change all of these. I assume this was following some pattern from FF and that's why it has __firefox__ which is also a bit weird to have in Brave. I mean we have window.chrome in Brave, but we have to for extensions to work.

Copy link
Collaborator

@Brandon-T Brandon-T May 9, 2022

Choose a reason for hiding this comment

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

We usually have this so we don't inject JS over and over again. Inject once, and call anywhere, any amount of times. This JS is actually injected into a sandbox called .defaultClient, so pages won't be able to access it :)

I guess if we want it could technically be loaded from file into memory, and injected again, every time the user hits paste. Either way seems fine to me.

Client/Frontend/Browser/Tab.swift Outdated Show resolved Hide resolved
@kylehickinson kylehickinson merged commit 8fcfe03 into development May 10, 2022
@kylehickinson kylehickinson deleted the force-paste-menu branch May 10, 2022 13:49
@kylehickinson kylehickinson added this to the 1.39 milestone May 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Force paste" menu item that bypasses onpaste handlers
3 participants