browser: improve error wrapping for actionability functions

[joaquinalmora]

What

Improves error context in browser actionability functions by wrapping errors with descriptive messages for better debugging.

Why

Follow-up refactoring from #5111 addressing code review suggestions (items 2-5 from #5123).

Changes

• clickablePoint(): Added "finding clickable point: %w" error wrapping
• offsetPosition(): Added "finding offset position: %w" error wrapping
• newPointerAction(): Changed to "pointer action: %w" (let clickablePoint() own its message)
• Frame.position(): Added "finding position in frame: %w" error wrapping

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes. (N/A - no behavior change)
• I have run linter and tests locally (make check) and all pass.

Closes #5123

[CLAassistant]

All committers have signed the CLA.

[joaquinalmora]

Items 1 (#5111 (comment)) and 6 (#5111 (comment)) were left out. 1 seems like an open question and 6 was noted for future backoff retry implementation. Happy to tackle either if needed.

[ankur22]

@joaquinalmora thanks for contributing. I've lost all the context of the original issue and what feedback was given. To make it simpler to review i would really appreciate it if you could create a single commit per change linking back to the feedback comment.

Also, it looks like not all feedback is being incorporated into this PR (it says 2-5 in the PR description), can you clarify why that is? If might be necessary to leave the issue open if not all feedback is being taken care of in this PR.

[joaquinalmora]

@ankur22 Thanks for the feedback.
Item 1 (nil Rect vs &Rect{}) was left as an open question in the original PR, happy to implement whichever approach you prefer. Item 6 (20ms retry) was left out because you noted it was better suited for a future backoff retry implementation.
I'll split this into separate commits per change with links to the original feedback comment.

[ankur22]

@joaquinalmora thanks for the clarification, i missed your earlier comment actually. I think the PR looks good.

1. In terms of nil rect or &rect{}, I think whatever matches the behaviour of Playwright. This is area under question:

   k6/internal/js/modules/k6/browser/browser/element_handle_mapping.go

   Lines 18 to 24 in ecfb525

   	box, err := eh.BoundingBox()
   	// We want to avoid errors when an element is not visible and instead
   	// opt to return a nil rectangle -- this matches Playwright's behaviour.
   	if errors.Is(err, common.ErrElementNotVisible) {
   	return nil, nil
   	}
   	return box, err

   . Go ahead if you want to tackle that point.
2. I'm happy for you to attempt a backoff retry implementation. Also happy for you to not work on it. I personally feel that we should only really work on it if it becomes an issue.

[joaquinalmora]

@ankur22 I can tackle 1, going to hold off on 2.

[joaquinalmora]

@ankur22 Fixed the inconsistency. locator.boundingBox() now returns nil for hidden elements to match Playwright behavior: https://playwright.dev/docs/api/class-elementhandle#element-handle-bounding-box

[ankur22]

Looks good. Left a comment to consider.

[ankur22]

This change looks correct, but it doesn't match what the original feedback was: #5111 (comment), which was asking whether we should return a nail nil rect or an empty rect. I think returning a nil rect is good enough.

I'm happy to leave the change in though. I'm wondering if other boundingbox implementations need to this change too (I haven't checked myself whether that's the case yet though).

[joaquinalmora]

Yea, the original feedback mentioned nil vs &Rect{} but after looking into Playwright's actual behavior, nil (returning null to JS) seemed like the correct choice. Looking at both JS-facing implementations they now behave the same: element_handle_mapping.go already had this check, and I just added it to locator_mapping.go. Those two are the only JS-facing ones I found.

[ankur22]

Nice one! Thanks for the clarification.