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

type(feat): Implement Bookmark Classes and Tests #327

Merged
merged 4 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions foxpuppet/windows/browser/bookmarks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this file,
# You can obtain one at http://mozilla.org/MPL/2.0/.
"""Contains the bookmarks interaction API and supporting files."""
218 changes: 218 additions & 0 deletions foxpuppet/windows/browser/bookmarks/bookmark.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this file,
# You can obtain one at http://mozilla.org/MPL/2.0/.
"""Contains classes for handling Firefox bookmarks."""

from selenium.webdriver.common.by import By
from selenium.common.exceptions import NoSuchElementException
from selenium.webdriver.remote.webelement import WebElement
from selenium.webdriver.common.keys import Keys
from foxpuppet.windows.browser.navbar import NavBar
from typing import TYPE_CHECKING, Optional, TypedDict, List


class BookmarkData(TypedDict):
"""Bookmark properties."""

name: str
url: str
tags: Optional[List[str]]
keyword: Optional[str]
Comment on lines +14 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏾



class BasicBookmark(NavBar):
"""Handles basic bookmark operations."""

if TYPE_CHECKING:
from foxpuppet.windows.browser.window import BrowserWindow

@staticmethod
def create(window: "BrowserWindow", root: WebElement) -> Optional["BasicBookmark"]:
"""Create a bookmark object.

Args:
window (:py:class:`BrowserWindow`): Window object this bookmark appears in
root (:py:class:`~selenium.webdriver.remote.webelement.WebElement`): WebDriver element object for bookmark

Returns:
:py:class:`BaseBookmark`: Bookmark instance or None
"""
with window.selenium.context(window.selenium.CONTEXT_CHROME):
try:
return BasicBookmark(window, root)
except NoSuchElementException:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious as to why you'd like to return None and not throw the error.

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 might be less verbose. But, now again, it could confusing and unclear. I will just correct that.


@property
def is_bookmarked(self) -> bool:
"""Checks if the current page is bookmarked.

Returns:
bool: True if the page is bookmarked, False otherwise.
"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
star_button_image = self.find_element(BookmarkLocators.STAR_BUTTON_IMAGE)
return star_button_image.get_attribute("starred") == "true"

def add(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference with add() or add_bookmark()?

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 should probably use a flag instead and combine both into one method so it isn't confusing

"""Add a Bookmark using the star button."""
self.click_element(BookmarkLocators.STAR_BUTTON)
self.click_element(BookmarkLocators.FOLDER_MENU)
self.click_element(BookmarkLocators.OTHER_BOOKMARKS_STAR)
self.click_element(BookmarkLocators.SAVE_BUTTON)

def retrieve_bookmark(self, label: str) -> bool:
"""
Check if a bookmark with the given label exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function name reads as if this will return a bookmark but it seems to be more of a check. Can you update the name to reflect that?

Args:
label (str): The name of the bookmark to search for.
"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
self.open_bookmark_menu(
locator_panel=BookmarkLocators.PANEL_MENU,
locator_bookmark=BookmarkLocators.PANEL_BOOKMARK_MENU,
)
panel_bookmarks = self.find_element(BookmarkLocators.PANEL_BOOKMARK_TOOLBAR)
if panel_bookmarks is not None:
menu_items = panel_bookmarks.find_elements(
By.CSS_SELECTOR, "toolbarbutton.bookmark-item"
)
if menu_items is not None:
for item in menu_items:
item_label = item.get_attribute("label")
if item_label and label.lower() in item_label.lower():
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be done with a list comprehension.

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 thought to go with a generator expression here which might better fit, since the intention here is not to create a list, but to evaluate a condition.

return False

def delete(self) -> None:
"""Delete a bookmark using the star button."""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
star_button_image = self.find_element(BookmarkLocators.STAR_BUTTON_IMAGE)
if star_button_image and star_button_image.get_attribute("starred") == "true":
self.click_element(BookmarkLocators.STAR_BUTTON)
self.click_element(BookmarkLocators.REMOVE_BUTTON)


class AdvancedBookmark(BasicBookmark):
"""Handles advanced bookmark operations."""

Copy link
Contributor

Choose a reason for hiding this comment

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

What's an "Advanced Bookmark" exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, so in Desktop Smoke test, a similar term is used so I decided to adopt it in FoxPuppet. Basically, an advanced bookmark is a more detailed way of adding a bookmark, which involves name, url, tags, and keywords, as against just simply using the url alone when creating a Basic Bookmark with the star button.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay let me ask Ben C for clarifications

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@ben-c-at-moz Can you explain what your "Advanced Bookmark" action does exactly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is maybe not the best name--the function here is "advanced" only in that it adds a bookmark through the "Other bookmarks" option in the Library window. The bookmark itself should be normal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree, the name is a little confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I'm fine if we just want to keep using it for more detailed bookmark operations, as long as we explain it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just call it Bookmarks and put all the functionality into one class? If a user wants to do it through the "Other Bookmarks" option, we can just have a flag or something when they create the bookmark.

