Introduce ClickToolJavaScriptFeature and click_tool.ts. [chromium/src : main]
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 8 comments![]( "Open in Gerrit")
Kirubel AkliluI've added some comments inline. Additionally, please consider opting in to enable `kAssertOnJavaScriptErrors` for your new tests if possible. I'm currently working to enable it everywhere, but am unable to do so yet. However, if it works for your tests, especially, minimally for JavascriptTests which won't be affected by errors in other scripts, it will help maintain your scripts from accidentally throwing any JS errors which will cause problems in the future.
Example CL: https://chromium-review.googlesource.com/c/chromium/src/+/7560593
Thanks for flagging, I've enabled this flag in these tests and they still pass.
Kirubel Akliluplease add tests for this feature. I'm not sure if this is the best example, but here is one that I was able to quickly find: https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/reader_mode/model/reader_mode_java_script_feature_unittest.mm
(Note you should avoid using `OverrideJavaScriptFeatures` as it will likely cause more trouble than it's worth for such a high level feature.)
Done
const base::TimeDelta kJavascriptExecutionTimeout = base::Seconds(5);Kirubel Akliluthis is extremly long, the default for executing JS is 100ms. Is there a reason this one was used?
I didn't know there was a default, thanks for pointing it out! Since [CallJavaScriptFunction](https://source.chromium.org/chromium/chromium/src/+/main:ios/web/public/js_messaging/java_script_feature.h;l=209;drc=15bd3b5e7b88bad5db5dbc3305e5b3c9e7c5c9d8) required both a timeout and a callback, I assumed there was no default. Otherwise, I'd have expected CallJavaScriptFunction could be called with just a callback.
I've updated this to use that default. I'd chosen 5s since that's what Autofill is using.
// TODO (crbug.com/476090817) - Inject in all frames once we canKirubel Akliluyou might not be able to perform a "click" in an iframe directly. I'm not sure how all these tools will be hooked together, but ptal at the context menu logic and see how clicks are always initiated from the global page coordinates and then messages are sent to iframes within the script rather than from the native side. This is necessary because the native side doesn't have the full layout of the webpage so it can't do the necessary coordinate translations from global points to iframe ones.
Agreed that we can't directly click in an iframe with this approach, I'd discussed this in the doc [here](https://docs.google.com/document/d/18owGXUeF1fKLxQTOB_999vuiHeDFY_BIrgzvRW-jn7Y/edit?resourcekey=0-hGeELLnA9OrfwX7zMcOjHg&tab=t.opqz585th1ag#heading=h.74r5hxnods1g).
From the JS, can't we return the relevant data needed for coordinate translations so that the native side can call the desired WebFrame with the correct arguments? I think this is aligned with the Remote Frame Registration approach that autofill currently uses.
I agree that we can use an approach similar to context menu. I think the main benefit is that it requires less hops between native & JS code.
In any case, I plan to handle this in a future CL and have a separate bug for it.
web::WebFrame* main_frame = manager->GetMainWebFrame();Kirubel AkliluIt would be better to pass the WebFrame directly into this function instead. As-is, the caller of ClickToolJavaScriptFeature::Click would have no control over the domain the click is performed on.
I don't think this is currently feasible since the caller of this JS feature won't know the relevant WebFrame by coordinate until we execute the JS on the page.
If we solve the iframe problem by passing a frame identifier from JS to native then I agree we could make this method take in the web::WebFrame since we'll want to click into.
Kirubel Akliluscripts should have tests based upon `JavascriptTest, https://source.chromium.org/chromium/chromium/src/+/main:ios/web/public/test/javascript_test.h
This allows for basic testing of script functionality without the need for spinning up all the surrounding //ios/web intrastructure.
I've added tests in the child CL, https://crrev.com/c/7544258. I didn't pull them into this CL to keep it smaller for review. I've now pulled them into this CL so we land it together.
* @param clickType The type of click (0=UNKNOWN, 1=LEFT, 2=RIGHT).Kirubel Akliluwhy the weird numeric mapping? Should the expected value from MouseEvent.button just be accepted instead? (0/default for left click, 2 for right click)
These are the values set in the proto by the caller (see [actions_data.proto](https://source.chromium.org/chromium/chromium/src/+/main:components/optimization_guide/proto/features/actions_data.proto;l=41;drc=bf712ec1a13783224debb691ba88ad5c15b93194)). We're using this proto to stay consistent with Desktop.
return element.dispatchEvent(Kirubel Akliluthis should be heavily tested via Javascript test as it could break at any time if iOS changes behavior in WebKit and without automated testing, maybe even with it, such failure would likely go unnoticed.
Testing was added in a downstream CL. I've pulled it into this CL and moved some of the actuation-specific stuff into a child CL.
The tests listen for and verify these events are dispatched.
Related details
- Mike Dougherty
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. |
Mike Dougherty (Gerrit)
Mike Dougherty added 6 comments![]( "Open in Gerrit")
web::WebFrame* main_frame = manager->GetMainWebFrame();Kirubel AkliluIt would be better to pass the WebFrame directly into this function instead. As-is, the caller of ClickToolJavaScriptFeature::Click would have no control over the domain the click is performed on.
I don't think this is currently feasible since the caller of this JS feature won't know the relevant WebFrame by coordinate until we execute the JS on the page.
If we solve the iframe problem by passing a frame identifier from JS to native then I agree we could make this method take in the web::WebFrame since we'll want to click into.
I don't know how this feature will be called from higher level feature code, but it would be better even if the caller just obtained the WebFrame itself from the WebState and passed it in. As-is, this leaves ClickToolJavaScriptFeature open to vulnerabilities.
The problem with the current implementation is the caller has no guarantee that the click occurs on the expected domain. For example:
- example.com is loaded, `web_state->GetLastCommittedURL()` is example.com (and maybe feature code even checks that) Activation Tool works to determine it wants to click on something on example.com
- WebPage navigation is triggered to go to evil.com
- Feature code calls `ClickToolJavaScriptFeature::Click(web_state,...)`
- This feature will then click on evil.com
The problem is that this existing API doesn't even provide an opportunity to the feature code to even check which domain will clicked on.
Instead, the feature code should obtain the WebFrame, do validation that the security origin is as expected, and then pass that in here. In that case, if the webpage has navigated to evil.com, the JS will not be executed.
In general, all features should accept a WebFrame pointer instead of WebState.
I've left specific comments below, but please make sure tests are consistent with those already in the codebase. This file feels very inconsistent with the codebase.
go/objc-style#optimize-for-the-reader-not-the-writer
<html>This is a fairly long/complex sample page. I would put it into a test only html file instead of inline.
Most tests use existing pages, but this shows how arbitrary files can be served with the embedded test server: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/navigation/history_state_operations_inttest.mm;l=64?q=file:test%20pony%20file:%5Eios
}This tests the implementation detail of the click tool JS (sending events) rather than ensuring the tool actually works. There should be tests to ensure that the click on the button actually worked. For example, consider having the button change some visible state on the webpage (like modifying some text) and then validate that the click on the button worked by checking the value of the visible text.
INSTANTIATE_TEST_SUITE_P(Why is a parameter based test used here instead of separate tests? I don't think this is the best format. It seems much more convoluted and it risks having all tests cases disabled even if only a single test case were to start failing.
You should move shared logic into helper functions on the test class (like `MakeExpectedEvents`), free functions, or use SetUp as is the common test file format in almost all test files I've seen.
Parameterized helpers should be used only if you want to run many tests in different cases. For example, with and without a feature flag implemented.
std::ostream& operator<<(std::ostream& os, const ClickInput& input) {IMO, these custom print functions further confirm that this isn't the right way to write these tests. Even just based on codebase consistency, there are zero other ios tests which use this pattern.
Related details
- Kirubel Aklilu
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. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 7 comments![]( "Open in Gerrit")
web::WebFrame* main_frame = manager->GetMainWebFrame();Kirubel AkliluIt would be better to pass the WebFrame directly into this function instead. As-is, the caller of ClickToolJavaScriptFeature::Click would have no control over the domain the click is performed on.
Mike DoughertyI don't think this is currently feasible since the caller of this JS feature won't know the relevant WebFrame by coordinate until we execute the JS on the page.
If we solve the iframe problem by passing a frame identifier from JS to native then I agree we could make this method take in the web::WebFrame since we'll want to click into.
I don't know how this feature will be called from higher level feature code, but it would be better even if the caller just obtained the WebFrame itself from the WebState and passed it in. As-is, this leaves ClickToolJavaScriptFeature open to vulnerabilities.
The problem with the current implementation is the caller has no guarantee that the click occurs on the expected domain. For example:
- example.com is loaded, `web_state->GetLastCommittedURL()` is example.com (and maybe feature code even checks that) Activation Tool works to determine it wants to click on something on example.com
- WebPage navigation is triggered to go to evil.com
- Feature code calls `ClickToolJavaScriptFeature::Click(web_state,...)`
- This feature will then click on evil.com
The problem is that this existing API doesn't even provide an opportunity to the feature code to even check which domain will clicked on.
Instead, the feature code should obtain the WebFrame, do validation that the security origin is as expected, and then pass that in here. In that case, if the webpage has navigated to evil.com, the JS will not be executed.
In general, all features should accept a WebFrame pointer instead of WebState.
OK, I've updated this to use WebFrame. I'll figure out how to identify the right frame and plumb it here in a future CL.
FWIW, I think this scenario may not be possible based on how the wider actuation feature will work, but it seems reasonable to prevent it in this manner.
In general, all features should accept a WebFrame pointer instead of WebState.
I'd recommend adding this guideline to ios/web/public/js_messaging/README.md. There are [several examples](https://source.chromium.org/search?q=f:java_script_feature.mm%20%22GetMainWebFrame%22) of JSFeatures that take in a WebState and use it to get the primary main frame.
I've left specific comments below, but please make sure tests are consistent with those already in the codebase. This file feels very inconsistent with the codebase.
go/objc-style#optimize-for-the-reader-not-the-writer
I couldn't glean a well-established pattern since there is only a handful tests that extend web::JavaScriptTest and they use a variety of approaches.
Acknowledge the concern to optimize for readability. I'd thought the parameterized approach was clear and minimized the loc needed to write & review, but that's may just be my opinion.
This is a fairly long/complex sample page. I would put it into a test only html file instead of inline.
Most tests use existing pages, but this shows how arbitrary files can be served with the embedded test server: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/navigation/history_state_operations_inttest.mm;l=64?q=file:test%20pony%20file:%5Eios
Done, thank you for the example
This is a fairly long/complex sample page. I would put it into a test only html file instead of inline.
Most tests use existing pages, but this shows how arbitrary files can be served with the embedded test server: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/navigation/history_state_operations_inttest.mm;l=64?q=file:test%20pony%20file:%5Eios
Done, thanks for the example
This tests the implementation detail of the click tool JS (sending events) rather than ensuring the tool actually works. There should be tests to ensure that the click on the button actually worked. For example, consider having the button change some visible state on the webpage (like modifying some text) and then validate that the click on the button worked by checking the value of the visible text.
Done, although I kept the logic in the HTML that records the events and saves it on the window.
Why is a parameter based test used here instead of separate tests? I don't think this is the best format. It seems much more convoluted and it risks having all tests cases disabled even if only a single test case were to start failing.
You should move shared logic into helper functions on the test class (like `MakeExpectedEvents`), free functions, or use SetUp as is the common test file format in almost all test files I've seen.
Parameterized helpers should be used only if you want to run many tests in different cases. For example, with and without a feature flag implemented.
Acknowledge the concern, I've un-parameterized these tests.
I've seen parameterized tests used in this manner before to test a simple function over many (input,output) pairs while minimizing repeated setup. I agree that we can utilize helpers and that separate test cases is better for gardeners.
std::ostream& operator<<(std::ostream& os, const ClickInput& input) {IMO, these custom print functions further confirm that this isn't the right way to write these tests. Even just based on codebase consistency, there are zero other ios tests which use this pattern.
Done, I'd added these to make debugging the parameterized tests easier. I've seen code like this in other parts of chrome but ack that it's not in ios.
Related details
- Mike Dougherty
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. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 1 comment![]( "Open in Gerrit")
+rohitrao@, can you review these changes?
moving michaeldo@ to cc since he's OOO for the next week.
Related details
- Rohit Rao
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. |
Mike Dougherty (Gerrit)
Mike Dougherty added 7 comments![]( "Open in Gerrit")
// TODO (crbug.com/476090817) - Inject in all frames once we canKirubel Akliluyou might not be able to perform a "click" in an iframe directly. I'm not sure how all these tools will be hooked together, but ptal at the context menu logic and see how clicks are always initiated from the global page coordinates and then messages are sent to iframes within the script rather than from the native side. This is necessary because the native side doesn't have the full layout of the webpage so it can't do the necessary coordinate translations from global points to iframe ones.
Agreed that we can't directly click in an iframe with this approach, I'd discussed this in the doc [here](https://docs.google.com/document/d/18owGXUeF1fKLxQTOB_999vuiHeDFY_BIrgzvRW-jn7Y/edit?resourcekey=0-hGeELLnA9OrfwX7zMcOjHg&tab=t.opqz585th1ag#heading=h.74r5hxnods1g).
From the JS, can't we return the relevant data needed for coordinate translations so that the native side can call the desired WebFrame with the correct arguments? I think this is aligned with the Remote Frame Registration approach that autofill currently uses.
I agree that we can use an approach similar to context menu. I think the main benefit is that it requires less hops between native & JS code.
In any case, I plan to handle this in a future CL and have a separate bug for it.
I am not aware of how autofill does this today, if they pass the coordinates back to the native side it should work for you too.
web::WebFrame* main_frame = manager->GetMainWebFrame();Kirubel AkliluIt would be better to pass the WebFrame directly into this function instead. As-is, the caller of ClickToolJavaScriptFeature::Click would have no control over the domain the click is performed on.
Mike DoughertyI don't think this is currently feasible since the caller of this JS feature won't know the relevant WebFrame by coordinate until we execute the JS on the page.
If we solve the iframe problem by passing a frame identifier from JS to native then I agree we could make this method take in the web::WebFrame since we'll want to click into.
Kirubel AkliluI don't know how this feature will be called from higher level feature code, but it would be better even if the caller just obtained the WebFrame itself from the WebState and passed it in. As-is, this leaves ClickToolJavaScriptFeature open to vulnerabilities.
The problem with the current implementation is the caller has no guarantee that the click occurs on the expected domain. For example:
- example.com is loaded, `web_state->GetLastCommittedURL()` is example.com (and maybe feature code even checks that) Activation Tool works to determine it wants to click on something on example.com
- WebPage navigation is triggered to go to evil.com
- Feature code calls `ClickToolJavaScriptFeature::Click(web_state,...)`
- This feature will then click on evil.com
The problem is that this existing API doesn't even provide an opportunity to the feature code to even check which domain will clicked on.
Instead, the feature code should obtain the WebFrame, do validation that the security origin is as expected, and then pass that in here. In that case, if the webpage has navigated to evil.com, the JS will not be executed.
In general, all features should accept a WebFrame pointer instead of WebState.
OK, I've updated this to use WebFrame. I'll figure out how to identify the right frame and plumb it here in a future CL.
FWIW, I think this scenario may not be possible based on how the wider actuation feature will work, but it seems reasonable to prevent it in this manner.
In general, all features should accept a WebFrame pointer instead of WebState.
I'd recommend adding this guideline to ios/web/public/js_messaging/README.md. There are [several examples](https://source.chromium.org/search?q=f:java_script_feature.mm%20%22GetMainWebFrame%22) of JSFeatures that take in a WebState and use it to get the primary main frame.
Understood, I've made a note to cleanup that readme. There is a note about using WebFrame directly, but it's at the very end of the file, buried in a paragraph about various topics so I'll clean that up. I'll also review the existing features taking a WebState.
class ClickToolJavaScriptFeatureTest : public PlatformTest {is there a reason that PlatformTest is used instead of `WebTestWithWebState` which will setup the WebState for you?
ASSERT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(you don't need to check for __gCrWeb if you instead wiat for the WebFrame instead. It's also possible that this wold pass, but your future calls to get the main frame fail since frame registration is async.
I would update this to simply wait for the main frame similar to this test:
https://source.chromium.org/chromium/chromium/src/+/main:ios/web/js_features/clipboard/clipboard_java_script_feature_unittest.mm;drc=75bd37cd98d8654e740d5b267c1be6fbf81c6ede;l=135?q=file:%5Eios%2Fweb%20wait%20mainwebframe
I thought WaitForMainFrame() or similar was a common shared helper, but I couldn't find it. (I'll look into adding it into a shared location in the future.)
Kirubel AkliluI've left specific comments below, but please make sure tests are consistent with those already in the codebase. This file feels very inconsistent with the codebase.
go/objc-style#optimize-for-the-reader-not-the-writer
I couldn't glean a well-established pattern since there is only a handful tests that extend web::JavaScriptTest and they use a variety of approaches.
Acknowledge the concern to optimize for readability. I'd thought the parameterized approach was clear and minimized the loc needed to write & review, but that's may just be my opinion.
feel free to reach out if you ever need an example for the best way to do something. Or, alternatively, I tend to append `(file:^ios OR (file:^components AND file:ios))` to my code search query and then look at the examples with the most recent copyright date at the top of the file.
<html>Kirubel AkliluThis is a fairly long/complex sample page. I would put it into a test only html file instead of inline.
Most tests use existing pages, but this shows how arbitrary files can be served with the embedded test server: https://source.chromium.org/chromium/chromium/src/+/main:ios/web/navigation/history_state_operations_inttest.mm;l=64?q=file:test%20pony%20file:%5Eios
Done, thanks for the example
np. You should still be able to use the EmbeddedTestServer as in the example instead of manually reading the string file and loading it this way. For example: https://source.chromium.org/search?q=EmbeddedTestServer%20javascripttest%20file:%5Eios&sq=
TEST_F(ClickToolJavascriptTest, PHYSICAL_SingleClick_OnButton) {Why is this and "DIPS" above all caps?
Related details
- Kirubel Aklilu
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. |