WebUILocationBar: use cr-searchbox within our omnibox impl. [chromium/src : main]
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
Split up first half (not quite) of Frankenbox. Somewhat high-risk since it increases changes of cr-searchbox changes breaking something w/o actually being integrated enough to be truly useful, but it's a step towards the future...
Related details
- Paul Adedeji
- Paul Jensen
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. |
Paul Jensen (Gerrit)
Paul Jensen voted and added 5 comments![]( "Open in Gerrit")
Votes added by Paul Jensen
| Code-Review | +1 |
5 comments
lgtm as a solid step towards integration. I wonder a bit about the performance concerns I mentioned in https://chromium-review.git.corp.google.com/c/chromium/src/+/7979233/comment/6d724c89_ec739117/ but I'll take your advice that it's likely not too significant.
import '//resources/cr_components/searchbox/searchbox_input.js';what is this line importing beyond SearchboxInputElement whose type is imported on line 7? I'm wondering if we should tighten it up to specify the particular class we're importing.
input.ariaLabel = this.getAriaLabel_();just to make sure I'm understanding this: the accessibility attributes need to be set on an element inside the searchbox input element now, so they can't be done in the Lit HTML template anymore?
// Returns if changed (and if so, also bumps the version).if we're bumping the `uiVersion`, should we send the updated input state to the browser? one call site does this if this method returns true, but the others don't.
// If the current input state (its value and selection) matches its lastjust to check that I'm understanding: this can be removed because now we're using the searchbox input element, so we don't have to duplicate this code here anymore?
Related details
- Maks Orlovich
- Paul Adedeji
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 4 comments![]( "Open in Gerrit")
import '//resources/cr_components/searchbox/searchbox_input.js';what is this line importing beyond SearchboxInputElement whose type is imported on line 7? I'm wondering if we should tighten it up to specify the particular class we're importing.
So loading the .js file actually includes it in the application, and executes the registrations that make component bindings to elements --- <cr-searchbox> in this case --- active. Without it, <cr-searchbox> won't actually do anything searchbox-y.
(There is also a presubmit for this).
The other stuff is basically just for typechecking.
input.ariaLabel = this.getAriaLabel_();just to make sure I'm understanding this: the accessibility attributes need to be set on an element inside the searchbox input element now, so they can't be done in the Lit HTML template anymore?
Yeah, since we're annotating the <input> that's inside cr-searchbox. I suppose a cleaner option would be to add Lit properties that then get forwarded to attributes in searchbox_input.html.ts, but that won't work for ariaActiveDescendantElement, since its attribute form inherently can't work across shadow roots.
// Returns if changed (and if so, also bumps the version).if we're bumping the `uiVersion`, should we send the updated input state to the browser? one call site does this if this method returns true, but the others don't.
In the copy/cut spot, yeah (I think the result of missing it is that we might lose the state if switching tabs immediately after, though I can't seem to reproduce that). The others basically just defer it further down the line and do call it then, so they don't send over an intermediate state.
// If the current input state (its value and selection) matches its lastjust to check that I'm understanding: this can be removed because now we're using the searchbox input element, so we don't have to duplicate this code here anymore?
- Paul Adedeji
- Paul Jensen
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. |
Paul Jensen (Gerrit)
Paul Jensen added 5 comments![]( "Open in Gerrit")
As we just discussed offline, if this method of integrating the searchbox element into the WebUI toolbar is not part of a more complete integration between the entire searchbox and popup, and the WebUI toolbar, then we may not want to land this.
import '//resources/cr_components/searchbox/searchbox_input.js';Maks Orlovichwhat is this line importing beyond SearchboxInputElement whose type is imported on line 7? I'm wondering if we should tighten it up to specify the particular class we're importing.
So loading the .js file actually includes it in the application, and executes the registrations that make component bindings to elements --- <cr-searchbox> in this case --- active. Without it, <cr-searchbox> won't actually do anything searchbox-y.
(There is also a presubmit for this).
The other stuff is basically just for typechecking.
Acknowledged
input.ariaLabel = this.getAriaLabel_();Maks Orlovichjust to make sure I'm understanding this: the accessibility attributes need to be set on an element inside the searchbox input element now, so they can't be done in the Lit HTML template anymore?
Yeah, since we're annotating the <input> that's inside cr-searchbox. I suppose a cleaner option would be to add Lit properties that then get forwarded to attributes in searchbox_input.html.ts, but that won't work for ariaActiveDescendantElement, since its attribute form inherently can't work across shadow roots.
Acknowledged
// Returns if changed (and if so, also bumps the version).Maks Orlovichif we're bumping the `uiVersion`, should we send the updated input state to the browser? one call site does this if this method returns true, but the others don't.
In the copy/cut spot, yeah (I think the result of missing it is that we might lose the state if switching tabs immediately after, though I can't seem to reproduce that). The others basically just defer it further down the line and do call it then, so they don't send over an intermediate state.
Acknowledged
// If the current input state (its value and selection) matches its lastMaks Orlovichjust to check that I'm understanding: this can be removed because now we're using the searchbox input element, so we don't have to duplicate this code here anymore?
Yeah.
- Maks Orlovich
- Paul Adedeji
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
As we just discussed offline, if this method of integrating the searchbox element into the WebUI toolbar is not part of a more complete integration between the entire searchbox and popup, and the WebUI toolbar, then we may not want to land this.
So I think the next step is the start of the long-term sensible way of doing it; but of course it's different from the full popup (and requires stabilization of unbounded for best results)
Related details
- Paul Adedeji
- Paul Jensen
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. |
Paul Adedeji (Gerrit)
Paul Adedeji added 3 comments![]( "Open in Gerrit")
lgtm % a couple comments
const input = this.$.textInput.inputElement;nit: The child component might not have rendered yet by the time `connectedCallback` runs. Might be safer to this in `firstUpdated()` instead, using `await this.$.textInput.updateComplete;` to ensure the child's render cycle to complete.
this.updateStateFromTextInput();Should this be `this.onSearchboxInputTextUpdated_()` instead, since `this.onInputInput();` used to call `sendInputToBrowser()`?
Related details
- Maks Orlovich
- Paul Jensen
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. |
Paul Adedeji (Gerrit)
Paul Adedeji voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Maks Orlovich (Gerrit)
Maks Orlovich added 4 comments![]( "Open in Gerrit")
Split up first half (not quite) of Frankenbox. Somewhat high-risk since it increases changes of cr-searchbox changes breaking something w/o actually being integrated enough to be truly useful, but it's a step towards the future...
Acknowledged
Maks OrlovichAs we just discussed offline, if this method of integrating the searchbox element into the WebUI toolbar is not part of a more complete integration between the entire searchbox and popup, and the WebUI toolbar, then we may not want to land this.
So I think the next step is the start of the long-term sensible way of doing it; but of course it's different from the full popup (and requires stabilization of unbounded for best results)
Done
const input = this.$.textInput.inputElement;nit: The child component might not have rendered yet by the time `connectedCallback` runs. Might be safer to this in `firstUpdated()` instead, using `await this.$.textInput.updateComplete;` to ensure the child's render cycle to complete.
Good point. firstUpdated should be fine per docs, w/o any awaits I think:
https://lit.dev/docs/components/lifecycle/#:~:text=Some%20examples%20might,%7D
Should this be `this.onSearchboxInputTextUpdated_()` instead, since `this.onInputInput();` used to call `sendInputToBrowser()`?
- Paul Adedeji
- Paul Jensen
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
- Chromium IPC Reviews
- Paul Adedeji
- Paul Jensen
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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: ari...@chromium.org; IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
Shadow IPC reviewer(s): ari...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ari...@chromium.org
Reviewer source(s):
ari...@chromium.org, ke...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Ari Chivukula
- Ken Buchanan
- Paul Adedeji
- Paul Jensen
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
+ Charlie and Duncan for test update in page_load_metrics and contextual_tasks respectively
Related details
- Ari Chivukula
- Charles Harrison
- Duncan Mercer
- Ken Buchanan
- Paul Adedeji
- Paul Jensen
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. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
1 comment
(shadow) IPC review: I think I'm just looking at the PageHandlerFactory parts? Those seem reasonable
Related details
- Charles Harrison
- Duncan Mercer
- Ken Buchanan
- Maks Orlovich
- Paul Adedeji
- Paul Jensen
Ken Buchanan (Gerrit)
Ken Buchanan voted Code-Review+1![]( "Open in Gerrit")
- Charles Harrison
- Duncan Mercer
Charles Harrison (Gerrit)
Charles Harrison voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Duncan Mercer
- Maks Orlovich
- Paul Adedeji
- Paul Jensen
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
- Duncan Mercer
- Paul Adedeji
- Paul Jensen
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
const input = this.$.textInput.inputElement;Maks Orlovichnit: The child component might not have rendered yet by the time `connectedCallback` runs. Might be safer to this in `firstUpdated()` instead, using `await this.$.textInput.updateComplete;` to ensure the child's render cycle to complete.
Good point. firstUpdated should be fine per docs, w/o any awaits I think:
https://lit.dev/docs/components/lifecycle/#:~:text=Some%20examples%20might,%7D
Done
Related details
- Duncan Mercer
- Paul Adedeji
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
+qinmin@ for chrome/browser/contextual_tasks/contextual_tasks_interactive_uitest.cc
Related details
- Min Qin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- Maks Orlovich
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maks Orlovich (Gerrit)
Maks Orlovich voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resources/webui_toolbar/readonly_omnibox.ts
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
WebUILocationBar: use cr-searchbox within our omnibox impl.
(As a prep step for further integration). Unfortunately this already
requires pulling in a searchbox handler, since cr-searchbox-input will
listen on it for direct text control (which should never happen when we
use it).
- M chrome/browser/chrome_browser_interface_binders_webui_parts_desktop.cc
- M chrome/browser/contextual_tasks/contextual_tasks_interactive_uitest.cc
- M chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_browsertest.cc
- M chrome/browser/page_load_metrics/observers/initial_webui_page_load_metrics_observer_unittest.cc
- M chrome/browser/resources/webui_toolbar/app.ts
- M chrome/browser/resources/webui_toolbar/location_bar.ts
- M chrome/browser/resources/webui_toolbar/readonly_omnibox.css
- M chrome/browser/resources/webui_toolbar/readonly_omnibox.html.ts
- M chrome/browser/resources/webui_toolbar/readonly_omnibox.ts
- M chrome/browser/ui/views/location_bar/webui_location_bar_interactive_uitest.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_interactive_uitest.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_web_view.cc
- M chrome/browser/ui/views/toolbar/webui_toolbar_web_view.h
- M chrome/browser/ui/waap/initial_webui_browsertest.cc
- M chrome/browser/ui/webui/webui_toolbar/BUILD.gn
- M chrome/browser/ui/webui/webui_toolbar/webui_toolbar_ui.cc
- M chrome/browser/ui/webui/webui_toolbar/webui_toolbar_ui.h
- M chrome/browser/ui/webui/webui_toolbar/webui_toolbar_ui_browsertest.cc
- M chrome/test/data/webui/webui_toolbar/BUILD.gn
- M chrome/test/data/webui/webui_toolbar/location_bar_focus_test.ts
- M chrome/test/data/webui/webui_toolbar/location_bar_high_contrast_focus_test.ts
- M chrome/test/data/webui/webui_toolbar/location_bar_test.ts
- M chrome/test/data/webui/webui_toolbar/readonly_omnibox_focus_test.ts
- M chrome/test/data/webui/webui_toolbar/readonly_omnibox_test.ts
- M chrome/test/data/webui/webui_toolbar/toolbar_app_test.ts
Code-Review: +1 by Paul Adedeji, +1 by Paul Jensen, +1 by Ken Buchanan, +1 by Ari Chivukula, +1 by Charles Harrison, +1 by Min Qin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |