Change ToolBase::ResolveTarget for points to allow pseudo-nodes [chromium/src : main]
Russ Hamilton (Gerrit)
Russ Hamilton added 4 comments![]( "Open in Gerrit")
Bug:448353852Russ Hamiltonnit: the format for internal bugs is `Bug: b:448353852`
Done
// Type into input covered by pseudo-elementRuss HamiltonWould this have failed prior to this CL? IIUC we're not running TOCTOU checks in browsertests because we don't actually fetch an observation from the tab (unless I'm missing where that's happening...)
You're right. This test would pass before (because we don't run TOCTOU tests here). I can remove it if you think it's redundant.
// Third test case - input obscured by pseudo-element. Just try to click
// and type on the container.
GetPageContextFromFocusedTab(),
GetClientRect(kTypingTestTabId, "pseudoContainer", current_bounds),
ExecuteAction(type_in_current_bounds),
WaitForJsResult(kTypingTestTabId,
"()=>document.getElementById('pseudoInput').value",
kTypedString));Russ Hamiltonnit: WDYT about splitting these into separate tests...that way it's easier to see what's broken and each case is simpler to understand (as well has having less state to keep track of)
I'm fine with splitting them up, it just seems more verbose as we would need to copy 20+ lines of setup just to run 3 lines of test.
When they fail, the error message tells you where it failed (timed out waiting for <JS snippet>), so it doesn't seem that hard to understand.
Also, what state to keep track of are you referring to here?
// TODO: behamilton - This is a bit of a hack. Is there a better way?Russ HamiltonI expect we'd have to add some APIs on WebNode to do this correctly.
But why not just fix the [hit test](https://source.chromium.org/chromium/chromium/src/+/main:chrome/renderer/actor/tool_base.cc;l=96;drc=3173880ec3dc5bce93ea50722d517a9056db6523) to use `InnerPossiblyPseudoNode()` (which you'd have to expose on WebHitTestResult) so that you don't have to touch this part? IMHO if APC includes pseudo nodes we should hit test DOM to pseudo nodes too.
Yeah, I guess that makes sense. For some reason I was thinking we'd need more extensive changes than that.
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
3 comments
// Type into input covered by pseudo-elementRuss HamiltonWould this have failed prior to this CL? IIUC we're not running TOCTOU checks in browsertests because we don't actually fetch an observation from the tab (unless I'm missing where that's happening...)
You're right. This test would pass before (because we don't run TOCTOU tests here). I can remove it if you think it's redundant.
Don't feel strongly but yeah, it's not adding anything so don't see any reason to keep it.
// Third test case - input obscured by pseudo-element. Just try to click
// and type on the container.
GetPageContextFromFocusedTab(),
GetClientRect(kTypingTestTabId, "pseudoContainer", current_bounds),
ExecuteAction(type_in_current_bounds),
WaitForJsResult(kTypingTestTabId,
"()=>document.getElementById('pseudoInput').value",
kTypedString));Russ Hamiltonnit: WDYT about splitting these into separate tests...that way it's easier to see what's broken and each case is simpler to understand (as well has having less state to keep track of)
I'm fine with splitting them up, it just seems more verbose as we would need to copy 20+ lines of setup just to run 3 lines of test.
When they fail, the error message tells you where it failed (timed out waiting for <JS snippet>), so it doesn't seem that hard to understand.
Also, what state to keep track of are you referring to here?
It's nit-level but mainly just minimizing cognitive load (i.e. an unfamiliar reader investigating this broken test wouldn't have to understand what each step is doing and keep track of any possible state change to the test harness or page). But it's a minor point and I don't feel strongly so I'll leave it to you.
Related details
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 2 comments![]( "Open in Gerrit")
// Type into input covered by pseudo-elementRuss HamiltonWould this have failed prior to this CL? IIUC we're not running TOCTOU checks in browsertests because we don't actually fetch an observation from the tab (unless I'm missing where that's happening...)
David BokanYou're right. This test would pass before (because we don't run TOCTOU tests here). I can remove it if you think it's redundant.
Don't feel strongly but yeah, it's not adding anything so don't see any reason to keep it.
Done
// Third test case - input obscured by pseudo-element. Just try to click
// and type on the container.
GetPageContextFromFocusedTab(),
GetClientRect(kTypingTestTabId, "pseudoContainer", current_bounds),
ExecuteAction(type_in_current_bounds),
WaitForJsResult(kTypingTestTabId,
"()=>document.getElementById('pseudoInput').value",
kTypedString));Russ Hamiltonnit: WDYT about splitting these into separate tests...that way it's easier to see what's broken and each case is simpler to understand (as well has having less state to keep track of)
David BokanI'm fine with splitting them up, it just seems more verbose as we would need to copy 20+ lines of setup just to run 3 lines of test.
When they fail, the error message tells you where it failed (timed out waiting for <JS snippet>), so it doesn't seem that hard to understand.
Also, what state to keep track of are you referring to here?
It's nit-level but mainly just minimizing cognitive load (i.e. an unfamiliar reader investigating this broken test wouldn't have to understand what each step is doing and keep track of any possible state change to the test harness or page). But it's a minor point and I don't feel strongly so I'll leave it to you.
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
1 comment
// Tests that typing at a coordinates succeednit: s/a coordinates/coordinates/
Related details
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |