Closed
Bug 1403648
Opened 7 years ago
Closed 7 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)
Tracking
()
VERIFIED
FIXED
Firefox 60
People
(Reporter: florian, Assigned: florian)
References
Details
(Whiteboard: [reserve-photon-performance])
Attachments
(3 files, 3 obsolete files)
(deleted),
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•7 years ago
|
||
I don't think this has much to do with Windows per se; bug 1399454 is effective over here on Windows 10.
Assignee | ||
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P3
QA Contact: adrian.florinescu
Whiteboard: [photon-performance] [triage] → [reserve-photon-performance]
Updated•7 years ago
|
Summary: [Windows] The address bar should be focused before first paint → The address bar sometimes still gets focused after the first paint
Updated•7 years ago
|
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
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
Same as attachment 8949347 [details] [diff] [review], unbitrotted.
Attachment #8950591 -
Flags: review?(jhofmann)
Assignee | ||
Updated•7 years ago
|
Attachment #8949347 -
Attachment is obsolete: true
Attachment #8949347 -
Flags: review?(jhofmann)
Assignee | ||
Comment 8•7 years ago
|
||
This lets us get the about:home favicon painted at first paint.
Attachment #8950592 -
Flags: review?(jhofmann)
Assignee | ||
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8950592 -
Flags: review?(jhofmann) → review+
Comment 11•7 years ago
|
||
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+
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
bugherder |
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: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 14•7 years ago
|
||
I've backed this out for causing bug 1438622 and bug 1438749.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffbe7e6f0b22f102189a2eb7c38a336b4a6f394
https://hg.mozilla.org/integration/mozilla-inbound/rev/90e960e02981918a06ff08fc410db764ac8c6af7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adb6b91b758fc7e45bd0476a2fbadc913adeee0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
Updated•7 years ago
|
status-firefox60:
fixed → ---
Target Milestone: Firefox 60 → ---
Updated•7 years ago
|
Assignee | ||
Comment 17•7 years ago
|
||
The problem was the instanceof checks on Promise, with promises coming from another compartment.
Attachment #8951636 -
Flags: review?(jhofmann)
Assignee | ||
Updated•7 years ago
|
Attachment #8950591 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
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)
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8951724 -
Flags: review?(jhofmann)
Assignee | ||
Updated•7 years ago
|
Attachment #8951636 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8951724 -
Flags: review?(jhofmann) → review+
Comment 20•7 years ago
|
||
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.
Assignee | ||
Comment 21•7 years ago
|
||
(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.
Comment 22•7 years ago
|
||
bugherder |
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: 7 years ago → 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 23•7 years ago
|
||
(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*!
Comment 24•7 years ago
|
||
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.
Description
•