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.

Sign up for GitHub

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

Jump to bottom

Move the vomnibar to an iframe #1139

Closed
wants to merge 4 commits into from
Closed

Move the vomnibar to an iframe #1139

wants to merge 4 commits into from

Conversation

mrmr1993
Copy link
Contributor

This PR

@mrmr1993 mrmr1993 mentioned this pull request Aug 21, 2014
Closed
@mrmr1993 mrmr1993 force-pushed the vomnibarToPage branch 2 times, most recently from 5604da8 to de016dc Compare August 21, 2014 23:13
philc
philc reviewed Sep 2, 2014
View reviewed changes
tests/dom_tests/dom_tests.html
@@ -53,5 +54,12 @@

<div id="output-div"></div>

<div id="vomnibar" class="vimiumReset">
Copy link
Owner

Choose a reason for hiding this comment

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

Why does this need to be here now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't work out how to run the tests in the vomnibar iframe, so I injected the iframe script directly into the page and use this in place of the iframe's dom.

It's a hack, any idea how best to fix it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be helpful to add a comment in this regard, so we know in the future why it's here.

@philc
Copy link
Owner

philc commented Sep 2, 2014

Cool change. In terms of weight, I don't like the extra iframe machinery, but I do like that this decouples the vomnibar resources (css and some of the js) into other files outside of the content script and main vimium css file.

@mrmr1993
Copy link
Contributor Author

mrmr1993 commented Sep 2, 2014

@philc I've added some comments to the code, as per your suggestions, and will look into tidying up the tests.

@philc
Copy link
Owner

philc commented Sep 5, 2014

Hey @mrmr1993, I'm just about ready to merge this in. Two requests:

  1. Can you rebase this onto master?
  2. The UI isn't as responsive now that it's in an iframe. Type a phrase which has a long list of completions, and then as you type, watch it shorten. The bottom edge of the dialog is laggy. Compare this with the current implementation. Can this be improved? I suspect what's happening is that you're resizing the iframe which in turn resizes the vomnibar's HTML element. If that's the case and if that's causing the slowness, one change you could make is to instead resize the vomnibar HTML element inside the iframe, and then also resize the iframe. As long as the iframe has a transparent background, it won't be visibly apparent if the iframe is slower to resize than the vomnibar's HTML container.

mrmr1993 and others added 3 commits September 17, 2014 22:54
@mrmr1993
Move the vomnibar to an iframe
5885bc7
@sharat87 @mrmr1993
Allow open popups from bookmarklets. Fix philc#968.
f9cb424 
Use the `openUrlInCurrentTab` message to open bookmarklets so they are opened
via `chrome.tabs.update` instead of an injected script element. This allows
bookmarklets to open popups.
@mrmr1993
Hacks to make Vomnibar tests work again
fdf592b
@mrmr1993
Copy link
Contributor Author

Hi @philc, I've tried to address your comments:

  1. I've now rebased onto master, so it should merge cleanly.
  2. I've moved the box-shadow and border properties from the iframe back to the vomnibar element, and resized the iframe to fill the screen vertically. This stops the lag, but now the area below the vomnibar is now "unclickable" (due to the iframe in the way). Presumably this shouldn't be an issue for the vast majority of users.

@mrmr1993 mrmr1993 mentioned this pull request Sep 19, 2014
Closed
@mrmr1993 mrmr1993 mentioned this pull request Oct 13, 2014
Closed
@mrmr1993 mrmr1993 mentioned this pull request Oct 30, 2014
Closed
@smblott-github smblott-github mentioned this pull request Oct 31, 2014
Closed
@smblott-github
Copy link
Collaborator

@mrmr1993: Is the problem with clicking below the vomnibar because the iframe is bigger than its contents? If so, might we dynamically adjust the iframe's height:

@mrmr1993
Copy link
Contributor Author

@smblott-github originally it was adjusted dynamically, but I just changed it to be height: 100% after @philc's comment:

The UI isn't as responsive now that it's in an iframe. Type a phrase which has a long list of completions, and then as you type, watch it shorten. The bottom edge of the dialog is laggy...

Before it was set by passing a message via the background page, which was causing the lag. I'll look into accessing window.frameElement from the Vomnibar iframe and see if that would work.

@mrmr1993
Copy link
Contributor Author

No joy; going both ways we get

Blocked a frame with origin "ORIGIN_HERE" from accessing a cross-origin frame.

The options we are left with then are:

  1. Resize via messages through the background page, with corresponding lag.
  2. Have a height: 100% iframe that captures clicks where there is not necessarily any iframe content.

Since I can think of no good reason why the user would want to be clicking around on the underlying page while the vomnibar is open, my instinct is to go with 2, but I'm open to discussion.

@smblott-github
Copy link
Collaborator

OK. Thanks for looking into it.

@smblott-github
Copy link
Collaborator

Hi @mrmr1993.

Unfortunately, this doesn't fix #1210. Vomnibar scripts are still executed in the security context of the host page. The issue is to do with the mechanism by which our scripts are injected into the page. See here for details. And that may be why you're seeing:

Blocked a frame with origin "ORIGIN_HERE" from accessing a cross-origin frame.

If the vomnibar page were restructured such that pages/vomnibar.coffee runs as a DOM injected script, then that might fix the issue.

FYI: I migrated your PR here to master, fixed a couple of bugs and fixed the tests. It's here (Edit: And in #1214). And one version of the favicon code, based on your vomnibar/iframe code, is here.

This was referenced Nov 1, 2014
Closed
Closed
@smblott-github
Copy link
Collaborator

Closing in favour of #1214 (which is basically just @mrmr1993's work with fiddles).

@mrmr1993 mrmr1993 deleted the vomnibarToPage branch November 27, 2014 17:59
@smblott-github smblott-github mentioned this pull request Dec 19, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vomnibar: Bookmarklet popups are blocked Current selection wiped out when a vomnibar is opened
4 participants
@mrmr1993 @philc @smblott-github @sharat87