Closed Bug 1403648 Opened 2 years ago Closed 2 years ago

The address bar sometimes still gets focused after the first paint and the favicon of about:home flickers

Categories

(Firefox :: Address Bar, defect, P3)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox58 --- wontfix
firefox60 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(3 files, 3 obsolete files)

Bug 1399454 improved the situation quite a bit, and I have verified on my macbook that the urlbar was focused there at first paint, but when testing on the quantum reference hardware on Windows 10, I see the location bar gets focused about 10 frames after first paint of the chrome UI.

I think the problem is that http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/base/content/browser.js#1630 is waiting for a promise resolution, and that delays execution. When I reviewed the patch I thought that would run before first paint, but apparently it doesn't, at least on Windows.
I don't think this has much to do with Windows per se; bug 1399454 is effective over here on Windows 10.
(In reply to Dão Gottwald [::dao] from comment #1)
> I don't think this has much to do with Windows per se; bug 1399454 is
> effective over here on Windows 10.

I can also reproduce on my high end Dell XPS 13", but it's unfocused there for only 3-4 frames (instead of 10-12 on the quantum reference hardware).

I think this depends on how much event loop spinning happens between processing the onload event and first paint. I would expect that to have some platform specific component, and also depend on computer speed.
(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> I can also reproduce on my high end Dell XPS 13"

With that same computer booted on Ubuntu instead of Windows 10, I can still reproduce but it's harder to spot. During startup it takes 9 frames to focus the urlbar (but that's barely visible, due to a much bigger flickering of the whole window that isn't created at the right size initially). When opening new windows (ie. not startup), the urlbar also isn't focused before first paint, but it is focused before the end of the window opening animation. It typically only takes one or two frames before the urlbar is focused.
Flags: qe-verify+
Priority: -- → P3
QA Contact: adrian.florinescu
Whiteboard: [photon-performance] [triage] → [reserve-photon-performance]
Summary: [Windows] The address bar should be focused before first paint → The address bar sometimes still gets focused after the first paint
Blocks: 1421456
Summary: The address bar sometimes still gets focused after the first paint → The address bar sometimes still gets focused after the first paint and the favicon of about:home flickers
I have a work in progress patch for this. Focusing the urlbar before first paint causes us to load UnifiedComplete.js before first paint, and so makes browser_startup.js angry. Bug 1371610 was already there to lazy load UnifiedCommplete.js, marking it as a dependency.
Depends on: 1371610
Attached patch Patch (obsolete) — Splinter Review
I think this patch fixes the main problem here. Unfortunately the result isn't as good as I was hoping.

On Try, it seems the urlbar flicker is fixed on Mac and Windows but not on Linux (where I had to keep the exception in browser_startup_flicker.js to have green try runs).

Locally, the bug seems fixed on Windows on the quantum ref hardware (I verified with a screen recording), but on my Mac it isn't fixed. Profiling what happens on the Mac shows that we fire an "activate" event right after the window has been first painted, and the location bar focus ring isn't painted in inactive windows. I think this is a separate bug that shouldn't block this patch.

Try push that's green for the performance tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=236d69c4f87161411016e2c94d6138c76912983a
Try push for all mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=976ac7993e371f32cf44e938f9b988426f993a2a
Attachment #8949347 - Flags: review?(jhofmann)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Same as attachment 8949347 [details] [diff] [review], unbitrotted.
Attachment #8950591 - Flags: review?(jhofmann)
Attachment #8949347 - Attachment is obsolete: true
Attachment #8949347 - Flags: review?(jhofmann)
This lets us get the about:home favicon painted at first paint.
Attachment #8950592 - Flags: review?(jhofmann)
This gets rid of the urlbar history dropmarker flicker. This flicker is caused by the CSS transition: if CSS is flushed even once before we select the urlbar, the CSS transition will occur. I tried to get rid of all the style flushes that occur before we select the urlbar and ended up getting nowhere... because calling .focus on the urlbar actually triggers a flush.

The best workaround I've found is to set the 'focused' attribute on the urlbar by default, and remove it before first paint for the cases where we aren't sure the urlbar needs to be focused. In my local testing this didn't seem to cause flicker for the case where the urlbar isn't focused, but ymmv I guess.

This is a hack, but I think it's worth it, and try seems to agree: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca3e510bb0df5b28dfaf8e1fc6b1183fa012542d
Attachment #8950595 - Flags: review?(jhofmann)
Comment on attachment 8950591 [details] [diff] [review]
part1 - focus the urlbar without waiting for a promise

Review of attachment 8950591 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really not a big fan of the whole "maybe it's async" API, but I can't come up with a better way to do this, so I'll not stand in the way of progress. Would be great to find a different way to do this at some point...

::: browser/base/content/browser.js
@@ +1771,1 @@
>        });

Can this be shortened to:

return willOverride.then(willOverrideHomepage => willOverrideHomepage ? null : uri)

?
Attachment #8950591 - Flags: review?(jhofmann) → review+
Attachment #8950592 - Flags: review?(jhofmann) → review+
Comment on attachment 8950595 [details] [diff] [review]
part 3 - set the 'focused' attribute by default

Review of attachment 8950595 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!
Attachment #8950595 - Flags: review?(jhofmann) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23ac3f571cf
focus the urlbar at first paint without waiting for a promise to resolve in most cases, r=johannh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03c60285f5d
set urlbar focus and about:home favicon in the DOMContentLoaded handler to reduce window open flicker, r=johannh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9833d3aa5451
set the 'focused' attribute on the urlbar by default, and remove it when we are unsure, to avoid flickering of the urlbar-history-dropmarker, r=johannh.
https://hg.mozilla.org/mozilla-central/rev/f23ac3f571cf
https://hg.mozilla.org/mozilla-central/rev/c03c60285f5d
https://hg.mozilla.org/mozilla-central/rev/9833d3aa5451
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1438622
Depends on: 1438749
When this landed (comment 12), these perf regressions were noticed:

== Change summary for alert #11582 (as of Wed, 14 Feb 2018 19:22:38 GMT) ==

Regressions:

 72%  tp5o responsiveness windows7-32 pgo e10s stylo     3.69 -> 6.36
 14%  tp5o responsiveness windows10-64 opt e10s stylo    2.08 -> 2.38
  9%  tp5o responsiveness windows10-64 pgo e10s stylo    1.91 -> 2.09
  5%  sessionrestore_no_auto_restore osx-10-10 opt e10s stylo660.67 -> 694.33
  4%  tp5o responsiveness windows7-32 opt e10s stylo     2.12 -> 2.20

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11582
Target Milestone: Firefox 60 → ---
The problem was the instanceof checks on Promise, with promises coming from another compartment.
Attachment #8951636 - Flags: review?(jhofmann)
Attachment #8950591 - Attachment is obsolete: true
Comment on attachment 8951636 [details] [diff] [review]
part1 focus the urlbar without waiting for a promise, v3

There are other problems caused by this version of the patch, removing review flag for now.
Attachment #8951636 - Flags: review?(jhofmann)
Attachment #8951636 - Attachment is obsolete: true
Attachment #8951724 - Flags: review?(jhofmann) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0435bcb13e06
focus the urlbar at first paint without waiting for a promise to resolve in most cases, r=johannh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e992be21e7e9
set urlbar focus and about:home favicon in the DOMContentLoaded handler to reduce window open flicker, r=johannh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/66815ce7cfc9
set the 'focused' attribute on the urlbar by default, and remove it when we are unsure, to avoid flickering of the urlbar-history-dropmarker, r=johannh.
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #15)
> When this landed (comment 12), these perf regressions were noticed:
> 
> == Change summary for alert #11582 (as of Wed, 14 Feb 2018 19:22:38 GMT) ==
> 
> Regressions:
> 
>  72%  tp5o responsiveness windows7-32 pgo e10s stylo     3.69 -> 6.36
>  14%  tp5o responsiveness windows10-64 opt e10s stylo    2.08 -> 2.38
>   9%  tp5o responsiveness windows10-64 pgo e10s stylo    1.91 -> 2.09
>   5%  sessionrestore_no_auto_restore osx-10-10 opt e10s stylo660.67 -> 694.33
>   4%  tp5o responsiveness windows7-32 opt e10s stylo     2.12 -> 2.20
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=11582

The tp5o responsiveness regressions seem unrelated. The sessionrestore_no_auto_restore regression was mine. Bisecting on try showed that it was part 3 that was causing this regression. With an additional requestAnimationFrame to reduce the style invalidations caused by adding/removing the 'focused' attribute, the regression seems almost gone on windows, and on Mac I'm winning on tpaint what I'm losing on sessionrestore_no_auto_restore:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f8231d5d8a658bd2b149723e0c1dc600712bd4d1&newProject=try&newRevision=baea9fb7c25bf900f7753471bce9a902b2295dfa&framework=1

I think we'll just have to accept the sessionrestore_no_auto_restore regression.

I spent several hours looking at profiles of sessionrestore_no_auto_restore, and I have doubts about it measuring what it actually intends:
- 9% of it is code from session-restore-test-2@mozilla.org/SessionRestoreTalosTest.js
- we pay at least twice the cost of creating a new content process synchronously (bug 1348361)
- this test measures something async, with random other async things sometimes executing in the middle. So it's not exactly clear what we are measuring... and as long as we are responsive to user events, it's not clear that this session restore task ending sooner has any user impact.
https://hg.mozilla.org/mozilla-central/rev/0435bcb13e06
https://hg.mozilla.org/mozilla-central/rev/e992be21e7e9
https://hg.mozilla.org/mozilla-central/rev/66815ce7cfc9
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
(In reply to Florian Quèze [:florian] from comment #21)
> The tp5o responsiveness regressions seem unrelated.

Indeed, the tp5o values don't look too stable. They seem to have returned to normal values.
I'm marking them as invalid.
Thanks for working on perf improvements for sessionrestore*!
Backout from comment 4 resulted in these improvements:

== Change summary for alert #11624 (as of Fri, 16 Feb 2018 06:28:00 GMT) ==

Improvements:

  7%  tp5o responsiveness windows10-64 pgo e10s stylo     2.08 -> 1.93
  6%  sessionrestore_no_auto_restore osx-10-10 opt e10s stylo694.04 -> 651.25
  4%  ts_paint_webext windows7-32 pgo e10s stylo          644.54 -> 621.00
  2%  ts_paint osx-10-10 opt e10s stylo                   773.21 -> 755.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11624
I verified this issue on Mac OS X 10.12, Ubuntu 16.04, Windows 10 with FF Nightly 61.0a1(2018-03-12) and I can't reproduce this issue anymore. I also verified this issue on Nightly 60.0a1(2017-09-27) and I could not reproduce it. Based on the the results mentioned above, I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
OS: Unspecified → Windows 10
NOTE: Sorry I wrote 60.0a1(2017-09-27) instead of the right version 57.0a1(2017-09-27). Please note that I also tested with FF Nightly 60.0a1(2018-03-12).
You need to log in before you can comment on or make changes to this bug.