if TYPE_CHECKING:
from foxpuppet.windows.browser.window import BrowserWindow

@staticmethod
def create(window: "BrowserWindow", root: WebElement) -> Optional["AdvancedBookmark"]:
"""Create an advanced bookmark object.

Args:
window (:py:class:`BrowserWindow`): The window object where the bookmark appears.
root (:py:class:`~selenium.webdriver.remote.webelement.WebElement`): WebElement for the bookmark panel.

Returns:
:py:class:`AdvancedBookmark`: An instance of AdvancedBookmark if successful, otherwise None.
"""
with window.selenium.context(window.selenium.CONTEXT_CHROME):
try:
return AdvancedBookmark(window, root)
except NoSuchElementException:
return None

@property
def is_bookmarked(self) -> bool:
"""Checks if the current page is bookmarked.

Returns:
bool: True if the page is bookmarked, False otherwise.
"""
current_page_title = self.selenium.title
self.open_main_menu(
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX,
locator_menu_bar=BookmarkLocators.MENU_BAR,
)
bookmark_menu = self.find_element(BookmarkLocators.MAIN_MENU_BOOKMARK)
if bookmark_menu is not None:
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK)
with self.selenium.context(self.selenium.CONTEXT_CHROME):
menu_items = bookmark_menu.find_elements(
By.CSS_SELECTOR, "menuitem.bookmark-item"
)
for item in menu_items:
item_label = item.get_attribute("label")
if item_label and item_label.lower() in current_page_title.lower():
return True
return False

def add_bookmark(self, bookmark_data: BookmarkData) -> None:
"""Add a Bookmark using the main bookmark menu."""
self.open_main_menu(
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX,
locator_menu_bar=BookmarkLocators.MENU_BAR,
)
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK)
self.context_click(BookmarkLocators.MANAGE_BOOKMARKS)
self.click_element(BookmarkLocators.ADD_BOOKMARK)
self.switch_to_frame(BookmarkLocators.ADD_BOOKMARK_FRAME)

if bookmark_data["name"]:
self.actions.send_keys(bookmark_data["name"]).perform()
self.actions.send_keys(Keys.TAB).perform()

if bookmark_data["url"]:
self.actions.send_keys(bookmark_data["url"] + Keys.TAB).perform()

tags = bookmark_data["tags"]
if tags is not None:
for tag in tags:
self.actions.send_keys(tag).perform()
self.actions.send_keys(",").perform()
self.actions.send_keys(Keys.TAB).perform()

if bookmark_data.get("keyword"):
keyword = (
bookmark_data["keyword"] if bookmark_data["keyword"] is not None else ""
)
self.actions.send_keys(keyword + Keys.TAB).perform()

self.actions.send_keys(Keys.TAB, Keys.TAB, Keys.TAB, Keys.ENTER).perform()
self.actions.send_keys(Keys.TAB, Keys.ENTER).perform()
self.switch_to_default_context()

def delete_bookmark(self, label: str) -> bool:
"""Delete a bookmark using the main bookmark menu."""
self.open_main_menu(
locator_toolbar=BookmarkLocators.NAVIGATOR_TOOLBOX,
locator_menu_bar=BookmarkLocators.MENU_BAR,
)
bookmark_menu = self.find_element(BookmarkLocators.MAIN_MENU_BOOKMARK)
self.click_element(BookmarkLocators.MAIN_MENU_BOOKMARK)
with self.selenium.context(self.selenium.CONTEXT_CHROME):
menu_item = bookmark_menu.find_element(
By.CSS_SELECTOR, f"menuitem.bookmark-item[label='{label}']"
)
self.actions.context_click(menu_item).perform()
self.click_element(BookmarkLocators.DELETE_MENU_ITEM)
return True


class BookmarkLocators:
ADD_BOOKMARK = (By.ID, "placesContext_new:bookmark")
ADD_BOOKMARK_FRAME = (By.CSS_SELECTOR, "browser[class='dialogFrame']")
BOOKMARK_PROPERTIES_DIALOG = (By.ID, "bookmarkproperties")
DELETE_MENU_ITEM = (By.ID, "placesContext_deleteBookmark")
FOLDER_MENU = (By.ID, "editBMPanel_folderMenuList")
MAIN_MENU_BOOKMARK = (By.ID, "bookmarksMenu")
MANAGE_BOOKMARKS = (By.ID, "bookmarksShowAll")
MENU_BAR = (By.ID, "toggle_toolbar-menubar")
NAME_FIELD = (By.ID, "editBMPanel_namePicker")
NAVIGATOR_TOOLBOX = (By.ID, "navigator-toolbox")
OTHER_BOOKMARKS = (By.ID, "OtherBookmarks")
OTHER_BOOKMARKS_STAR = (By.ID, "editBMPanel_unfiledRootItem")
PANEL_BOOKMARK_MENU = (By.ID, "appMenu-bookmarks-button")
PANEL_BOOKMARK_TOOLBAR = (By.ID, "panelMenu_bookmarksMenu")
PANEL_MENU = (By.ID, "PanelUI-menu-button")
REMOVE_BUTTON = (By.ID, "editBookmarkPanelRemoveButton")
SAVE_BOOKMARK = (By.CSS_SELECTOR, 'button[dlgtype="accept"][label="Save"]')
SAVE_BUTTON = (By.ID, "editBookmarkPanelDoneButton")
STAR_BUTTON = (By.ID, "star-button-box")
STAR_BUTTON_IMAGE = (By.ID, "star-button")
TOOLBAR_CONTEXT_MENU = (By.ID, "toolbar-context-menu")
80 changes: 78 additions & 2 deletions foxpuppet/windows/browser/navbar.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@
"""Creates Navbar object to interact with Firefox Navigation Bar."""

from selenium.webdriver.common.by import By

from selenium.webdriver.remote.webelement import WebElement
from selenium.webdriver.remote.webdriver import WebDriver
from selenium.webdriver.common.action_chains import ActionChains
from selenium.common.exceptions import NoSuchElementException
from foxpuppet.region import Region
from typing import Tuple, Optional
from foxpuppet.windows.base import BaseWindow


class NavBar(Region):
Expand All @@ -20,7 +25,14 @@ class NavBar(Region):

"""

_tracking_protection_shield_locator = (By.ID, "tracking-protection-icon-box")
_tracking_protection_shield_locator: Tuple[str, str] = (
By.ID,
"tracking-protection-icon-box",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not


def __init__(self, window: BaseWindow, root: WebElement) -> None:
super().__init__(window, root)
self.actions: ActionChains = ActionChains(self.selenium)

@property
def is_tracking_shield_displayed(self) -> bool:
Expand All @@ -36,3 +48,67 @@ def is_tracking_shield_displayed(self) -> bool:
return el.get_attribute("active") is not None
el = self.root.find_element(By.ID, "tracking-protection-icon")
return bool(el.get_attribute("state"))

def click_element(self, locator: Tuple[str, str]) -> None:
"""Click on an element by its locator."""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
self.find_element(locator).click()

def find_element(self, locator: Tuple[str, str]) -> WebElement:
"""Find and return a web element by its locator."""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
try:
element = self.root.find_element(*locator)
return element
except Exception as e:
raise NoSuchElementException(
f"Error locating element with locator {locator}: {e}"
)

def context_click(self, locator: Tuple[str, str]) -> None:
"""
Perform a right-click (context-click) on an element.

Args:
locator (tuple): Locator for the element to right-click.
"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
element = self.find_element(locator)
self.actions.context_click(element).perform()

def switch_to_frame(self, locator: Tuple[str, str]) -> None:
"""
Switch to an iFrame using its locator

Args:
locator (tuple): Locator for the iFrame elemeent.
"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
frame_element = self.find_element(locator)
if frame_element is not None:
self.selenium.switch_to.frame(frame_element)
else:
raise NoSuchElementException(f"iFrame with locator {locator} not found.")

def switch_to_default_context(self) -> None:
"""Switch back to default context."""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
self.selenium.switch_to.default_content()

def open_main_menu(
self, locator_toolbar: Tuple[str, str], locator_menu_bar: Tuple[str, str]
) -> None:
"""Activate main menu bar"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
self.context_click(locator_toolbar)
self.click_element(locator_menu_bar)

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of scope for this PR no? Maybe I am missing something

Copy link
Collaborator Author

@Temidayo32 Temidayo32 Dec 18, 2024

Choose a reason for hiding this comment

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

These methods are used in Bookmarks. I added them here because the issue stated to extend the NavBar with Bookmark functionalities. But then, along the way, I realized Bookmarks doesn't exactly fit with the nav-bar element. While some bookmark web elements exist in the nav bar, others are scattered across different parts of the dom. I could move them all to Bookmark itself if that makes things clearer.

Add functionality to NavBar (and elsewhere) to set, unset, and retrieve bookmarks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well some of these just seem like generic actions, like switch_to_default_context() and find_element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will you suggest I move them to a class higher up the hierarchy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I moved all the generic actions into region.py

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be removed if they aren't specifically related to bookmark functionality. We can add these later if need be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, these methods are functionalities I reused a lot in the Bookmark functionality. So instead of repeating them, I just factored them out into those methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand but a lot of them are doing what selenium has built in. Selenium has a find_element method already, as an example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@b4handjr I have refactored and removed all of them. It is now ready for another review.

def open_bookmark_menu(
self, locator_panel: Tuple[str, str], locator_bookmark: Tuple[str, str]
) -> None:
"""
Opens the Bookmarks menu in Panel UI
"""
with self.selenium.context(self.selenium.CONTEXT_CHROME):
self.click_element(locator_panel)
self.click_element(locator_bookmark)
Loading
Loading