[ntp-realbox] Adding animated caret [chromium/src : main]
Roman Arora (Gerrit)
Roman Arora added 3 comments![]( "Open in Gerrit")
Optional: I wonder if we could have the caret animation be something that is managed by its own class, like the way we do the placeholder animation, makes it maybe easier to reduce/test in isolation and reusable.
emptySpan.textContent = '\u200b';nit: this string should probably be a constant at the file level.
const originX = 12;nit: add explanation for the purpose/reason/origin of these numbers, maybe a constant too.
Related details
- Marlon Facey
- Omar Flores
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. |
Omar Flores (Gerrit)
Omar Flores added 1 comment![]( "Open in Gerrit")
Optional: I wonder if we could have the caret animation be something that is managed by its own class, like the way we do the placeholder animation, makes it maybe easier to reduce/test in isolation and reusable.
I don't know how much complexity would be involved in this but perhaps can be added as a follow up past resolving this blocker
Related details
- Marlon Facey
- Roman Arora
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. |
Marlon Facey (Gerrit)
Marlon Facey added 12 comments![]( "Open in Gerrit")
LGTM. Just a few comments on simplification.
grid-template-areas: 'stack';I don't mind this but it does seem like it does the same thing that smart compose is accomplishing with position: absolute down in line 273. Would it be simpler to just share those styles too? In fact, it looks like the mirror div already does have those styles. Is this necessary?
#mirror {Seems like a lot of these styles are shared with the other two input boxes that should be fairly similar. Could we not share the styles with them? Also are all of these styles needed? Seems like at least some of these font styles seem excessive.
<div id="caret"></div>Could we move this below the mirror element inside the same conditional?
private updateMirror_() {
if (this.disableCaretColorAnimation) {
return;
}
const mirror = this.$.mirror;
if (!mirror) {
return;
}
mirror.textContent = '';
const chars = this.input_.split('');
if (chars.length === 0) {
const emptySpan = document.createElement('span');
emptySpan.textContent = '\u200b';
mirror.appendChild(emptySpan);
return;
}
chars.forEach(char => {
const span = document.createElement('span');
if (char === ' ') {
span.textContent = ' ';
} else if (char === '\n') {
span.textContent = '\n\u200b';
} else {
span.textContent = char;
}
mirror.appendChild(span);
});
if (chars.length === 0) {
mirror.textContent = '\u200b';
}
}I wonder if instead of manually adding the input to this div every time we can just set the content to this.input_ like we do in the smart compose div. That would make this function not necessary right?
if (mirror.innerText.length !== input.value.length) {
this.updateMirror_();
}Would this ever happen?
caret.classList.remove('animating');
void caret.offsetHeight;
caret.classList.add('animating');I assume this just forces a rerender but is this necessary?
const firstSpan = mirror.firstChild as HTMLElement;nit: Can we name this something more descriptive?
if (firstSpan && typeof firstSpan.getBoundingClientRect === 'function') {Wouldn't this always be true? Doesn't every HTML element have a boundingClientRect?
if (charBeforeCursor && charBeforeCursor.getBoundingClientRect) {Ditto here. Shouldn't this always exist if charBeforeCursor exists?
const originX = 12;nit: add explanation for the purpose/reason/origin of these numbers, maybe a constant too.
+1
this.$.caret.classList.remove('caret-visible');I'm a little surprised that this isn't handled by blurring the input. Can we double check if this is necessary?
Related details
- Omar Flores
- Roman Arora
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. |
Marlon Facey (Gerrit)
Marlon Facey added 1 comment![]( "Open in Gerrit")
Omar FloresOptional: I wonder if we could have the caret animation be something that is managed by its own class, like the way we do the placeholder animation, makes it maybe easier to reduce/test in isolation and reusable.
I don't know how much complexity would be involved in this but perhaps can be added as a follow up past resolving this blocker
I think this is fine to do in a follow up. FWIW, I think it should be fairly simple since the caret component should really only need to know about the input.
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
Thank you Omar! to make sure we don't reintroduce the graphics smoothness regressions documented and mitigated in http://shortn/_vC8AOXjab6, let's make sure @jon...@chromium.org signs off on this change. a few gotchas comes to mind.
- iiuc getBoundingClientRect() forces a synchronous layout, if called repeatedly inside the input handler or animation frame, it could lead to dropped frames.
- I would benefit from a half-pagers description of how this trick is done, but iiuc the general idea is syncing the text to a hidden div. we should be mindful that every keystroke doesn't trigger a double-layout, one for the input, and one for the mirror.
- Ideally, animations should be offloaded to the compositor thread. Jonathan is best positioned to guide how to ensure that and which animations are compositor friendly or not (some information on that in the doc).
- making sure the caret component is hidden and/or its animation removed on blur. otherwise a WebUI like OB which is always present in the bg will continuously animate.
- general code complexity and maintenance, I'm particularly keen on minimizing the number of hidden/mirror divs we have for this solution and things like smart compose which iiuc uses a similar trick. I don't know if Search source of truth does a good job of managing complexity. It would be good to check in with Search FE Eng and philipbell@ to have consistent and manageable codes.
- Perhaps Jonathan can also help guide how to use the DevTools Performance panel to see the impact of this change.
Related details
- Omar Flores
- Roman Arora
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. |
Omar Flores (Gerrit)
Omar Flores added 4 comments![]( "Open in Gerrit")
Could we move this below the mirror element inside the same conditional?
moved above
nit: this string should probably be a constant at the file level.
Done
Marlon Faceynit: add explanation for the purpose/reason/origin of these numbers, maybe a constant too.
+1
Added explanation above these, its to match the padding spacing from the bounding box
I'm a little surprised that this isn't handled by blurring the input. Can we double check if this is necessary?
this is right, I think I was being overly cautious about this case, it's not needed after testing
Related details
- Marlon Facey
- Roman Arora
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. |
Omar Flores (Gerrit)
Omar Flores added 5 comments![]( "Open in Gerrit")
if (mirror.innerText.length !== input.value.length) {
this.updateMirror_();
}Omar FloresWould this ever happen?
This is needed when the mirror itself changes with values that can be prepopulated with suggestions, which can happen, without this the caret itself won't move unless text is directly typed in the input
caret.classList.remove('animating');
void caret.offsetHeight;
caret.classList.add('animating');Omar FloresI assume this just forces a rerender but is this necessary?
We do need this as the caret color needs to be reset + the blinking animations need to restart whenever there's new input
nit: Can we name this something more descriptive?
Done
if (firstSpan && typeof firstSpan.getBoundingClientRect === 'function') {Omar FloresWouldn't this always be true? Doesn't every HTML element have a boundingClientRect?
Done
if (charBeforeCursor && charBeforeCursor.getBoundingClientRect) {Ditto here. Shouldn't this always exist if charBeforeCursor exists?
Done
Related details
- Marlon Facey
- Roman Arora
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. |
Omar Flores (Gerrit)
Omar Flores added 1 comment![]( "Open in Gerrit")
Omar FloresI wonder if instead of manually adding the input to this div every time we can just set the content to this.input_ like we do in the smart compose div. That would make this function not necessary right?
this wouldn't really work for the mirror purposes because we need to figure out the exact x and y coordinates of all the letters placed in the input, if we were to use a single input this would make it almost impossible to figure out the y coordinates for multi line
Related details
- Marlon Facey
- Roman Arora
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. |
Omar Flores (Gerrit)
Omar Flores added 1 comment![]( "Open in Gerrit")
grid-template-areas: 'stack';Omar FloresI don't mind this but it does seem like it does the same thing that smart compose is accomplishing with position: absolute down in line 273. Would it be simpler to just share those styles too? In fact, it looks like the mirror div already does have those styles. Is this necessary?
we don't need this
Omar Flores (Gerrit)
Omar Flores added 2 comments![]( "Open in Gerrit")
Omar FloresOptional: I wonder if we could have the caret animation be something that is managed by its own class, like the way we do the placeholder animation, makes it maybe easier to reduce/test in isolation and reusable.
Marlon FaceyI don't know how much complexity would be involved in this but perhaps can be added as a follow up past resolving this blocker
I think this is fine to do in a follow up. FWIW, I think it should be fairly simple since the caret component should really only need to know about the input.
Done
Seems like a lot of these styles are shared with the other two input boxes that should be fairly similar. Could we not share the styles with them? Also are all of these styles needed? Seems like at least some of these font styles seem excessive.
I've removed a few rules that are in deed overkill, however, and I've used Gemini to help me out here, we want to keep a few of the font rendering rules because the cursor could drift as there is more text in the input, and I'd rather play it safe here
Related details
- Marlon Facey
- Roman Arora
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. |
Marlon Facey (Gerrit)
Marlon Facey added 2 comments![]( "Open in Gerrit")
#mirror {Omar FloresSeems like a lot of these styles are shared with the other two input boxes that should be fairly similar. Could we not share the styles with them? Also are all of these styles needed? Seems like at least some of these font styles seem excessive.
I've removed a few rules that are in deed overkill, however, and I've used Gemini to help me out here, we want to keep a few of the font rendering rules because the cursor could drift as there is more text in the input, and I'd rather play it safe here
That makes sense that there are a few extra styles but we could at least have the div share the reused styles and then only specify the remaining styles that differ with a different selector.
Omar FloresI wonder if instead of manually adding the input to this div every time we can just set the content to this.input_ like we do in the smart compose div. That would make this function not necessary right?
this wouldn't really work for the mirror purposes because we need to figure out the exact x and y coordinates of all the letters placed in the input, if we were to use a single input this would make it almost impossible to figure out the y coordinates for multi line
I'm not sure how that would change the y coordinate for multiline. Shouldn't the caret always be positioned right after the characters. So if the characters, font stylings and padding etc. are the same, the caret should just be in the right position naturally right?
Related details
- Omar Flores
- Roman Arora
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. |
Roman Arora (Gerrit)
Roman Arora added 1 comment![]( "Open in Gerrit")
Looping in John who is a pro at webui / animations.
Related details
- John Lee
- Omar Flores
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
@keyframes cycleColor {Have you considered using a step animation so the animation effectively runs at lower fps? For example:
```
.box {
width: 150px;
height: 150px;
/* 3s duration divided into 15 discrete steps = 5 frames per second */
animation: choppyColor 3s steps(15) infinite;
}
@keyframes choppyColor {
0% {
background-color: red;
}
50% {
background-color: green;
}
100% {
background-color: blue;
}
}
```Please reach out to flackr@ to ask if this is a potential power savings.
Omar Flores (Gerrit)
Omar Flores added 1 comment![]( "Open in Gerrit")
Omar FloresI wonder if instead of manually adding the input to this div every time we can just set the content to this.input_ like we do in the smart compose div. That would make this function not necessary right?
Marlon Faceythis wouldn't really work for the mirror purposes because we need to figure out the exact x and y coordinates of all the letters placed in the input, if we were to use a single input this would make it almost impossible to figure out the y coordinates for multi line
I'm not sure how that would change the y coordinate for multiline. Shouldn't the caret always be positioned right after the characters. So if the characters, font stylings and padding etc. are the same, the caret should just be in the right position naturally right?
That might be case, however for the x position, I believe the bounding box, only calculates the x positions of the entire box, not where the last word is typed in a block, happy to explain offline
Related details
- John Lee
- Marlon Facey
Omar Flores (Gerrit)
Omar Flores added 1 comment![]( "Open in Gerrit")
Have you considered using a step animation so the animation effectively runs at lower fps? For example:
```
.box {
width: 150px;
height: 150px;
/* 3s duration divided into 15 discrete steps = 5 frames per second */
animation: choppyColor 3s steps(15) infinite;
}@keyframes choppyColor {
0% {
background-color: red;
}
50% {
background-color: green;
}
100% {
background-color: blue;
}
}
```Please reach out to flackr@ to ask if this is a potential power savings.
I've tried this change, and unfortunately this removes the sharp change between the colors that is required in the design
Related details
- John Lee
- Marlon Facey
- Philip Rogers