Closed Bug 1337492 Opened 8 years ago Closed 8 years ago

Late loading of identity icon makes Firefox feel less responsive

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- fixed

People

(Reporter: phlsa, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 file)

Some hover effects on buttons in Firefox seem to load the hover graphic only once the mouse moves over the button. That leads to a white flash when hovering a button for the first time. The most prominent example is the i-button in the URL bar, but I can swear that I've seen this in a few other places as well. STR: - Quit Firefox - Start Firefox - Move the mouse over the i-icon in the URL bar Expected result: - The hover state gets shown immediately Actual result: - There is a short flash where the icon disappears before the hover graphic is loaded. This flash lasts even longer if the machine is busy (e.g. while loading Activity Stream)
Component: Toolbars and Customization → Theme
Jonathan, I *believe* (but I could be wrong!) that this is specific to icons that use SVG. Is that possible? Do we load SVG differently from PNG (in particular, is loading not sync once we reflow for the :hover effect, like it seems to be for png files?)
Flags: needinfo?(jwatt)
I can't think of any difference in the RasterImage vs. VectorImage code paths that would cause this. Maybe seth is still active enough that he responds to needinfo's.
Flags: needinfo?(jwatt) → needinfo?(seth.bugzilla)
Does anyone know where the image resource containing the i-icon in the URL bar is in the repository? Looking at that might shed some light on the issue.
(In reply to Mike Conley (:mconley) from comment #4) > Yep, right here: > http://searchfox.org/mozilla-central/rev/ > ac8a72f2d5204ced2004c93a9c193430c63a6a71/browser/themes/shared/identity- > block/identity-icon.svg Ni'ing per comment #3 :-)
Flags: needinfo?(jwatt)
Perfect, thanks. The |use:not(:target)| trick is something I remember suggesting a long time ago that *could* be optimized to make it faster to render different icons by placing them in a single file. (The idea was that we could optimize the implementation internally so that a single SVG document would be shared between different icons.) However, this optimization does not exist yet (and would require a fair bit of work). Currently we create a new VectorImage (and therefore SVG document) for each URL with a different query string. So by using the |use:not(:target)| trick this setup means that the icons pay the cost of having a lot more DOM content and styling that won't be displayed without any of the benefits that might come if the optimization was implemented. In other words I'd expect the SVG to take noticeably longer to render on hover than an equivalent static raster image. I'd suggest separating the file out into 12 different .svg files and see if that helps. (Or at least try an experiment with two SVG files for the default and default-hovered icons.)
Flags: needinfo?(seth.bugzilla)
Flags: needinfo?(jwatt)
Blocks: 1358998
I'm making this bug specific to fixing the identity icon as discussed in bug 1358998.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Summary: Late loading of button hover states makes Firefox feel less responsive → Late loading of identity icon makes Firefox feel less responsive
Marking this as an improvement to perceived performance.
Whiteboard: [photon-performance]
Flags: qe-verify?
Priority: -- → P1
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Depends on: 1365846
Comment on attachment 8870040 [details] Bug 1337492 - Split up identity icon into SVG files to avoid slow load on hover. https://reviewboard.mozilla.org/r/141556/#review145156 ::: browser/themes/shared/identity-block/identity-icon-hover.svg:10 (Diff revision 1) > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > + <defs> > + <path id="glyph-hover" d="M102,179a7,7,0,1,1-7,7A7,7,0,0,1,102,179Zm0,3a1,1,0,1,1-1,1A1,1,0,0,1,102,182Zm0,3a1,1,0,0,1,1,1v3a1,1,0,0,1-2,0v-3A1,1,0,0,1,102,185Z" transform="translate(-94 -178)"/> > + </defs> > + > + <use fill="context-fill" fill-rule="evenodd" href="#glyph-hover"/> You don't need <use> here, just move <path> outside of <defs> and add the fill and fill-rule attributes.
Comment on attachment 8870040 [details] Bug 1337492 - Split up identity icon into SVG files to avoid slow load on hover. https://reviewboard.mozilla.org/r/141556/#review145156 > You don't need <use> here, just move <path> outside of <defs> and add the fill and fill-rule attributes. Good point, thanks, for some reason I thought that this had not worked in a previous patch.
I'm not sure about a good strategy to verify this is actually fixed, since the flicker seems to be pretty intermittent, at least for me. I couldn't see it after applying the patch, but that could just be placebo. If someone has a good idea or is able to reproduce it consistently right now let me know. In any case, even if the issue isn't fixed, the patch is still strictly an improvement.
Whiteboard: [photon-performance] → [reserve-photon-performance]
Comment on attachment 8870040 [details] Bug 1337492 - Split up identity icon into SVG files to avoid slow load on hover. https://reviewboard.mozilla.org/r/141556/#review145526 Looks good to me. ::: browser/themes/shared/identity-block/identity-icon-hover.svg:1 (Diff revision 2) > +<?xml version="1.0" encoding="utf-8"?> You don't need this XML declaration line, so please remove it. Same in the other three files. ::: browser/themes/shared/identity-block/identity-icon-hover.svg:6 (Diff revision 2) > +<?xml version="1.0" encoding="utf-8"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > + <path fill="context-fill" fill-rule="evenodd" d="M102,179a7,7,0,1,1-7,7A7,7,0,0,1,102,179Zm0,3a1,1,0,1,1-1,1A1,1,0,0,1,102,182Zm0,3a1,1,0,0,1,1,1v3a1,1,0,0,1-2,0v-3A1,1,0,0,1,102,185Z" transform="translate(-94 -178)"/> We can easily apply the translation to the path data in the markup rather than have the browser apply it dynamically every time it's loaded. If you don't do that here can you file a follow-up bug, please? (shorlander has an Illustrator post-processor to do this.) Again, this comment applies to the other three files too.
Attachment #8870040 - Flags: review?(jwatt) → review+
Comment on attachment 8870040 [details] Bug 1337492 - Split up identity icon into SVG files to avoid slow load on hover. https://reviewboard.mozilla.org/r/141556/#review145526 > We can easily apply the translation to the path data in the markup rather than have the browser apply it dynamically every time it's loaded. If you don't do that here can you file a follow-up bug, please? (shorlander has an Illustrator post-processor to do this.) Again, this comment applies to the other three files too. Got the updated SVGs from shorlander, thanks :)
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30e9f1ac108c Split up identity icon into SVG files to avoid slow load on hover. r=jwatt
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.6 - May 29
I've tried to reproduce the initial issue on a build without the patch, but I'm not sure what exactly should I look for. Given the fact that this fix had more than enough time to bake in 55 and 56, and given Johann 's comment 13, I'm not sure how much sense it makes to have it as a qe+. Johann, thoughts?
Flags: needinfo?(jhofmann)
(In reply to Adrian Florinescu [:AdrianSV] from comment #19) > I've tried to reproduce the initial issue on a build without the patch, but > I'm not sure what exactly should I look for. Given the fact that this fix > had more than enough time to bake in 55 and 56, and given Johann 's comment > 13, I'm not sure how much sense it makes to have it as a qe+. Johann, > thoughts? I agree, this may be very hard to verify, especially at this point. I'll remove the flag, thanks. I'm confident that we improved the situation to a point where it's acceptable at the very least (I haven't had any flickering anymore).
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(jhofmann)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: