Skip to content

fix: support wildcard and type-variable list generics in page factory decorators - #2494

Open
hoanle0126 wants to merge 3 commits into
appium:masterfrom
hoanle0126:fix/wildcard-list-decoration-1257
Open

hoanle0126 wants to merge 3 commits into
appium:masterfrom
hoanle0126:fix/wildcard-list-decoration-1257

Conversation

@hoanle0126

@hoanle0126 hoanle0126 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Proposed changes

Closes #1257

When page objects are written in Kotlin, read-only list properties (val items: List<WebElement> or val widgets: List<MyWidget>) are compiled to Java bytecode with upper-bounded wildcard generics (List<? extends WebElement>, List<? extends MyWidget>) because Kotlin's List<out T> interface is declaration-site covariant. Previously, both DefaultFieldDecorator and AppiumFieldDecorator required the parameterized type argument to be a direct class reference, leaving wildcard-typed element and widget lists undecorated.

This PR updates the page factory decorators and locator builders to:

  • Extract list element type resolution into DefaultFieldDecorator.resolveListElementClasses and single-field type resolution into DefaultFieldDecorator.resolveFieldClasses (protected static), unwrapping direct classes, nested parameterized types, upper- and lower-bounded wildcards, and type variables (resolving concrete type arguments declared on parameterized generic page superclasses first and retaining the type variable's declared upper bounds as fallback when a concrete element subtype is not an exact match in AVAILABLE_ELEMENT_CLASSES).
  • Intentionally support lower-bounded wildcard lists (? super WebElement and ? super Widget, corresponding to Kotlin's contravariant mutable lists) in addition to upper-bounded wildcards, unlike upstream Selenium's decorator which only inspects upper bounds.
  • Resolve both single widget fields and list widget fields by passing the runtime page class and resolved widget class explicitly through the locator factory and element/widget builders so class-level locator, timeout, locator strategy, and widget override annotations on the resolved widget type are honored.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

@mykola-mokhnach mykola-mokhnach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • AppiumFieldDecorator.java:144: for a list typed with a type variable or wildcard, the widget class resolves to the declared upper bound, not the type the page is parameterised with. With GenericPage<W extends SampleWidget> instantiated as GenericPage<FancyWidget>, extractWidgetClass returns SampleWidget, so the proxy holds SampleWidget instances. for (FancyWidget w : page.typeVarWidgets) then throws ClassCastException. Any @FindBy, @HowToUseLocators, @WithTimeout or @OverrideWidget on FancyWidget is silently ignored. Could the field's declaring class and the page instance's runtime type be used to resolve the concrete argument? If the bound is intended, it should be documented.
  • AppiumFieldDecorator.java:117: lower-bounded wildcards (List<? super WebElement>, Kotlin MutableList<in WebElement>) are skipped on purpose and asserted in tests. The field stays undecorated and null, so the first use throws an NPE with no hint why. A List<WebElement> proxy would be a valid value for it. Is skipping intentional, given the goal of supporting wildcard lists? If so, consider failing fast or logging.
  • AppiumFieldDecorator.java:106: the visited set in collectElementClasses (and in isWebElementType) guards against cycles that can't occur. Bounds aren't walked through parameterized type arguments, and Java forbids T extends U, U extends T. It adds a HashSet allocation and an extra parameter in each recursion for no benefit.
  • AppiumFieldDecorator.java:77: generic resolution runs per field in isDecoratableList, decorateWidget, findHowToUseLocators, findTimeout and WidgetByBuilder. Each call re-runs resolveListElementClasses, allocating a list and a set and doing reflection, so a page with many list fields walks the type tree about 4-5 times per field in initElements. Resolving once in decorate and passing the result down would avoid that.

@hoanle0126

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @mykola-mokhnach! I have updated the PR in 6549e10 to address all of your feedback:

  1. Concrete type argument resolution on parameterized generic pages (AppiumFieldDecorator / DefaultFieldDecorator):
    • DefaultFieldDecorator.resolveListElementClasses(Field) now resolves TypeVariable<?> declarations on generic superclasses by walking the superclass hierarchy from PageFactory.getCurrentPageClass() up to the declaring class (so class ConcreteGenericPage extends GenericPage<AppiumWebElement, FancyWidget> resolves W to FancyWidget, preserving FancyWidget instantiation and its @FindBy, @HowToUseLocators, @WithTimeout, and @OverrideWidget annotations).
    • When a generic page is instantiated directly without a subclass (new GenericPage<>()), where runtime type arguments are erased, resolution falls back to the type variable's declared upper bounds (documented in Javadoc).
  2. Lower-bounded wildcards (List<? super WebElement>, List<? super SampleWidget>):
    • When wildcardType.getLowerBounds() is non-empty, the type walker now inspects the lower bounds instead of skipping them, so List<? super WebElement> (Kotlin MutableList<in WebElement>) and List<? super SampleWidget> are decorated with List<WebElement> / List<SampleWidget> proxies while List<?> and List<? super Object> remain undecorated.
  3. Removed unnecessary visited set & deduplicated the type walker:
    • Removed the Set<Type> visited (HashSet) parameter and allocation, and consolidated the walker into DefaultFieldDecorator.resolveListElementClasses(Field) so both DefaultFieldDecorator and AppiumFieldDecorator share a single implementation.
  4. Single per-field resolution in AppiumFieldDecorator.decorate:
    • AppiumFieldDecorator.decorate(ClassLoader, Field) now resolves listElementClasses and widgetClass once at the start of decorate, passes widgetClass directly into decorateWidget, and caches the active FieldDecorationContext for the duration of decorate so isDecoratableList, findHowToUseLocators, resolveCustomDuration, and WidgetByBuilder reuse the resolved result without re-walking reflection.
  5. Class-level @FindBy / @FindBys / @FindAll in DefaultElementByBuilder & AbstractFindByBuilder:
    • Marked @Nullable Field field on AbstractFindByBuilder.buildIt (and FindBy, FindBys, FindAll builders), updated DefaultElementByBuilder.buildBy() to also avoid assuming annotatedElement is always a Field, documented this in the PR description, and added direct tests for class-level @FindBy, @FindBys, and @FindAll on Widget classes.
  6. Split AppiumFieldDecoratorTest & documented FoundElement.getWrappedDriver():
    • Split the test into focused methods (decoratesWildcardElementLists, decoratesWildcardWidgetListsAndHonorsWidgetAnnotations, doesNotDecorateUnboundedOrNonElementWildcardLists, and resolvesConcreteTypeArgumentsOnParameterizedGenericPageSubclasses) verifying @FindBy/@FindBys/@FindAll, @HowToUseLocators, @WithTimeout, and @OverrideWidget across both wildcard and generic subtype bounds.
    • Added a comment on FoundElement explaining why getWrappedDriver() is needed (when WidgetListInterceptor / WidgetInterceptor initializes a Widget, WebDriverUnpackUtility.getCurrentContentType(element) and nested AppiumFieldDecorator unpack the parent WebDriver from the root WebElement via WrapsDriver.getWrappedDriver()).

@mykola-mokhnach mykola-mokhnach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at 6549e10. The earlier threads are addressed. A regression for resolved type variables and a few semantic questions remain, inline below.

@hoanle0126

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up review @mykola-mokhnach! Updated in b38fcca:

  1. Type-variable fallback to declared bounds (DefaultFieldDecorator.collectElementClasses): When resolving a TypeVariable<?> against pageClass, we now collect the resolved concrete type argument first (so widget subclass resolution keeps the most specific type) and also collect the type variable's declared upper bounds afterward. That preserves decoration for Base<T extends WebElement> subclassed with concrete WebElement subtypes (such as RemoteWebElement) that are not directly listed in AVAILABLE_ELEMENT_CLASSES.
  2. Lower-bounded wildcards (? super): Kept intentional support for List<? super WebElement> and List<? super Widget> (matching Kotlin's MutableList<in T>) and noted this difference relative to upstream Selenium's DefaultFieldDecorator in the PR description.
  3. Explicit pageClass / widgetClass passing & protected static visibility: Changed DefaultFieldDecorator.resolveListElementClasses and DefaultFieldDecorator.resolveFieldClasses to protected static taking (Field, @Nullable Class<?> pageClass), removed the CURRENT_FIELD_CONTEXT ThreadLocal from AppiumFieldDecorator, and passed pageClass and widgetClass explicitly through AppiumElementLocatorFactory, DefaultElementByBuilder, and WidgetByBuilder.
  4. Single type-variable fields (W single;): Updated AppiumFieldDecorator.resolveWidgetClass and DefaultFieldDecorator to resolve non-list type-variable fields against pageClass via resolveFieldClasses(field, pageClass), and added test coverage in AppiumFieldDecoratorTest and PageFactoryTest.

@mykola-mokhnach mykola-mokhnach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

This branch has not been deployed

No deployments
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.

List element annotation unsupported for kotlin

2 participants