[non-e10s] twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: masayuki, Assigned: roc)

Tracking

Trunk
mozilla45
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox43+ verified, firefox44+ fixed, firefox45+ fixed, b2g-v2.5 fixed)

Details

(Whiteboard: Test with layout.interruptible-reflow.enabled=true and disabling e10s, )

Attachments

(5 attachments, 2 obsolete attachments)

STR #1:

1. Scroll down your timeline a lot.
2. Click a tweet which has one or more images.
3. Click an image in the tweet for zoom.
4. Close the image.

STR #2:

1. Scroll down your timeline a lot.
2. Click RT.
3. Click RT button on the confirmation dialog.

Actual result:

After 1-4 or 2-3, i.e., closing twitter's dialog, timeline scrolls up.

Expected result:

Even after closing twitter's dialog, timeline should keep the scroll position.


I'm not sure if this is a bug of us or twitter's bug. I cannot reproduce this bug with Google Chrome.

I've reproduce this bug at least for several months with Nightly.
This bug is a real annoyance - I'm on Ubuntu Linux 14.10 x86_64 & Firefox 33.0.

I have to switch to Chrome while using twitter. Chrome does not have this bug.
When user opens image, twitter.com sets overflow-y of <body> to hidden. And when user close the image dialog, twitter.com restores the overflow-y to scroll. However, I cannot reproduce this bug with simplest testcase which just hides and restores scrollbar of <body>. So, I guess that something doing for the dialog breaks the scroll position of <body>.


twitter_more_1.bundle.css:

> .modal-enabled,.grid-enabled,.overlay-enabled,.gallery-enabled{
> 	position:relative;
> 	overflow:hidden
> }


>   this.open = function (a, b) {
>     this.calculateScrollbarWidth(),
>     this.fromGrid = b && !!b.fromGrid,
>     this.title = b && b.title ? b.title : this.attr.defaultGalleryTitle,
>     this.select('galleryTitleSelector').text(this.title),
>     b && b.inOverlay ? this.$node.addClass(this.attr.inOverlayGalleryClass)  : this.$node.removeClass(this.attr.inOverlayGalleryClass),
>     b && b.showGrid && b.profileUser ? (this.select('gallerySelector').removeClass('no-grid'), this.select('gridSelector').attr('href', '/' + b.profileUser.screen_name + '/media'), this.select('gridSelector').find('.visuallyhidden').text(b.profileUser.name), this.select('gridSelector').addClass('js-nav'))  : (this.select('gallerySelector').addClass('no-grid'), this.select('gridSelector').removeClass('js-nav'));
>     var c = $(a.target).closest(this.attr.mediaSelector);
>     if (this.isOpen() || c.length == 0) return;
>     b && b.timelineSelector ? this.$timeline = c.closest(b.timelineSelector)  : this.$timeline = c.parent(),
>     imageResizer.resetMinSize(this.select('gallerySelector')),
>     this.render(c),
>     $('body').addClass('gallery-enabled'),
>     this.select('gallerySelector').addClass('show-controls'),
>     this.on('mousemove', function () {
>       this.select('gallerySelector').removeClass('show-controls')
>     }.bind(this)),
>     this.trigger('uiGalleryOpened')
>   },
>   this.closeGallery = function () {
>     $('body').hasClass('gallery-enabled') && ($('body').removeClass('gallery-enabled'), this.select('galleryMediaSelector').empty(), this.hideNav(), this.enableNav(!1, !1), this.off(window, 'resize', this.debouncedResize), this.off('mousemove'), delete this.$timeline, delete this.$current, this.trigger('uiGalleryClosed'), this.trigger('uiDialogClosed'))
>   },

Adding gallery-enabled class overwrites the overflow property.
Component: Untriaged → Layout
I can reproduce the problem(STR #1) on Nightly36.0a1, Firefox 31esr, 24esr, 17esr, 10esr as well as Firefox4, but not Firefox3.6.


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/b94cf8147ccd
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre ID:20100111140136
Bad:
http://hg.mozilla.org/mozilla-central/rev/47cd92d616d7
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100111 Minefield/3.7a1pre ID:20100111150802
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b94cf8147ccd&tochange=47cd92d616d7

Triggered by Bug 526394


In local build
Last Good: fada8c5cef07
First Bad: 2f7d76044ee8
Similar to Bug 691095
(In reply to Alice0775 White from comment #4)
> Similar to Bug 691095

Yeah, but Asa said that it's scrolled without user action. I guess that the bug has already been fixed by twitter.com side or our side.
roc:

The regression point is your bug. Do you have any ideas? Or, do you know somebody who can research this? This is really annoying bug for twitter uses who use Web UI...
Flags: needinfo?(roc)
I don't see this bug on trunk currently. Masayuki, do you still see it?
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> I don't see this bug on trunk currently. Masayuki, do you still see it?

Yeah, I can reproduce this on today's Nightly build too.

If you want some stack trace or something, I could provide it.
When this occurs, ScrollFrameHelper::ScrollToImpl() is called by ScrollFrameHelper::ReflowFinished().

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=0b743302cbfb#4532
> 4532   // Clamp current scroll position to new bounds. Normally this won't
> 4533   // do anything.
> 4534   nsPoint currentScrollPos = GetScrollPosition();
> 4535   ScrollToImpl(currentScrollPos, nsRect(currentScrollPos, nsSize(0, 0)));

Could be the position is already clamped or still overflow is hidden yet?
When it's scrolled up, each value is:

ScrollFrameHelper::ScrollToImpl():

aPt={ x=0, y=3703029 }

> 2355   nsPoint curPos = GetScrollPosition();

curPos={ x=0, y=3703029 }

> 2356   nsPoint alignWithPos = mScrollPosForLayerPixelAlignment == nsPoint(-1,-1)
> 2357       ? curPos : mScrollPosForLayerPixelAlignment;

alignWithPos={ x=0, y=3703029 }

> 2358   // Try to align aPt with curPos so they have an integer number of layer
> 2359   // pixels between them. This gives us the best chance of scrolling without
> 2360   // having to invalidate due to changes in subpixel rendering.
> 2361   // Note that when we actually draw into a PaintedLayer, the coordinates
> 2362   // that get mapped onto the layer buffer pixels are from the display list,
> 2363   // which are relative to the display root frame's top-left increasing down,
> 2364   // whereas here our coordinates are scroll positions which increase upward
> 2365   // and are relative to the scrollport top-left. This difference doesn't actually
> 2366   // matter since all we are about is that there be an integer number of
> 2367   // layer pixels between pt and curPos.
> 2368   nsPoint pt =
> 2369     ClampAndAlignWithLayerPixels(aPt,
> 2370                                  GetScrollRangeForClamping(),
> 2371                                  aRange,
> 2372                                  alignWithPos,
> 2373                                  appUnitsPerDevPixel,
> 2374                                  scale);

pt={ x=0, y=30660 }

> 2381   nsPoint dist(std::abs(pt.x - mLastUpdateImagesPos.x),
> 2382                std::abs(pt.y - mLastUpdateImagesPos.y));

mLastUpdateImagesPos={ x=-1, y=-1 }
dist={ x=1, y=30661 }

> 2383   nsSize scrollPortSize = GetScrollPositionClampingScrollPortSize();

scrollPortSize={ width=69240, height=50520 }

> 2384   nscoord horzAllowance = std::max(scrollPortSize.width / std::max(sHorzScrollFraction, 1),
> 2385                                    nsPresContext::AppUnitsPerCSSPixel());
> 2386   nscoord vertAllowance = std::max(scrollPortSize.height / std::max(sVertScrollFraction, 1),
> 2387                                    nsPresContext::AppUnitsPerCSSPixel());

horzAllowance=34620, vertAllowance=25260

> 2388   if (dist.x >= horzAllowance || dist.y >= vertAllowance) {
> 2389     needImageVisibilityUpdate = true;
> 2390   }
> 2391 
> 2392   if (needImageVisibilityUpdate) {
> 2393     presContext->PresShell()->ScheduleImageVisibilityUpdate();
> 2394   }
> 2395 
> 2396   // notify the listeners.
> 2397   for (uint32_t i = 0; i < mListeners.Length(); i++) {
> 2398     mListeners[i]->ScrollPositionWillChange(pt.x, pt.y);
> 2399   }
> 2400 
> 2401   nsPoint oldScrollFramePos = mScrolledFrame->GetPosition();

oldScrollFramePos={ x=0, y=-3703029 }

> 2402   // Update frame position for scrolling
> 2403   mScrolledFrame->SetPosition(mScrollPort.TopLeft() - pt);

mScrolledFrame->GetPosition()={ x=0, y=-30660 }


I still don't understand why the pt is strange point value.
aPt={ x=0, y=2894350 },
aRange={ x=0, y=2894350, XMost()=0, YMost()=2894350 },

appUnitsPerDevPixel=60,
scale={ width=1.000000, height=1.000000 },

curPos={ x=0, y=2894350 },

GetScrollRangeForClamping()={ x=0, y=0, XMost()=0, YMost()=43620 }(ShouldClampScrollPosition()=true),

alignWithPos={ x=0, y=2894350 },

pt={ x=0, y=43620 },
mLastUpdateImagesPos={ x=-1, y=-1 },

dist={ x=1, y=43621 },
scrollPortSize={ width=69240, height=50520 },

horzAllowance=34620, vertAllowance=25260,

oldScrollFramePos={ x=0, y=-2894350 },

mScrolledFrame->GetPosition()={ x=0, y=-43620 }



Looks like that GetScrollRangeForClamping() result causes computing wrong scroll destination.

Roc, any idea?

FYI: When it's not scrolled, looks like |if (pt == curPos)| is true and quit.
Flags: needinfo?(roc)
I still can't reproduce this.

It certainly looks like GetScrollRangeForClamping is at fault. Can you drill down further? What is aScrolledFrameOverflowArea in ScrollFrameHelper::GetScrolledRectInternal? I'm guessing its height is too small.

I wonder if this is due to interruptible reflow. Can you try disabling that by setting layout.interruptible-reflow.enabled to false?
Flags: needinfo?(roc)
Wow! Setting layout.interruptible-reflow.enabled to false

It seems to fix the problem for me.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150503030209
Hmm... Odd. I cannot reproduce this bug with the latest Nightly with default setting around layout. I'll check deeper more.
It is easy to reproduce the problem when e10s disabled. 
ex. page https://twitter.com/IE_Japan

I do not know whether the problem occurs when e10s enabled.
Excellent!! This bug can be reproduced only without e10s. And I confirmed not to reproduce this bug with layout.interruptible-reflow.enabled=false.

Roc, don't you enable e10s when you test this?
Flags: needinfo?(roc)
Summary: twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image → [non-e10s] twitter web UI (twitter.com) scrolls up unexpectedly at doing RT or showing image
Whiteboard: Test with layout.interruptible-reflow.enabled=true and disabling e10s
I had tested with e10s enabled. With e10s disabled, I can reproduce this.

So I guess an interruptible reflow occurs and we stop restoring the scroll position prematurely.
Flags: needinfo?(roc)
Posted file simple testcase
Load the page, and when it's finished loading click in the page and quickly move the mouse. The scroll position should shift from the end of the page to somewhere further up.
Assignee: nobody → roc
Bug 1092626. When a reflow is interrupted and a scroll position is clamped, try to restore to the old scroll position when the reflow completes. r=mats
Attachment #8684515 - Flags: review?(mats)
I don't have any good ideas for testing this...
I'm not sure I understand what the problem is.  After attachment 8684208 [details] is
fully loaded, is the expected result that there should be no scroll movement
when I click on the text?
The actual result I see in Linux builds is that the text moves due to a vertical
scroll (and that by just clicking - no mouse movement needed after the click).

I see that result also with the patches applied, and also with
layout.interruptible-reflow.enabled=false
Flags: needinfo?(roc)
Posted file updated testcase
Sorry, in an opt build the previous testcase completes too quickly to trigger a layout interrupt.

Try this testcase instead. Same steps as last time: wait for the page to finish loading and scroll to the bottom, then click in the body and move the mouse quickly. You should find that it scrolls to somewhere near the top.
Flags: needinfo?(roc)
Attachment #8684515 - Flags: review?(mats)
Comment on attachment 8684515 [details]
MozReview Request: Bug 1092626. When a reflow is interrupted and a scroll position is clamped, try to restore to the old scroll position when the reflow completes. r=mats

https://reviewboard.mozilla.org/r/24597/#review22141

r- since it doesn't fix the bug for me.

::: layout/generic/nsGfxScrollFrame.cpp:4758
(Diff revision 1)
> +    mRestorePos = currentScrollPos;

Thanks for the additional testcase.  I can now reproduce the reported problem.  Unfortunately though, applying both patches here doesn't fix the problem for me (Linux x86-64 m-i debug build).  I see that we hit this line but it just doesn't seem to help for some reason.

Also, why not simply use "mOuter->PresContext()->HasPendingInterrupt()" instead of propagating the aInterrupted flag? and possibly also NS_SUBTREE_DIRTY(mOuter) to avoid interrupts that didn't affect this subtree.

FWIW, I still see the issue when just clicking as described in comment 22. In this case it scrolls just 10px or so.  It might be related though.
Comment on attachment 8684514 [details]
MozReview Request: Bug 1092626. Add aInterrupted parameter to ReflowFinished. r=mats

https://reviewboard.mozilla.org/r/24595/#review22143

Maybe we can just use "mOuter->PresContext()->HasPendingInterrupt()" instead?
and/or NS_SUBTREE_DIRTY(mOuter).
Attachment #8684514 - Flags: review?(mats)
(In reply to Mats Palmgren (:mats) from comment #24)
> Thanks for the additional testcase.  I can now reproduce the reported
> problem.  Unfortunately though, applying both patches here doesn't fix the
> problem for me (Linux x86-64 m-i debug build).  I see that we hit this line
> but it just doesn't seem to help for some reason.

It definitely works for me. Can you look into why it isn't working for you?

> Also, why not simply use "mOuter->PresContext()->HasPendingInterrupt()"
> instead of propagating the aInterrupted flag? and possibly also
> NS_SUBTREE_DIRTY(mOuter) to avoid interrupts that didn't affect this subtree.

I don't want to create a dependency on exactly when the interrupt state is cleared. Only applying this change when NS_SUBTREE_DIRTY is true would be a good improvement though.
Flags: needinfo?(mats)
Sure, on the next call (after the interrupted reflow) to ReflowFinished,
ScrollToRestoredPosition() is called but it takes the early return
because mLastPos=-1,-1.  So if I add "mLastPos = GetScrollPosition()"
after the mRestorePos assignment in you patch, then it works as
expected.  I'm not happy with the result though, the scrollbar thumb
jumps very clearly from the end to the top and then back to the end.
In my debug build this takes about one second and is clearly visible.
I guess it's less of a problem in an optimized build but I think we
can do better.

If I *instead of* your part 2 patch add this as the first statement
in ReflowFinished():

  if (mRestorePos.x == -1 && aInterrupted && NS_SUBTREE_DIRTY(mOuter)) {
    return false;
  }

this also fixes the bug, and it looks much better - the scrollbar
thumb doesn't jump and the "paint flash" feels less disturbing.

Does that also work for you?
Flags: needinfo?(mats) → needinfo?(roc)
This also works:

  if (aInterrupted && NS_SUBTREE_DIRTY(mOuter)) {
    return false;
  }

I don't think we really care what mRestorePos is at the top of this
method.  A comment to go along with the code above would be:
// We will get another call after the next reflow and doing the
// scroll later is less janky.
BTW, just checking "NS_SUBTREE_DIRTY(mOuter)" also works for me.
I don't really see why we need to check aInterrupted at all.
All we need to know is that we will get another call after the
next reflow, right?
(In reply to Mats Palmgren (:mats) from comment #29)
> BTW, just checking "NS_SUBTREE_DIRTY(mOuter)" also works for me.
> I don't really see why we need to check aInterrupted at all.
> All we need to know is that we will get another call after the
> next reflow, right?

Yes, that sounds right.

I worry about the consequences of allowing the scroll position to be outside the scroll range, though. That breaks an invariant that could be important. Seems to me the scrollbar could be rendered completely incorrectly in this case and the user might even be able to interact with it and get surprising results. The fix that I posted here is conservative and doesn't break any invariants.

I guess we can go with your approach and see what happens. Do you want to post it, or should I?
Flags: needinfo?(roc)
OK, let's try that and see if it works.  It's almost bedtime here so it's faster
if you post it and make a Try run.  I can review it tomorrow.
Posted patch Alternative fix (obsolete) — Splinter Review
I pushed this to Try, let's see what falls out:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5d63d5c0103
Posted patch Alternative fix, v2 (obsolete) — Splinter Review
Whoa, plenty of failures in the above Try push.

I think the problem is that we need to always reset mPostedReflowCallback
before returning, so I moved that to the top.  New Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea34282db2e
Attachment #8685389 - Attachment is obsolete: true
... and here's a Try run with Opt builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=449a9bdf29ab
It looks OK to me; I think the failures are unrelated.

I also removed the NS_SUBTREE_DIRTY(mOuter) check later in the method
since it can't be true there.

I've tested that this patch fixes the problem locally in a Linux debug build.
Can you verify one of the Try builds fix it on Windows?
Attachment #8685526 - Attachment is obsolete: true
Attachment #8685697 - Flags: review?(roc)
Attachment #8685697 - Flags: review?(roc) → review+
(In reply to Mats Palmgren (:mats) from comment #34)
> I've tested that this patch fixes the problem locally in a Linux debug build.
> Can you verify one of the Try builds fix it on Windows?

Not easily, no, my Windows laptop is sick.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> (In reply to Mats Palmgren (:mats) from comment #34)
> > I've tested that this patch fixes the problem locally in a Linux debug build.
> > Can you verify one of the Try builds fix it on Windows?
> 
> Not easily, no, my Windows laptop is sick.

I can check it after meeting.
I tested briefly the trysever build in comment 34 on Windows (non-e10s, layout.interruptible-reflow.enabled is true), I don't see the reported problem on twitter.com.

Thank you very much!!!
Thanks for testing it Masayuki-san.
Masayuki-san, is the reported use case common when using Twitter?
Do you think it's worth uplifting?
Flags: needinfo?(masayuki)
(In reply to Mats Palmgren (:mats) from comment #40)
> Masayuki-san, is the reported use case common when using Twitter?

Yes. I don't see similar symptom in other websites.

> Do you think it's worth uplifting?

If the patch is not so risky, I'd like you to do that. I still see some tweets to complain about this bug. When they try to use Chrome, this bug could be a reason of switching their default browser.
Flags: needinfo?(masayuki)
https://hg.mozilla.org/mozilla-central/rev/f748eade3e29
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8685697 [details] [diff] [review]
Alternative fix, v3

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: scroll jank on Twitter timeline page
[Describe test coverage new/current, TreeHerder]: manual testing (testcase attached to this bug)
[Risks and why]: medium risk
[String/UUID change made/needed]: none

The reasons I think this might be worth an uplift:
 * Twitter is a popular site so the issue might affect many users
 * we're still early in the cycle(?)
Attachment #8685697 - Flags: approval-mozilla-beta?
Attachment #8685697 - Flags: approval-mozilla-aurora?
Comment on attachment 8685697 [details] [diff] [review]
Alternative fix, v3

Let's uplift this, as mats says it's high impact on users and we're still early in beta. This should make it into beta 3.
Attachment #8685697 - Flags: approval-mozilla-beta?
Attachment #8685697 - Flags: approval-mozilla-beta+
Attachment #8685697 - Flags: approval-mozilla-aurora?
Attachment #8685697 - Flags: approval-mozilla-aurora+
Tracking since we're uplifting, so I make sure this lands and sticks.
Flags: qe-verify+
Reproduced the initial issue using an old nightly from 2015-10-01, verified that the issue is fixed using Firefox 43 beta 3 build 2 across platforms (Windows 7 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 32-bit).
Status: RESOLVED → VERIFIED
Also verified using latest Developer Edition 44.0a2 and latest Nightly 45.0a1.
Status: VERIFIED → RESOLVED
Closed: 4 years ago4 years ago
Status: RESOLVED → VERIFIED
Forgot to drop qeverify as well...sorry for the noise.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.