Regression: Unwanted 'Theme created by.' tooltip is seen on reloading chrome://apps page.
Reported by
rk...@etouch.net,
Jan 16 2017
|
|||||||||
Issue descriptionChrome Version: 57.0.2983.0 Revision 266a86c94d34f3c7c3d22ee00624636b801f80d3-refs/heads/master@{#443819} OS:Windows (7,8,8.1,10),Linux (14.04 LTS),Mac OS X(10.10.5, 10.11.4) What steps will reproduce the problem? (1) Launch chrome, navigate to chrome://apps (2) Relaod the page and observe at the left bottom side of the page. Actual: Unwanted 'Theme created by.' tooltip is seen on reloading page. Expected: No such a tooltip should seen. This is a regression issue, broken in 'M-57', will soon update the other info: Good Build: 57.0.2953.0 Bad Build: 57.0.2955.0
,
Jan 16 2017
Comparing chrome://tracing output from 55 and 57 shows 57 generates 3 frames, 55 generates 2. Need to figure out why the second frame is not blocked in 57.
,
Jan 17 2017
Adding release block label, please undo if not the case.
,
Jan 17 2017
Reduction so far:
<!DOCTYPE html>
<link id="themecss" rel="stylesheet" href="none.css">
<script>
document.addEventListener('DOMContentLoaded', () => {
themecss.href = 'none.css?' + Date.now();
});
</script>
<div id="none">FAIL</div>
<div>Should you see a flash of FAIL?</div>
where "none.css" is:
#none { display:none }
,
Jan 18 2017
Just to update, M-57 gets branched tomorrow and would probably go to Beta during 1st week of Feb. Please adjust or update the blocker accordingly. Would be good to have this fixed and verified before branch.
,
Jan 19 2017
This is related to issue 682618. What happens is: 1. themecss contains a display:none for the tooltip 2. themecss finishes loading, marking active stylesheets dirty 3. DOMContentLoaded triggers 4. DOMContentLoaded handler changes themecss href to include a query part. 5. We generate a frame -> lifecycle update 6. Lifecycle update updates active stylesheets, which doesn't include the old themecss because we are currently loading the new sheet. See [1]. 7. We paint without any themecss. This is the flash. 8. The new url for themecss finishes loading. 9. New frame generated with the new themecss applied -> tooltip hidden. Before we introduced active stylesheet update as part of the lifecycle update, we would do a synchronous update of the active stylesheets as part of step 2 above. Since changing href does not trigger an active stylesheet update by itself, as described in issue 682618, we would not update the active stylesheets while loading the new themecss which would avoid the flashing tooltip in chrome://apps. As noted in 682618, any other changes that triggers an active stylesheet update will change the applied stylesheets during the load. That means, if we conclude that Firefox is right and fix issue 682618, that would have caused this regression even without moving the active stylesheet update to the lifecycle update. I think this is a race condition in the chrome://apps that should be fixed. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/DocumentStyleSheetCollection.cpp?sq=package:chromium&dr=C&l=67-68
,
Jan 19 2017
cc'ing estade@ based on git log/blame of new_tab.js. @estade: do you know why the theme.css href is changed on the DOMContentLoaded event? The initial href might have been loaded and applied when that happens, or it might not. Also, it seems the href is just modified to add a timestamp to the query part. Is that a hack to work around a caching issue? Would it be possible to set that url earlier than DOMContentLoaded?
,
Jan 19 2017
Sorry, I think you're actually guaranteed that it's loaded because it's script blocking. But then, why load the other stylesheet first?
,
Jan 19 2017
> I think you're actually guaranteed that it's loaded because it's script blocking. But then, why load the other stylesheet first? I dunno, lots of people have worked on this over a long time. The theming stuff was carried over from some previous version of the ntp (which is now the apps page). It does seem like we should be able to remove that href in new_tab.html. Maybe it used to be listening for a "load" event and needed that initial css reference to avoid a FOUC? > Is that a hack to work around a caching issue? yes, I didn't write that but it's sorta documented here: https://cs.chromium.org/chromium/src/chrome/browser/resources/ntp4/incognito_tab.html?rcl=0&l=14
,
Jan 20 2017
Review in progress: https://codereview.chromium.org/2647653002/
,
Jan 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/babe852aafe34f62d5ac8fbfc252830b64eebeea commit babe852aafe34f62d5ac8fbfc252830b64eebeea Author: rune <rune@opera.com> Date: Sat Jan 21 09:40:45 2017 Initially load new_tab.css with a timestamp for chrome://apps. This is what incognito_tab.html already does. Changing the url to add a timestamp query on DOMContentLoaded caused a FoUC. BUG= 681495 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2647653002 Cr-Commit-Position: refs/heads/master@{#445285} [modify] https://crrev.com/babe852aafe34f62d5ac8fbfc252830b64eebeea/chrome/browser/resources/ntp4/new_tab.html [modify] https://crrev.com/babe852aafe34f62d5ac8fbfc252830b64eebeea/chrome/browser/resources/ntp4/new_tab.js
,
Jan 22 2017
,
Jan 22 2017
,
Jan 22 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 23 2017
Please merge your change to M57 branch 2987 ASAP. So we can pick it for Dev release on Tuesday (02/24).
,
Jan 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1db7914d0f409e02b43ef2b2ad0a5a99e048c588 commit 1db7914d0f409e02b43ef2b2ad0a5a99e048c588 Author: Rune Lillesveen <rune@opera.com> Date: Mon Jan 23 09:36:29 2017 Initially load new_tab.css with a timestamp for chrome://apps. This is what incognito_tab.html already does. Changing the url to add a timestamp query on DOMContentLoaded caused a FoUC. BUG= 681495 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2647653002 Cr-Commit-Position: refs/heads/master@{#445285} (cherry picked from commit babe852aafe34f62d5ac8fbfc252830b64eebeea) Review-Url: https://codereview.chromium.org/2650683002 . Cr-Commit-Position: refs/branch-heads/2987@{#18} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/1db7914d0f409e02b43ef2b2ad0a5a99e048c588/chrome/browser/resources/ntp4/new_tab.html [modify] https://crrev.com/1db7914d0f409e02b43ef2b2ad0a5a99e048c588/chrome/browser/resources/ntp4/new_tab.js
,
Jan 24 2017
Verified the fix on windows-7, Mac 10.12.2 and Linux Ubuntu-14.04 using Chrome dev M57 #57.0.2987.8 as per the comment #0. Not observed Unwanted 'Theme created by.' tooltip on reloading page as expected. Hence, the fix is working as expected. Attaching the screen-cast for reference Adding the verified labels. Thanks...!! |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by hdodda@chromium.org
, Jan 16 2017Labels: hasbisect-per-revision
Owner: r...@opera.com
Status: Assigned (was: Unconfirmed)