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

Execute print user script in page content world #238

Merged
merged 3 commits into from
Nov 17, 2021

Conversation

alistairjcbrown
Copy link
Member

Task/Issue URL: Related to https://app.asana.com/0/1199230911884351/1200442572797700/f
Tech Design URL:
CC:

Description:

The print user script needs to execute in the page content world so that
it mutates the globals used by the page which the user is interacting
with. The existing setup runs scripts under the default content world
(which is what you generally want), but this script is a special case as
we want to overwrite a global function on the window object.

Steps to test this PR:

  1. Go to https://printablefreecoloring.com/print/animals/coloring-alligator-414.jpg
    • This is actually a webpage which calls window.print()
  2. Expect print dialog to appear

image

Testing checklist:

  • Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@samsymons samsymons self-assigned this Sep 29, 2021
@samsymons
Copy link
Collaborator

Unfortunately I don't have time to finish up this review before heading out for the rest of the week, unassigning myself for now. But, I do think we should add a contentWorld property to UserScript and let scripts override it directly. Any thoughts on this @tomasstrba @brindy?

@samsymons samsymons removed their assignment Sep 29, 2021
@tomasstrba
Copy link
Contributor

@alistairjcbrown, I'm sorry, but this is a bit harder to follow. Is the next step review or is additional work required?

@samsymons
Copy link
Collaborator

Checking in on this PR, I'd like to close it out this week. I have some BSK changes in mind that could help out with this, will see about implementing those during Quick Wins Days this week.

@samsymons samsymons self-assigned this Oct 13, 2021
@alistairjcbrown alistairjcbrown force-pushed the abrown/overwrite-window-print branch 2 times, most recently from 001a146 to cb0f965 Compare October 15, 2021 10:05
@alistairjcbrown
Copy link
Member Author

alistairjcbrown commented Oct 15, 2021

@tomasstrba @samsymons

Is the next step review or is additional work required?

This PR works as expected, but to get there PrintingUserScript had to reimplement makeWKUserScript instead of getting it from BrowserServicesKit, and we now having (more) content world specific functions in WKUserContentControllerExtension.

I guess ideally this PR could have some review to see if the approach is suitable for now -- and weigh that against if we'd first want to, a) figure out a nicer way to make the chosen content world configurable and, b) then back port this approach to BrowserServicesKit.
My vote would be land this PR, figure out the above, and then update at the same time as bumping the BrowserServicesKit version used -- but I'm also not enough in the weeds to know the gotchas of that approach.

Edit: There may also be tests failing -- I have access to bitrise now, so I'll take a look once this CI run finishes

@alistairjcbrown alistairjcbrown force-pushed the abrown/overwrite-window-print branch from cb0f965 to 0a68d1c Compare October 15, 2021 11:12
@samsymons
Copy link
Collaborator

samsymons commented Oct 25, 2021

@alistairjcbrown Finally taking a proper look at this, so sorry for the wait

EDIT: yet again F&F has foiled this, despite my best attempts

The print user script needs to execute in the page content world so that
it mutates the globals used by the page which the user is interacting
with. The existing setup runs scripts under the default content world
(which is what you generally want), but this script is a special case as
we want to overwrite a global function on the window object.
@alistairjcbrown alistairjcbrown force-pushed the abrown/overwrite-window-print branch from 0a68d1c to cb58a76 Compare November 15, 2021 14:48
@alistairjcbrown
Copy link
Member Author

@samsymons @jonathanKingston Updated to use requiresRunInPageContentWorld from BrowserServicesKit 👍

@alistairjcbrown
Copy link
Member Author

@jonathanKingston @samsymons Updated for feedback -- the diff for this PR is looking pretty sweet! 😁

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

@alistairjcbrown You weren't kidding, this is super clean. ✨ LGTM!

Copy link
Collaborator

@jonathanKingston jonathanKingston left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up based on feedback! 💯

@alistairjcbrown alistairjcbrown merged commit abf2820 into develop Nov 17, 2021
@alistairjcbrown alistairjcbrown deleted the abrown/overwrite-window-print branch November 17, 2021 11:16
samsymons added a commit that referenced this pull request Nov 22, 2021
* develop:
  Avoid using a specific browser as the default. (#341)
  Closing animation duration changed to 0.15 (#333)
  Hiding the favicon view when favicon is nil (#326)
  export bookmarks (#339)
  Ensure ITP database is cleared (#328)
  Execute print user script in page content world (#238)
  move the next/previous handlers to the main view controller (#331)
  Search with DuckDuckGo in PDF Context Menu (#323)
  Fix  Back button takes to r.duckduckgo.com (#321)
  Track the existing print handler to avoid DoS attacks. (#330)
  Minor copy fixes (#325)
  support cmd-option-left/right for navigating tabs (#324)
  Version 0.17.5
  Debounce privacy entry point update (#309)
  Change major tracker network threshold (#319)
  Check for click handler on printing user script (#318)
  Fix Homepage navigation (#322)
samsymons added a commit that referenced this pull request Nov 25, 2021
* develop: (23 commits)
  Update the credit card model + UI tweaks (#342)
  0.17.6
  Add Usage Pixel (#336)
  Bump find-in-page to latest version (#337)
  Forward delete collision with suffix resolved (#334)
  Avoid using a specific browser as the default. (#341)
  Closing animation duration changed to 0.15 (#333)
  Hiding the favicon view when favicon is nil (#326)
  export bookmarks (#339)
  Ensure ITP database is cleared (#328)
  Execute print user script in page content world (#238)
  move the next/previous handlers to the main view controller (#331)
  Search with DuckDuckGo in PDF Context Menu (#323)
  Fix  Back button takes to r.duckduckgo.com (#321)
  Track the existing print handler to avoid DoS attacks. (#330)
  Minor copy fixes (#325)
  support cmd-option-left/right for navigating tabs (#324)
  Version 0.17.5
  Debounce privacy entry point update (#309)
  Change major tracker network threshold (#319)
  ...
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.

5 participants