Skip to content

Improve mobile element highlighting and rectangle accuracy #4480

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

Merged
ravirk91 merged 1 commit into from
Apr 7, 2026
Merged

Improve mobile element highlighting and rectangle accuracy #4480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion Ginger/Ginger/ApplicationModelsLib/POMModels/PomAllElementsPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,12 @@ public void FocusSpyItemOnElementsGrid()

if (matchingOriginalElement == null)
{
mWinExplorer.LearnElementInfoDetails(mSpyElement);
var learnedElement = mWinExplorer.LearnElementInfoDetails(mSpyElement);
if (learnedElement != null)
{
mSpyElement = learnedElement;
mSpyElement.WindowExplorer = mWinExplorer;
}
matchingOriginalElement = mWinExplorer.GetMatchingElement(mSpyElement, mPOM.GetUnifiedElementsList());
}

Expand Down
9 changes: 7 additions & 2 deletions Ginger/Ginger/AutomatePageLib/AddActionMenu/LiveSpy/LiveSpyPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,13 @@ private void SpyTimerHandler(object sender, EventArgs e)
mWindowExplorerDriver.UnHighLightElements();
mSpyElement.WindowExplorer = mWindowExplorerDriver;

// Learn details (fills properties/locators)
xWindowSelectionUC.mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
// Learn details (fills properties/locators) and keep enriched instance if returned
var learnedElement = xWindowSelectionUC.mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 6, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Use the active driver field for element learning to avoid reference drift.

Line 269 calls LearnElementInfoDetails via xWindowSelectionUC.mWindowExplorerDriver while this flow is guarded by mWindowExplorerDriver. Prefer a single source of truth here to avoid null/out-of-sync edge failures.

Suggested change 
- var learnedElement = xWindowSelectionUC.mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
+ var learnedElement = mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var learnedElement = xWindowSelectionUC.mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
var learnedElement = mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Ginger/Ginger/AutomatePageLib/AddActionMenu/LiveSpy/LiveSpyPage.xaml.cs` at
line 269, The code calls LearnElementInfoDetails on
xWindowSelectionUC.mWindowExplorerDriver causing potential reference drift;
replace that call to use the active field mWindowExplorerDriver instead (i.e.,
call mWindowExplorerDriver.LearnElementInfoDetails(mSpyElement)) so the single
source of truth is used; ensure the existing null/guard check around
mWindowExplorerDriver remains in place before invoking LearnElementInfoDetails.

if (learnedElement != null)
{
mSpyElement = learnedElement;
mSpyElement.WindowExplorer = mWindowExplorerDriver;
}

// Ensure element location/size fields are updated (important for mobile highlighting)
try
Expand Down
62 changes: 43 additions & 19 deletions Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ limitations under the License.
using Amdocs.Ginger.Common.Repository.ApplicationModelLib.POMModelLib;
using Amdocs.Ginger.Common.UIElement;
using Amdocs.Ginger.CoreNET;
using Amdocs.Ginger.CoreNET.Drivers.CoreDrivers.Mobile;
using Ginger.Actions.Locators.ASCF;
using Ginger.Actions.UserControls;
using Ginger.BusinessFlowsLibNew.AddActionMenu;
Expand Down Expand Up @@ -1441,7 +1442,7 @@ private async Task HighlightSelectedElement(System.Drawing.Point ClickedPoint)
}

// Draw rectangle in screenshot view using the element's (now fresh) bounds
DrawElementRectangleAsync(inspectElementInfo);
DrawElementRectangleAsync(inspectElementInfo, highlightInApp: false);

// Keep persistent highlighted element reference for UI flows
currentHighlightedElement = inspectElementInfo;
Expand Down Expand Up @@ -1482,13 +1483,33 @@ private async Task HighlightSelectedElement(System.Drawing.Point ClickedPoint)
}
}

private void DrawElementRectangleAsync(ElementInfo clickedElementInfo)
private void DrawElementRectangleAsync(ElementInfo clickedElementInfo, bool highlightInApp = true)
{
try
{
// remove previous rectangle
RemoveElemntRectangle();

// For some Android paths the element is identified but bounds are not populated yet.
// Refresh once before rectangle math; if still invalid, skip drawing a misleading rectangle.
if (clickedElementInfo.Width <= 0 || clickedElementInfo.Height <= 0)
{
try
{
mWindowExplorerDriver?.UpdateElementInfoFields(clickedElementInfo);
}
catch (Exception exUpd)
{
Reporter.ToLog(eLogLevel.DEBUG, "DrawElementRectangleAsync: UpdateElementInfoFields failed before rectangle draw: " + exUpd.Message, exUpd);
}
Comment on lines +1501 to +1504
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use ERROR level in these exception paths.

Line 1503, Line 1637, and Line 1644 log caught failures with DEBUG. These are error paths and should use eLogLevel.ERROR for consistency and alerting fidelity.

Suggested change 
- Reporter.ToLog(eLogLevel.DEBUG, "DrawElementRectangleAsync: UpdateElementInfoFields failed before rectangle draw: " + exUpd.Message, exUpd);
+ Reporter.ToLog(eLogLevel.ERROR, "DrawElementRectangleAsync: UpdateElementInfoFields failed before rectangle draw: " + exUpd.Message, exUpd);

- Reporter.ToLog(eLogLevel.DEBUG, "Driver HighLightElement failed", ex);
+ Reporter.ToLog(eLogLevel.ERROR, "Driver HighLightElement failed", ex);

- Reporter.ToLog(eLogLevel.DEBUG, "Request to driver highlight element failed", ex);
+ Reporter.ToLog(eLogLevel.ERROR, "Request to driver highlight element failed", ex);

As per coding guidelines: **/*.cs: Use Reporter.ToLog(eLogLevel.ERROR, message) for logging errors.

Also applies to: 1635-1644

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs`
around lines 1501 - 1504, The catch blocks currently logging exceptions with
Reporter.ToLog at eLogLevel.DEBUG are error paths and should be changed to
eLogLevel.ERROR; update the Reporter.ToLog calls inside
DrawElementRectangleAsync (the catch around UpdateElementInfoFields) and the
other two catch blocks referenced (around lines 1635–1644) to use
eLogLevel.ERROR instead of eLogLevel.DEBUG so failures are logged as errors;
keep the same message and exception parameter when replacing the log level for
Reporter.ToLog.


if (clickedElementInfo.Width <= 0 || clickedElementInfo.Height <= 0)
{
Reporter.ToLog(eLogLevel.DEBUG, "DrawElementRectangleAsync: skipping rectangle draw due to invalid element bounds.");
return;
}
}

// element bounds in source image/device coordinates
System.Drawing.Point ElementStartPoint = new System.Drawing.Point(clickedElementInfo.X, clickedElementInfo.Y);
System.Drawing.Point ElementMaxPoint = new System.Drawing.Point(clickedElementInfo.X + clickedElementInfo.Width, clickedElementInfo.Y + clickedElementInfo.Height);
Expand Down Expand Up @@ -1597,28 +1618,31 @@ private void DrawElementRectangleAsync(ElementInfo clickedElementInfo)
mScreenShotViewPage.xHighlighterBorder.Visibility = Visibility.Visible;
}), System.Windows.Threading.DispatcherPriority.Render);

// ask driver to highlight in the actual app as well (non-blocking)
try
if (highlightInApp)
{
var drv = mWindowExplorerDriver;
if (drv != null)
// ask driver to highlight in the actual app as well (non-blocking)
try
{
System.Threading.Tasks.Task.Run(() =>
var drv = mWindowExplorerDriver;
if (drv != null)
{
try
{
drv.HighLightElement(clickedElementInfo, locateElementByItLocators: true);
}
catch (Exception ex)
System.Threading.Tasks.Task.Run(() =>
{
Reporter.ToLog(eLogLevel.DEBUG, "Driver HighLightElement failed", ex);
}
});
try
{
drv.HighLightElement(clickedElementInfo, locateElementByItLocators: true);
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.DEBUG, "Driver HighLightElement failed", ex);
}
});
}
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.DEBUG, "Request to driver highlight element failed", ex);
}
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.DEBUG, "Request to driver highlight element failed", ex);
}
}
catch (Exception ex)
Expand Down
27 changes: 21 additions & 6 deletions Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1794,20 +1794,24 @@ private void EnsureDeviceBounds(ElementInfo ei)
{
if (ei == null) return;

// If already have bounds property or width/height set - assume ok
if ((ei.Properties != null && ei.Properties.Any(p => string.Equals(p.Name, "bounds", StringComparison.InvariantCultureIgnoreCase)))
|| (ei.Width > 0 && ei.Height > 0))
// If we already have concrete size, nothing to do.
if (ei.Width > 0 && ei.Height > 0)
{
return;
}

// Try attribute "bounds" (common for Android UiAutomator)
string boundsAttr = null;
string boundsAttr = ei.Properties?
.FirstOrDefault(p => string.Equals(p.Name, "bounds", StringComparison.InvariantCultureIgnoreCase))
?.Value;
if (ei.ElementObject is IWebElement we)
{
try
{
boundsAttr = we.GetAttribute("bounds");
if (string.IsNullOrEmpty(boundsAttr))
{
boundsAttr = we.GetAttribute("bounds");
}
Comment on lines +1804 to +1814
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Refresh bounds from the live element, not the first cached property.

This now prefers ei.Properties["bounds"], but EnsureDeviceBounds() also writes "bounds" back into ei.Properties. On a reused ElementInfo, FirstOrDefault() keeps returning the oldest rectangle, and GetLocationAndSizeOfElement() also keeps the first duplicate, so the highlight can stay stale after a refresh.

💡 Suggested fix 
-                string boundsAttr = ei.Properties?
-                    .FirstOrDefault(p => string.Equals(p.Name, "bounds", StringComparison.InvariantCultureIgnoreCase))
-                    ?.Value;
+                string boundsAttr = null;
                 if (ei.ElementObject is IWebElement we)
                 {
                     try
                     {
-                        if (string.IsNullOrEmpty(boundsAttr))
-                        {
-                            boundsAttr = we.GetAttribute("bounds");
-                        }
+                        boundsAttr = we.GetAttribute("bounds");
                     }
                     catch { boundsAttr = null; }
                 }
+
+                if (string.IsNullOrEmpty(boundsAttr))
+                {
+                    boundsAttr = ei.Properties?
+                        .LastOrDefault(p => string.Equals(p.Name, "bounds", StringComparison.InvariantCultureIgnoreCase))
+                        ?.Value;
+                }
-                        if (ei.Properties == null) ei.Properties = new ObservableList<ControlProperty>();
-                        ei.Properties.Add(new ControlProperty() { Name = "bounds", Value = $"[{l},{t}][{r},{b}]" });
+                        if (ei.Properties == null) ei.Properties = new ObservableList<ControlProperty>();
+                        var existingBounds = ei.Properties
+                            .FirstOrDefault(p => string.Equals(p.Name, "bounds", StringComparison.InvariantCultureIgnoreCase));
+                        if (existingBounds != null)
+                        {
+                            existingBounds.Value = $"[{l},{t}][{r},{b}]";
+                        }
+                        else
+                        {
+                            ei.Properties.Add(new ControlProperty() { Name = "bounds", Value = $"[{l},{t}][{r},{b}]" });
+                        }

Also applies to: 1835-1837

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs`
around lines 1804 - 1814, The code is using ei.Properties.FirstOrDefault(...) to
read a cached "bounds" which can be stale; change the logic in
GenericAppiumDriver (where boundsAttr is set) and in
GetLocationAndSizeOfElement()/EnsureDeviceBounds() to prefer the live value from
the IWebElement: always call we.GetAttribute("bounds") when ei.ElementObject is
an IWebElement and use that result (falling back to ei.Properties only if
GetAttribute returns null/empty), then write the fresh bounds back into
ei.Properties["bounds"] (update existing key or add) so subsequent reads reflect
the latest rectangle and duplicate stale values are not preserved.

}
catch { boundsAttr = null; }
}
Expand Down Expand Up @@ -2932,6 +2936,16 @@ async void IWindowExplorer.HighLightElement(
ElementInfo filteredElementInfo =
POMExecutionUtils.FilterElementDetailsByCategory(ElementInfo, PomCategory);

// Keep exact spied element identity when caller did not request locator-based re-resolution.
// FilterElementDetailsByCategory may strip ElementObject in some flows, which can cause
// fallback XY lookup to highlight a nearby element (seen on iOS).
if (!locateElementByItLocators &&
filteredElementInfo.ElementObject == null &&
ElementInfo.ElementObject != null)
{
filteredElementInfo.ElementObject = ElementInfo.ElementObject;
}

((IWindowExplorer)mSeleniumDriver)
.HighLightElement(filteredElementInfo, locateElementByItLocators);

Expand Down Expand Up @@ -3088,7 +3102,6 @@ private ElementInfo GetElementAtMousePosition()
// ANDROID (mobile) + iOS → KEEP EXISTING LOGIC
// ================================================
foundNode = FindElementXmlNodeByXY(mousePosCurrent.X, mousePosCurrent.Y, false).Result;

if (foundNode != null)
{
foundElement = GetElementInfoforXmlNode(foundNode).Result;
Expand Down Expand Up @@ -3398,6 +3411,8 @@ private async Task<ElementInfo> GetElementInfoforXmlNode(XmlNode xmlNode)
{
EI.X = Convert.ToInt32(boundsXY[0]);
EI.Y = Convert.ToInt32(boundsXY[1]);
EI.Width = Math.Max(0, Convert.ToInt32(boundsXY[2]) - EI.X);
EI.Height = Math.Max(0, Convert.ToInt32(boundsXY[3]) - EI.Y);
}
}

Expand Down
Loading