[Tabbed Web App] Fix spurious incorrect origin text flash [chromium/src : main]
Dana Fried (Gerrit)
Dana Fried voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Chromium Web Applications Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Shadowed: jap...@chromium.org
Reviewer source(s):
jap...@chromium.org is from context(googleclient/chrome/chromium_gwsq/chrome/browser/ui/web_applications/config.gwsq,googleclient/chrome/chromium_gwsq/chrome/browser/web_applications/config.gwsq)
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 1 comment![]( "Open in Gerrit")
// Flash text is generated from the app, not the tab. If the tab is
// switched, it may end up being a different source which for which the
// app-generated text is incorrect. Therefore, if the tab changes, don't
// start a new flash animation, ever.
Observe(nullptr);Won't this stop observing the web contents, and therefore prevent any new origin text animations from happening for the same browser instance? Since this is created once per `BrowserView`, should this instead be doing the following?
```
Observe(nullptr);
Observe(selection.new_contents);
```
Related details
- Dana Fried
- Nate Chapin
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. |
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Auto-Submit | +1 |
1 comment
// Flash text is generated from the app, not the tab. If the tab is
// switched, it may end up being a different source which for which the
// app-generated text is incorrect. Therefore, if the tab changes, don't
// start a new flash animation, ever.
Observe(nullptr);Won't this stop observing the web contents, and therefore prevent any new origin text animations from happening for the same browser instance? Since this is created once per `BrowserView`, should this instead be doing the following?
```
Observe(nullptr);
Observe(selection.new_contents);
```
The problem is that the text isn't updated, and there's no easy way to do that.
That's the current behavior; it observes whatever the current web contents is. But the text comes from the app, so if it's not the same site it shows but it's wrong, which is what the attached bug is.
Related details
- Dibyajyoti Pal
- Nate Chapin
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. |
Dana Fried (Gerrit)
Dana Fried voted Auto-Submit+0![]( "Open in Gerrit")
| Auto-Submit | +0 |
Dana Fried (Gerrit)
Dana Fried voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dibyajyoti Pal
- Nate Chapin
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. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
// Flash text is generated from the app, not the tab. If the tab is
// switched, it may end up being a different source which for which the
// app-generated text is incorrect. Therefore, if the tab changes, don't
// start a new flash animation, ever.
Observe(nullptr);Dana FriedWon't this stop observing the web contents, and therefore prevent any new origin text animations from happening for the same browser instance? Since this is created once per `BrowserView`, should this instead be doing the following?
```
Observe(nullptr);
Observe(selection.new_contents);
```
The problem is that the text isn't updated, and there's no easy way to do that.
That's the current behavior; it observes whatever the current web contents is. But the text comes from the app, so if it's not the same site it shows but it's wrong, which is what the attached bug is.
I've updated it to handle things a little more accurately.
Related details
- Dibyajyoti Pal
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
if (!selection.old_contents) {
// Observe the new contents if it's the first active contents.
Observe(selection.new_contents);
} else if (selection.old_contents != selection.new_contents) {
// Flash text is generated from the app, not the tab. If the tab is
// switched, it may end up being a different source which for which the
// app-generated text is incorrect. Therefore, if the tab actually
// changes (as opposed to the initial contents being added), stop trying
// to flash the origin text on page load.
Observe(nullptr);
}
}Hmmmm, looking at the bug, it seems like the fix might be a bit more complicated. The old origin still showing up when a new navigation happens means that `origin_text_` [1] is stale, and stopping the observing of the old web contents might not be enough to fix it.
Imo, we should also be updating the `origin_text_` and `label_` both when the tab strip model changes, because in case the tab changes to a new tab with a different theme, the flash will still occur regardless of whether `web_contents()` changed, leading to the previous tab's origin being shown.
What if we instead did the following:
1. Move L169-200, which casues the `label_` and `origin_text_` to be updated to a new function, called `bool UpdateOriginText(bool start_animation)`. The only difference is that in L200, we'll wrap the starting of the animation to the following:
```
if(start_animation) {
StartFadeAnimation();
}
```
2. Update `DidFinishNavigation()` to call:
```
void WebAppOriginText::DidFinishNavigation(content::NavigationHandle* handle) {
if (!handle->IsInPrimaryMainFrame() || handle->IsSameDocument()) {
return;
}
UpdateOriginText(true);
}
```
3. Update `OnTabStripModelChanged()` to be:
```
void WebAppOriginText::OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) {
if (selection.active_tab_changed() && !tab_strip_model->empty()) {
Observe(nullptr); // Stop observing old web contents.
Observe(selection.new_contents);
UpdateOriginText(false); // Updates origin, but prevents animation from showing.
}
}
```
Wdyt?
void DrawImageToCanvas(StaticBitmapImage* image, cc::PaintCanvas& canvas) {Is this change needed? Let's remove if not?
Related details
- Dana Fried
- Nate Chapin
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. |
Dana Fried (Gerrit)
Dana Fried added 2 comments![]( "Open in Gerrit")
if (!selection.old_contents) {You're right; there's no really easy fix here. I'm gonna try to rethink this CL and will send it back a bit later.
void DrawImageToCanvas(StaticBitmapImage* image, cc::PaintCanvas& canvas) {Is this change needed? Let's remove if not?
- Dibyajyoti Pal
- Nate Chapin
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. |
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Commit-Queue | +1 |
1 comment
if (!selection.old_contents) {
// Observe the new contents if it's the first active contents.
Observe(selection.new_contents);
} else if (selection.old_contents != selection.new_contents) {
// Flash text is generated from the app, not the tab. If the tab is
// switched, it may end up being a different source which for which the
// app-generated text is incorrect. Therefore, if the tab actually
// changes (as opposed to the initial contents being added), stop trying
// to flash the origin text on page load.
Observe(nullptr);
}
}Dana FriedHmmmm, looking at the bug, it seems like the fix might be a bit more complicated. The old origin still showing up when a new navigation happens means that `origin_text_` [1] is stale, and stopping the observing of the old web contents might not be enough to fix it.
Imo, we should also be updating the `origin_text_` and `label_` both when the tab strip model changes, because in case the tab changes to a new tab with a different theme, the flash will still occur regardless of whether `web_contents()` changed, leading to the previous tab's origin being shown.
What if we instead did the following:
1. Move L169-200, which casues the `label_` and `origin_text_` to be updated to a new function, called `bool UpdateOriginText(bool start_animation)`. The only difference is that in L200, we'll wrap the starting of the animation to the following:```
if(start_animation) {
StartFadeAnimation();
}
```2. Update `DidFinishNavigation()` to call:
```
void WebAppOriginText::DidFinishNavigation(content::NavigationHandle* handle) {
if (!handle->IsInPrimaryMainFrame() || handle->IsSameDocument()) {
return;
}
UpdateOriginText(true);
}
```3. Update `OnTabStripModelChanged()` to be:
```
void WebAppOriginText::OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) {
if (selection.active_tab_changed() && !tab_strip_model->empty()) {
Observe(nullptr); // Stop observing old web contents.
Observe(selection.new_contents);
UpdateOriginText(false); // Updates origin, but prevents animation from showing.
}
}
```Wdyt?
You're right; there's no really easy fix here. I'm gonna try to rethink this CL and will send it back a bit later.
Done
Related details
- Dibyajyoti Pal
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
1 comment
Related details
- Dana Fried
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Commit-Queue+2![]( "Open in Gerrit")
- Nate Chapin
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![]( "Open in Gerrit")
Change information
[Tabbed Web App] Fix spurious incorrect origin text flash
This disables the origin text flash on tabbed PWAs when the user
switches tabs; it was previously causing a spurious flash of the wrong
text any time the user changed tabs and a new page loaded.
Explanation in the code.
- M chrome/browser/ui/views/web_apps/frame_toolbar/web_app_origin_text.cc
- M chrome/browser/ui/views/web_apps/frame_toolbar/web_app_origin_text.h
Code-Review: +1 by Dibyajyoti Pal
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |