Skip to content

Conversation

b4handjr
Copy link
Contributor

This should allow users to interact with an extension after they install it by simply knowing the name.

Since webextensions can not change the URL of the current page, FoxPuppet with switch to the new tab that gets opened once the extension is clicked. If no new tab is opened the current tab will still be selected for interaction.

I also moved some of the notification fixtures from the test_notifications.py file to the conftest.py file as I use them for this new test.

This should allow users to interact with an extension after they
install it by simply knowing the name.

Since webextensions can not change the URL of the current page,
FoxPuppet with switch to the new tab that gets opened once the
extension is clicked. If no new tab is opened the current tab
will still be selected for interaction.
@b4handjr b4handjr requested a review from davehunt November 28, 2017 19:51
@coveralls
Copy link

coveralls commented Nov 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 923ec38 on jrbenny35:add-extension-selection into 195402f on mozilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 923ec38 on jrbenny35:add-extension-selection into 195402f on mozilla:master.

@b4handjr
Copy link
Contributor Author

I have no idea why the flake8 build is failing. Any ideas?

@davehunt
Copy link
Member

I have no idea why the flake8 build is failing. Any ideas?

It looks like flake8 is now checking files in the .eggs directory. I'm not sure why that's changed, but we could probably safely add this to the list of ignored paths.

Copy link
Member

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

I don't see a compelling argument for this patch. If you want to interact with an extension button on the nav bar you can currently do this outside of FoxPuppet, where you can also cater for any consequences from clicking the button. This seems too restrictive to me, in that clicking a button must cause a non-specific change in the window handles, we switch to the last window handle (these are not guaranteed to be sorted in any particular order), and we assume the current URL before the click is 'about:blank'.

@b4handjr
Copy link
Contributor Author

b4handjr commented Nov 29, 2017

I don't see a compelling argument for this patch. If you want to interact with an extension button on the nav bar you can currently do this outside of FoxPuppet, where you can also cater for any consequences from clicking the button. This seems too restrictive to me, in that clicking a button must cause a non-specific change in the window handles, we switch to the last window handle (these are not guaranteed to be sorted in any particular order), and we assume the current URL before the click is 'about:blank'.

This is true to an extent. If an addon/extension has a URL it wants to navigate to it MUST do it within another tab. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create

Now 'about:blank' is the default URL that get's opened when a new tab is opened. Maybe it is too much to assume a new tab will ALWAYS be opened when you click on an extension. It would be better to check if a new tab is opened and if so, check that the URL changes.

This seems too restrictive to me, in that clicking a button must cause a non-specific change in the window handles

I could change the click to not switch tabs and the user must do it manually.

@davehunt
Copy link
Member

This is true to an extent. If an addon/extension has a URL it wants to navigate to it MUST do it within another tab.

But not all extensions will navigate to a URL when the button is clicked (think of the popups in Snooze Tabs or Containers).

I could change the click to not switch tabs and the user must do it manually.

By manually you mean external to FoxPuppet? This would only leave locating/clicking of the button, which is also trivial to do outside of FoxPuppet.

I'd be okay adding a helper function for opening a tab in FoxPuppet, which could take a function as an argument. This helper would call the function and wait for the new tab to be opened, and switch to it. I can see this being useful in a few places.

@b4handjr
Copy link
Contributor Author

By manually you mean external to FoxPuppet? This would only leave locating/clicking of the button, which is also trivial to do outside of FoxPuppet.

Yes I could add a switch_to_tab or something like that to the Extension object.

I'd be okay adding a helper function for opening a tab in FoxPuppet, which could take a function as an argument. This helper would call the function and wait for the new tab to be opened, and switch to it. I can see this being useful in a few places.

I agree we need a context manager or helper function of some sort for opening a new tab.

@whimboo
Copy link
Collaborator

whimboo commented Nov 29, 2017

I feel we are in a loop here, given that we have discussed this already on another PR. So grab what Puppeteer is using.

@davehunt
Copy link
Member

I feel we are in a loop here, given that we have discussed this already on another PR. So grab what Puppeteer is using.

Sounds good to me. Let's close this pull request, and shift efforts to a helper for opening a tab. This could be added to #126, which can be reduced to just opening a tab via the new tab button, dropping the close/select tab parts for now.

@davehunt davehunt closed this Nov 30, 2017
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.

4 participants