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)
Firefox
Theme
Tracking
()
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)
Updated•8 years ago
|
Component: Toolbars and Customization → Theme
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
Marking this as an improvement to perceived performance.
Blocks: photon-performance-triage
Whiteboard: [photon-performance]
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P1
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Updated•8 years ago
|
Blocks: photon-perf-upforgrabs
Updated•8 years ago
|
No longer blocks: photon-performance-triage
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 13•8 years ago
|
||
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.
Updated•8 years ago
|
Whiteboard: [photon-performance] → [reserve-photon-performance]
Comment 14•8 years ago
|
||
mozreview-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
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 hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
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 :)
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•8 years ago
|
Iteration: --- → 55.6 - May 29
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
(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.
Description
•