Closed Bug 890897 Opened 9 years ago Closed 9 years ago

Defect - Overlay scrollbars trigger continual repaints on metro start page and irccloud.com

Categories

(Core :: Layout, defect, P2)

x86_64
Windows 8.1
defect

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- unaffected
firefox23 + verified
firefox24 + verified
firefox25 + verified
firefox-esr17 --- unaffected

People

(Reporter: jimm, Assigned: spohl)

References

Details

(Whiteboard: [lion-scrollbars+] [external])

Attachments

(1 file, 1 obsolete file)

Noticed this on the metro start page. Basically, the final fade out callback changes opacity on the scrollbars, which triggers a replow, which in turn triggers a call to ScrollbarActivity::ActivityOccurred() via nsGfxScrollFrameInner::CurPosAttributeChanged(), which starts the fade again. 

If you put a debug print statement in the refresh driver, you can see the behavior. Not sure if this reproduces on osx, but it does in metrofx on Win8.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #772038 - Flags: feedback?(mstange)
Blocks: 872780
This seems to match what is being observed in bug 870917. Would you happen to know why (and how) the current position changes when the fade out completes? That doesn't seem right to me.
Depends on: 870917
Here's the stack. Looks like we always call CurPosAttributeChanged from within nsGfxScrollFrameInner::ReflowFinished.

xul.dll!mozilla::layout::ScrollbarActivity::ActivityOccurred() Line 54	C++
xul.dll!nsGfxScrollFrameInner::CurPosAttributeChanged(nsIContent * aContent=0x0a5563a8) Line 2948	C++
xul.dll!nsGfxScrollFrameInner::ReflowFinished() Line 3604	C++
xul.dll!PresShell::HandlePostedReflowCallbacks(bool aInterruptible=true) Line 3703	C++
xul.dll!PresShell::DidDoReflow(bool aInterruptible=true, bool aWasInterrupted=false) Line 7667	C++
xul.dll!PresShell::ProcessReflowCommands(bool aInterruptible=false) Line 8002	C++
xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush={...}) Line 3897	C++
xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch=1373290380164175, mozilla::TimeStamp aNowTime={...}) Line 1185	C++
xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver=0x05624e88, __int64 jsnow=1373290380164175, mozilla::TimeStamp now={...}) Line 172	C++
xul.dll!mozilla::RefreshDriverTimer::Tick() Line 163	C++
xul.dll!nsTimerImpl::Fire() Line 543	C++
xul.dll!nsTimerEvent::Run() Line 629	C++
So nsGfxScrollFrameInner::SetCoordAttribute (called via nsGfxScrollFrameInner::FinishReflowForScrollbar in nsGfxScrollFrameInner::ReflowFinished) checks whether or not the attribute actually changed, but that doesn't seem to propagate out and CurPosAttributeChanged is called regardless.

This raises two questions:
1. Does the "current position" attribute actually change?
2. Why does this work in most cases, but not in the situation described here or in bug 870917?

One thing to point out is that at the end of the fade out we set the scrollbar visibility to 'hidden' in nativescrollbars.css:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/nativescrollbars.css#26
I wonder if that has anything to do with it.
If we keep this, I need to replace the ActivityOccurred call in HandleEvent to call ActivityStarted/ActivityStopped directly. In the real world missing a mousemove input event would never happen but it could in tests.
(In reply to Stephen Pohl [:spohl] from comment #4)
> This raises two questions:
> 1. Does the "current position" attribute actually change?
> 2. Why does this work in most cases, but not in the situation described here
> or in bug 870917?

I think we need to answer these questions before we take a patch. I don't know the answers, unfortunately.
This causes a .5 - 1% cpu drain while sitting idle on the startpage on light weight tablets, so it blocks v1 release.
Priority: -- → P1
No longer blocks: 872780
Adding lion-scrollbars+ keyword for tracking and to make it appear on our overlay scrollbar triage page:
https://wiki.mozilla.org/Lion_Scrollbars/Triage
Whiteboard: [lion-scrollbars+]
Summary: Overlay scrollbars trigger continual repaints despite having completed a fade out → Defect - Overlay scrollbars trigger continual repaints despite having completed a fade out
Whiteboard: [lion-scrollbars+] → [lion-scrollbars+] feature=defect c=tbd u=tbc p=0
Status: NEW → ASSIGNED
Blocks: metrov1it11
No longer blocks: metrov1defect&change
QA Contact: jbecerra
Whiteboard: [lion-scrollbars+] feature=defect c=tbd u=tbc p=0 → [lion-scrollbars+] feature=defect c=tbd u=tbc p=3
Attachment #772038 - Flags: feedback?(mstange)
Assignee: jmathies → nobody
Removing based on Jim's comment, "spohl mentioned he was looking at it in the parent bug."
Blocks: metrov1defect&change
No longer blocks: metrov1it11
Status: ASSIGNED → NEW
Whiteboard: [lion-scrollbars+] feature=defect c=tbd u=tbc p=3 → [lion-scrollbars+] feature=defect c=tbd u=tbc p=0
Priority: P1 → --
QA Contact: jbecerra
Whiteboard: [lion-scrollbars+] feature=defect c=tbd u=tbc p=0 → [lion-scrollbars+] [external]
Attached patch PatchSplinter Review
This fixes the issue on irccloud.com. However, I have a feeling that this patch might not fix the problem reported here. Jim, would you mind applying this patch and letting me know what's happening? Thanks!
Attachment #775682 - Flags: feedback?(jmathies)
Comment on attachment 775682 [details] [diff] [review]
Patch

Fixes the refresh problem for me.
Attachment #775682 - Flags: feedback?(jmathies) → feedback+
Summary: Defect - Overlay scrollbars trigger continual repaints despite having completed a fade out → Defect - Overlay scrollbars trigger continual repaints on metro start page and irccloud.com
Comment on attachment 775682 [details] [diff] [review]
Patch

Thanks Jim!
Attachment #775682 - Flags: review?(roc)
Attachment #772038 - Attachment is obsolete: true
Jim, could you try today's mozilla-central nightly (which contains Stephen's patch for bug 870917)?  I suspect it might fix this bug, all by itself.
Flags: needinfo?(jmathies)
(In reply to Steven Michaud from comment #13)
> Jim, could you try today's mozilla-central nightly (which contains Stephen's
> patch for bug 870917)?  I suspect it might fix this bug, all by itself.

I did this morning, it didn't.
Flags: needinfo?(jmathies)
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Comment on attachment 775682 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Scrollbars can flicker continuously on some websites when we refuse to scroll because we're already in an acceptable place.
Testing completed (on m-c, etc.): Extensive testing locally.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #775682 - Flags: approval-mozilla-beta?
Attachment #775682 - Flags: approval-mozilla-aurora?
This bug solves an aspect that was first reported in bug 870917, which was tracking for beta. I think we'd want this in FF23 as well.
https://hg.mozilla.org/mozilla-central/rev/02f261f13de5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: metrov1it11
No longer blocks: metrov1defect&change
Priority: -- → P2
QA Contact: jbecerra
Attachment #775682 - Flags: approval-mozilla-beta?
Attachment #775682 - Flags: approval-mozilla-beta+
Attachment #775682 - Flags: approval-mozilla-aurora?
Attachment #775682 - Flags: approval-mozilla-aurora+
Keywords: verifyme
FF metro isn't available only in nightly 25 ?
I don't see any differences on metro builds from 2013-07-08 and 2013-07-31, but
(In reply to Jim Mathies [:jimm] from comment #0)
> If you put a debug print statement in the refresh driver, you can see the
> behavior. Not sure if this reproduces on osx, but it does in metrofx on Win8.
I don't know how to do this, so I'm going to need further instructions in order to verify this bug.
(In reply to Paul Silaghi [QA] from comment #21)
> I don't see any differences on metro builds from 2013-07-08 and 2013-07-31,

Jim, do you have any good advice for Paul here how to reproduce the issue on metro?
Flags: needinfo?(jmathies)
(In reply to Stephen Pohl [:spohl] from comment #22)
> (In reply to Paul Silaghi [QA] from comment #21)
> > I don't see any differences on metro builds from 2013-07-08 and 2013-07-31,
> 
> Jim, do you have any good advice for Paul here how to reproduce the issue on
> metro?

In debug builds you could turn on nspr nsRefreshDriver:1 logging.

I just checked in the debugger and can verify this is fixed for metrofx tip.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jmathies)
Thanks Stephen, Jim.
Keywords: verifyme
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.