1766284 - browser_click_bookmarks_on_toolbar.js keeps vsync enabled

Status: NEW

(Core :: Layout, defect)

Description

[Florian Quèze [:florian]]

The browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js test keeps vsync enabled at the end of its run.

Here is what the test does that triggers it:

• bookmark about:robots (a page with an animated favicon) and show the bookmarks toolbar.
• hide the toolbar (the toolbar is hidden with the "collapsed" attribute set to "true" in https://searchfox.org/mozilla-central/rev/13d69189a8abfc5064fe44944550b9b6eb4544f5/browser/base/content/browser.js#6650)

To make this issue produce a test failure, one needs to apply the patches from bug 1742842.

Note: the patches in bug 560067 are not enough to fix this bug.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP: Bug 1766284 - Test that vsync is disabled after hiding the bookrmarks toolbar containing animated favicons,

Comment 2

[Florian Quèze [:florian]]

I attached a browser-chrome mochitest that helps check quickly if this bug is present.

This bug feels very similar to bug 560067, but the patches you had there don't fix it, so I filed a new bug. Is it an edge case that wasn't handled in these patches, or an entirely different bug? In bug 560067 the animated image was hidden using visibility: hidden, here it's visibility: collapse;.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

This isn't fixed by those patches because those bookmarks are using <xul:image> rather than <html:img> and XUL images don't track visibility like everything else. So I'd much rather make the bookmark toolbars favicons <html:img> like in the tab bar (https://phabricator.services.mozilla.com/D137746).

Comment 4

[Florian Quèze [:florian]]

Thanks. I'm not sure if we should attempt to replace the xul images with html img specifically for the bookmarks toolbar, or if we should have the xul image replaced for all toolbarbuttons (in which case this is likely blocked by bug 1584641, which doesn't look very active right now). Marco, any opinion about whether replacing the image tags for the bookmarks toolbar specifically would make sense?

Also, I wonder if animated favicons are actually desirable in the bookmarks toolbar. This seems needlessly distracting to me. I wonder if there might be a way to force the animation to not play when the favicon is used in the context of bookmarks.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Bug 1584641 should be pretty fixable now, fwiw.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] from comment #4)

Thanks. I'm not sure if we should attempt to replace the xul images with html img specifically for the bookmarks toolbar, or if we should have the xul image replaced for all toolbarbuttons (in which case this is likely blocked by bug 1584641, which doesn't look very active right now). Marco, any opinion about whether replacing the image tags for the bookmarks toolbar specifically would make sense?

I can't easily answer that question without more data. Are there other toolbarbuttons that are commonly shown using xul:image? If so, it may be worth to do the whole replacement... otherwise if in 80% of the cases it's the bookmarks toolbar alone, it's probably worth doing it first.
It also depends on how complex Bug 1584641 is, Emilio suggests it's feasible, but I'm not sure about potential issues/regressions.

Also, I wonder if animated favicons are actually desirable in the bookmarks toolbar.

Do we show animated favicons? Afair we always convert to png, unless it's an svg. The favicons service only supports those 2 formats. If you have evidence of the opposite it would be useful to have examples.

Comment 7

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [:mak] from comment #6)

Do we show animated favicons? Afair we always convert to png, unless it's an svg. The favicons service only supports those 2 formats. If you have evidence of the opposite it would be useful to have examples.

The favicon of about:robots. It looks like it's an animated PNG image.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] from comment #7)

The favicon of about:robots. It looks like it's an animated PNG image.

Oh! thanks, I filed Bug 1770771.