Added tabbar and tab representation #126

b4handjr · 2017-09-28T23:27:08Z

This is a matured version of PR #69.

Solves #1, #2, #3, #5, #6

coveralls · 2017-09-28T23:33:28Z

Coverage remained the same at 100.0% when pulling 6ba3614 on jrbenny35:add-tab-and-tabbar into 2c6417c on mozilla:master.

coveralls · 2017-09-28T23:33:28Z

Coverage remained the same at 100.0% when pulling 6ba3614 on jrbenny35:add-tab-and-tabbar into 2c6417c on mozilla:master.

davehunt · 2017-10-02T10:49:08Z

foxpuppet/windows/browser/tab_bar.py

+from foxpuppet.region import Region
+
+
+class Tab_bar(Region):


We should use CamelCase naming convention (TabBar) as prescribed in pep8

davehunt · 2017-10-02T10:51:28Z

foxpuppet/windows/browser/tab_bar.py

+            tabs = [self.Tab(self, el) for el in
+                    self.selenium.find_elements(*self._tabs_locator)]
+        # Assign handles
+        for tab, handle in zip(tabs, self.selenium.window_handles):


Can we be sure that the tab elements and window handles will be returned in the same order? What if there are two windows, each with multiple tabs?

I'll look into this. I was under the impression that it will be in order of tab opened since it is a list.

Now thinking about the multiple window situation...technically, geckodriver would see it as the same session since you can have multiple windows per profile already. I don't know if there is a way to differentiate tabs from windows as far as window_handles goes. I think they are all in one.

Its a little tricky because do we assume that a user testing this will only open one window? If there is no way to tell a window and a tab apart, unless we create a foxpuppet tracking object that has a dict object and can tell the user which is which depending on when they opened it.

do we assume that a user testing this will only open one window?

I wouldn't make this assumption. We're providing them with an API for opening new windows and now tabs.

There is no order of window handles at all. Also note that window handles can change right now given if the tab is moved to a different content process (like remoteness change).

Chrome windows of Firefox are handled via chrome_window_handles but not sure if we pass those through geckodriver yet.

Going along with whimboo, it seems as if it doesn't matter if you open a window or a tab, selenium sees that as a window_handle.

We could either keep track of each event with a dict but would that be necessary? It seems as if we need an easy way to differentiate between a chrome window handle and a "Normal" window_handle.

If you are going to open a new tab, you are interested in that tab aka window_handle and not the window aka chrome_window_handle. Also it doesn't matter for the test in which window this tab is located in.

davehunt · 2017-10-02T10:56:23Z

foxpuppet/windows/browser/tab_bar.py

+        with self.selenium.context(self.selenium.CONTEXT_CHROME):
+            self.selenium.find_element(*self._new_tab_button_locator).click()
+        self.wait.until(
+            lambda _: len(self.selenium.window_handles) != current_tabs)


We should wait for there to be one additional tab open. This would also return true if a window/tab closed, or several new windows/tabs opened.

davehunt · 2017-10-02T10:58:40Z

foxpuppet/windows/browser/tab_bar.py

+                    'anonid', 'close-button')
+                button.click()
+            self.wait.until(
+                lambda _: len(self.selenium.window_handles) != current_tabs)


Again, we should narrow this condition to mean this tab is closed. We could wait for the current handle to be removed, rather than wait for the count of handles to change. Perhaps @whimboo has other suggestions.

This makes sense.

I have nothing additional to add here.

davehunt · 2017-10-02T10:59:04Z

foxpuppet/windows/browser/tab_bar.py

+
+            """
+            self.selenium.switch_to.window(self.handle)
+            return self


Why return the tab?

If you select or 'Focus' that tab, you should be able to get info about it. i.e.: name, close button, etc.

You already have a reference to the tab though, in order to call this method.

The reference before does not switch selenium over to the tab.

That should only affect content though? Is the tab region itself not possible to interact with without switching the window?

You are correct. Should we focus the tab when we open it?

I could follow firefox puppeteer and check if it is selected and if so switch to it.

I believe the default behaviour of Firefox is to select the new tab, however I believe it's possible to change this via a preference.

Yes so firefox will focus it, but selenium won't it seems.

Selenium requires an explicit update of the current window handle. As long as you don't do that the browsing context will remain in the opener tab.

davehunt · 2017-10-02T11:07:41Z

foxpuppet/windows/browser/tab_bar.py

+            self.selenium.find_element(*self._new_tab_button_locator).click()
+        self.wait.until(
+            lambda _: len(self.selenium.window_handles) != current_tabs)
+        return self.tabs


Why return the list of tabs? It would make more sense to return the new tab so we could do something like:

new_tab = tab_bar.open_new_tab() new_tab.close()

It would all make more sense if code or the technique which we have already written for Firefox Puppet would be taken. It said this already a couple of times, and here is one more example. It would help us to not have such a lot of review cycles.

Currently in Firefox Puppeteer:

Wait(self.marionette).until( lambda mn: len(mn.window_handles) == len(start_handles) + 1, message='No new tab has been opened.') handles = self.marionette.window_handles [new_handle] = list(set(handles) - set(start_handles)) [new_tab] = [tab for tab in self.tabs if tab.handle == new_handle] # if the new tab is the currently selected tab, switch to it if new_tab == self.selected_tab: new_tab.switch_to()

Which is what Dave is suggesting anyways. I will model this a little closer.

davehunt · 2017-10-02T11:11:22Z

tests/test_browser_model.py

+    """Test close Tab."""
+    browser.tab_bar.open_new_tab()
+    browser.tab_bar.tabs[1].close()
+    assert len(browser.tab_bar.tabs) == 1


We could also ensure the handle for the new tab is not in the list of handles.

davehunt · 2017-10-02T11:12:13Z

tests/test_browser_model.py

+def test_close_tab(browser, selenium):
+    """Test close Tab."""
+    browser.tab_bar.open_new_tab()
+    browser.tab_bar.tabs[1].close()


If open_new_tab returned the tab region, then we could simplify this to:

tab = browser.tab_bar.open_new_tab() tab.close()

davehunt · 2017-10-02T11:14:51Z

foxpuppet/windows/browser/tab_bar.py

+            Returns: :py:class:`Tab`.
+
+            """
+            self.selenium.switch_to.window(self.handle)


This won't show the tab if is not the current tab. In this case, we would want to click on the tab. It's also possible that the tab is scrolled out of view on the tab bar, so we'd need to bring it into view before clicking it.

Do we really need to bring it into view if selenium knows where it is? I mean technically it is there, its not hidden or anything.

I don't know if you'll be able to click on an element that's not in view. We're simulating a user, and a user can't switch to a tab without being able to click it.

Note that Marionette currently doesn't know about tabs which haven't been restored via sessionstore. Those don't have a browser, and as such will not be returned.

davehunt · 2017-10-02T11:18:06Z

tests/test_browser_model.py

+    tabs = browser.tab_bar.open_new_tab()
+    tabs[-1].select()
+    assert 'about:newtab' in selenium.current_url
+    tabs[0].select()


This makes me feel like we should have a way to access the currently displayed tab, but that can be a later enhancement.

What do you mean? Selenium has to switch_to that tab even though firefox switches to it automatically.

I mean like browser.tab_bar.current_tab or an attribute on the tab to indicate if it's the currently selected tab. This would allow you to store the currently selected tab, open a tab, close the new tab, and assert that we're back to the original tab.

b4handjr · 2017-10-02T17:54:49Z

@m8ttyB This is the PR.

b4handjr · 2017-10-02T20:07:15Z

I found this for making sure the handle is not a window:

@staticmethod
    def get_handle_for_tab(marionette, tab_element):
        """Retrieves the marionette handle for the given :class:`Tab` instance.

        :param marionette: An instance of the Marionette client.

        :param tab_element: The DOM element corresponding to a tab inside the tabs toolbar.

        :returns: `handle` of the tab.
        """
        # TODO: This introduces coupling with marionette's window handles
        # implementation. To avoid this, the capacity to get the XUL
        # element corresponding to the active window according to
        # marionette or a similar ability should be added to marionette.
        handle = marionette.execute_script("""
          let win = arguments[0].linkedBrowser;
          if (!win) {
            return null;
          }
          return win.outerWindowID.toString();
        """, script_args=[tab_element])

        return handle

Thoughts? I see there is a TODO on that function. I am thinking we modify window_manager to have it tell us which window_handle is a tab, or a actual window.

davehunt · 2017-10-03T09:53:23Z

@jrbenny35 you've mentioned this is blocking some other work. I'm wondering if we can reduce the scope of this so whatever it's blocking can be unblocked. Do you have details of what's required by the blocked work?

b4handjr · 2017-10-03T16:11:34Z

@jrbenny35 you've mentioned this is blocking some other work. I'm wondering if we can reduce the scope of this so whatever it's blocking can be unblocked. Do you have details of what's required by the blocked work?

@davehunt yeah this is for testing lockbox. I need to install the extension, select the tab the extension opens and then interact with what is on that page. I can use what I have now to get it done but we should still hash this out and find a good solution.

I've made a lot of changes that bring it more in line with puppeteer but the question of what handles are mapped to what windows is still what I am working on.

b4handjr · 2017-10-03T16:48:57Z

Why would we need to know what window handle is a window vs a tab. If the user opened a new window that has a tab, they could still interact with the window. i.e.: refresh/back/forward etc.

If the tab is in another window, firefox will switch to it and so will selenium.

Technically each tab can be its own 'window' as well.

whimboo · 2019-01-14T16:23:23Z

I assume we can close this PR given that it didn't receive any update for more than a year.

Added tabbar and tab representation

6ba3614

b4handjr requested review from davehunt and whimboo September 28, 2017 23:27

davehunt requested changes Oct 2, 2017

View reviewed changes

b4handjr added 2 commits October 4, 2017 11:24

Updated files based on review

009133d

Fix flake8 errors

1055740

davehunt mentioned this pull request Nov 30, 2017

Added the option to click on an installed extension from the navbar. #133

Closed

whimboo closed this Jan 14, 2019

Added tabbar and tab representation #126

Added tabbar and tab representation #126

Uh oh!

Conversation

b4handjr commented Sep 28, 2017

Uh oh!

coveralls commented Sep 28, 2017

Uh oh!

coveralls commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b4handjr Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b4handjr Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b4handjr commented Oct 2, 2017

coveralls commented Sep 28, 2017 •

edited

Loading

b4handjr Oct 2, 2017 •

edited

Loading

b4handjr Oct 3, 2017 •

edited

Loading

b4handjr commented Oct 3, 2017 •

edited

Loading

b4handjr commented Oct 3, 2017 •

edited

Loading