1081072 - Flex container with "overflow-y: auto" is jumping when shown, with fading overlay scrollbars

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Oleg Zasypkin [:azasypkin]]

Attachment: test-case.html

In bug 1074030, the following prefs were changed:

* pref("ui.scrollbarFadeBeginDelay", 450);
* pref("ui.scrollbarFadeDuration", 200);

This revealed weird behaviour of flex container with "overflow-y: auto": when such container is hidden initially and then shown - its content is moving a bit in the flex-direction (for "flex-direction: column" content is moved from top to bottom, for "flex-direction: row" it's moved from left to right).

To reproduce it, just open attached test case in B2G browser app and tap on Button several times and see how content behaves. The issue is reproduced on the oldest v2.0 Flame build I have (with patch from bug 1074030).

Comment 1

[Oleg Zasypkin [:azasypkin]]

[Blocking Requested - why for this release]: Some major UI elements look broken (eg. recipients input)

In Gaia/Messages we use such overflow&flex combination quite intensively so several major things look broken. Setting v2.1? as patch for bug 1074030 was uplifted to v2.1 too.

Comment 2

[Oleg Zasypkin [:azasypkin]]

Hey guys, 

Could you please help with that issue?

Thanks!

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I don't know anything about the flexbox implementation, but I can reproduce the problem. The content shifting seems to be related to the start of the scrollbar fade - if I increase the ui.scrollbarFadeBeginDelay pref then it takes longer for the content to shift, but that's not the case if I increase the ui.scrollbarFadeDuration pref.

Also, I tried all combinations of overflow-x and overflow-y, and it seems that in your example (where the content shifts horiztonally) it only happens if overflow-x is set to auto or scroll. The value of overflow-y doesn't matter. If I change the flex-direction to column, the entire box gets a little taller if the overflow-y is auto or scroll.

This appears to be a pre-existing bug in the flexbox code (in that it is affected by the overlay scrollbars) that was uncovered by extending the start time of the scrollbar fade timeout. Perhaps dholbert can provide more insight since he knows the flexbox code better.

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I can reproduce. Looks like maybe we're setting aside space for the scrollbar, for some reason (?).

kats, is there a way to enable overlay scrollbars on Desktop, so I can poke around at this this more easily?

(I tried setting the 4 ui.*scroll* prefs shown in attachment 8496655 [details] [diff] [review] in my desktop build, and it didn't seem to change my scrollbars. Maybe I made a mistake, but I suspect there might be more I need to do.)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I'm not sure how to get overlay scrollbars on Linux, but I'm not sure that would help either - I have overlay scrollbars on Mac and I can't reproduce this bug in my desktop browser (recent Aurora on OS X 10.9.5).

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I can reproduce in a desktop b2g build (w/ latest b2g nightly, using a fresh profile), loading the attached testcase in the b2g browser app.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I've got this in a debug build. When the jump happens, I get a bunch of instances of this warning:
{
WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6339
}

...and this assertion:
{
###!!! ASSERTION: Child frames aren't sorted correctly: '(!mFrames.IsEmpty() && mFrames.FirstChild()->GetContent()->GetContainingShadow()) || nsIFrame::IsFrameListSorted<IsOrderLEQWithDOMFallback>(mFrames)', file layout/generic/nsFlexContainerFrame.cpp, line 1849
}

I think the warning probably isn't too relevant here (might indicate something that could be improved, but probably not involved with the main symptom of this bug).  I think the assertion is likely also not relevant -- I initially thought we might be mis-sorting the scrollbars to the start of the child list during reflow, but it looks like they're actually being inserted at the beginning.

The frame tree ends up looking like this:
{
HTMLScroll(div)(3)@7f25e0cae8c0 next=7f2609d592e8 {0,3090,18240,8232} [state=0000024000000010] [content=7f2609d309c0] [sc=7f2618d61f20]<
  Box(scrollcorner)(-1)@7f2609d3ab20 next=7f25e0cae848 {18120,8112,0,0} [state=0000100080c00200] [content=7f25e0cb2d30] [sc=7f26061d8be0]<>
  FlexContainer(div)(3)@7f25e0cae848 {120,120,18000,7992} [state=0000100000000230] [content=7f2609d309c0] [sc=7f2613e8aa70:-moz-scrolled-content]<
    ScrollbarFrame(scrollbar)(-1)@7f25e0caec38 next=7f2609d39838 {-480,0,480,7992} [state=0000100080880020] [content=7f25e0cb2ca0] [sc=7f2613e8aec0,parent=7f2618d61f20]<
      SliderFrame(slider)(-1)@7f25e0caee50 next=7f2609d394c0 {0,0,480,7512} [state=0000160080800000] [content=7f2605f5d3e0] [sc=7f2613c21210]<
        ButtonBoxFrame(thumb)(0)@7f2609d39248 {0,0,480,7500} [state=2000160080000000] [content=7f2605f5d7d0] [sc=7f2609d39f40]<>
      >
      ButtonBoxFrame(scrollbarbutton)(-1)@7f2609d394c0 {0,7512,480,480} [state=2000160080c00000] [content=7f2605f5dfb0] [sc=7f2609d596a8]<>
    >
    ScrollbarFrame(scrollbar)(-1)@7f2609d39838 next=7f2609d59190 {0,-480,480,480} [state=0000108080c80020] [content=7f25e0cb2c10] [sc=7f2613c20718,parent=7f2618d61f20]<
      SliderFrame(slider)(-1)@7f2609d3a3a0 next=7f2609d3a9b0 {0,0,0,480} [state=0000160080c00000] [content=7f25e0cb2f70] [sc=7f25df0dc0c0]<
        ButtonBoxFrame(thumb)(0)@7f2609d3a738 {0,0,0,480} [state=2000160080400000] [content=7f2605e744c0] [sc=7f2609d3a468]<>
      >
      ButtonBoxFrame(scrollbarbutton)(-1)@7f2609d3a9b0 {0,0,480,480} [state=2000160080c00000] [content=7f2605e74670] [sc=7f2613e8a238]<>
    >
    Block(p)(1)@7f2609d59190 {480,2496,5760,3000} [state=0000120000100220] [content=7f2609d30a60] [sc=7f2613c212b8,parent=7f2618d61f20]<
      line 7f2609d59298: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,5760,3000} <
        Text(0)"Title"@7f2609d59228 {0,30,5760,2940} [state=80000040b0600000] [content=7f260aa98080] [sc=7f2609d3ae80:-moz-non-element] [run=7f2605fee1a0][0,5,T] 
}

Notably:
 - We have two scrollbar frames, which are children of the FlexContainer frame. (So, they're flex items.)
 - They're at the beginning of the FlexContainer's child list. (I verified that this is where they're inserted, in nsFrameManager::InsertFrames).
 - Each one is 480 app units wide.
 - The first one is at position -480, so its right edge is at position 0.
 - The second one is at position 0, so its right edge is at position 480; so, it pushes over the actual child with contents "Title".

(It surprises me a bit that these scrollbars are getting inserted as children of the flex container frame. I seem to recall that they should really be children of the HTMLScroll frame, or something...)

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Yeah, when I load this in a normal Linux Firefox build (with normal Linux scrollbars), my frametree looks more like what I expect:
 HTMLScroll(div)
   ScrollbarFrame(scrollbar)
   ScrollbarFrame(scrollbar)
   Box(scrollcorner)
   FlexContainer(div)
     Block(p)
       line
         Text(0)"Title"

Note that the ScrollbarFrames are children of the HTMLScroll, and are *outside* of the FlexContainer (unlike in comment 7).

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

It looks like on b2g, we create the frames, then we wait for the delay, and then we rapidly re-create the scrollframes over and over for each frame of the fade-out animation.  (That seems inefficient, if that's really what's happening.)

We get the parentage right on the first frame creation, but not on the recreations during the fade-out.

In particular, GetContentInsertionFrameFor is returning the flex container frame, here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7252

Comment 10

[Daniel Holbert [:dholbert] (PTO until 2/24)]

So a large part of this seems to be due to the re-creation of these scrollbar frames.

It looks like we're tweaking their style (via the inline style attribute) in SetOpacityOnElement(), here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/ScrollbarActivity.cpp?rev=4ef03db1385b#347
(called via WillRefresh()'s call to UpdateOpacity(), which looks up the element for each scrollbar frame, and calls SetOpacityOnElement on it).

When we get to figuring out what needs to change from that style update, though, we end up thinking we need to reconstruct the frame, because nsStyleDisplay::CalcDifference() sees mDisplay going from 18 (-moz-box) to 1 (block), for some reason.

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

We're converting mDisplay from "-moz-box" to "block" because: when we recreate the scrollbar's nsStyleContext for the style change, we end up blockifying its "display" value, in ApplyStyleFixups, because its parent style-context has "display:flex".

So, we need to pass down a bool to suppress that blockification.

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: wip fix

I found two different places where we were calling ApplyStyleFixups on a new nsStyleContext for a scrollbarframe:

 (1) RestyleManager::ComputeStyleChangeFor (where we can just add a RAII object to set the bool on the TreeMatchContext -- nice and clean).

 (2) RestyleManager::TryStartingTransition, which calls nsStyleSet::ResolveStyleByAddingRules.  Here, it's a bit more complicated, because there's no TreeMatchContext that we can just tweak. In this patch, I added a boolean arg to ResolveStyleByAddingRules, but that might not be the cleanest thing. (We also would need to make sure we pass the right value of the arg from the other ResolveStyleByAddingRules callers. But in one caller*, in nsAnimationManager, I'm not clear we can know what the right value of the arg is, since we don't have access to the element & can't tell if it's the root of a native anonymous content tree :-/ )

To fix the symtoms of this bug, it seems we really only need to fix code-path (2), but we might as well fix (1) as well, since we don't want to be blockifying the display value of a scrollbar. (even if it doesn't end up affecting rendering for some reason)

*(The one caller of ResolveStyleByAddingRules that I'm a bit worried about is here, FWIW:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsAnimationManager.cpp?rev=9ad680392072#407 )

Comment 13

[Boris Zbarsky [:bzbarsky]]

Modifying style on scrollbars like that is just insane.  :(

Comment 14

[Julien Wajsberg [:julienw]]

NI Bhavana for the blocker request.

Comment 15

[bhavana bajaj [:bajaj]]

Spoke offline with dholbert and we were able to reproduce one instance of this issue int he SMS app while adding contacts. We were not sure how many other places this combination is used and given this is a regression, blocking on this. :dholbert mentioned it should be a easy fix.

Comment 16

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Thanks! Yup, I'll have a patch up for review from bz by mid-day tomorrow. (Finishing up something else at the moment, and then this is next on my list to finish up.)

Comment 17

[Daniel Holbert [:dholbert] (PTO until 2/24)]

OK, new data point: it turns out we're *doubly* transitioning the scrollbar-opacity here, which is pretty broken.

 (A) We do the UpdateOpacity() / SetOpacityOnElement() manual style-tweak, from C++
 (B) We *also* have a XUL scrollbar style-rule that sets "transition: opacity 1s ease;" in b2g/chrome/content/content.css.

This is redundant, but worse,  it means that *each C++ style-tweak* (from part (A)) is detected as a style change, which ends up triggering a 1-second long transition (from (B)). This is why we end up actually exercising the "RestyleManager::TryStartingTransition" code from comment 12, which is involved in causing this.  This is also why this isn't reproducible on Firefox Desktop on Mac (per comment 5) -- Mac has fading scrollbars, but it lacks the redundant "transition" declaration.

So, I think the simplest (and most backportable) fix here is probably to drop that transition style rule.  Or, if it actually does what it's supposed to do, we could instead drop the C++ UpdateOpacity() stuff on B2G -- but since that's hooked in deeper, it scares me a bit more to rip it out.

Comment 18

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8503455 [details] [diff] [review]
wip fix

(We still should probably take something like my WIP patch, as well, but it won't be necessary to fix this. I'm spinning that off into a separate bug -- filed bug 1083435 to cover that.)

Comment 19

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> So, I think the simplest (and most backportable) fix here is probably to
> drop that transition style rule.  Or, if it actually does what it's supposed
> to do, we could instead drop the C++ UpdateOpacity() stuff

I tried both of these options, to see if either of them break scrollbar behavior. Firstly, both options fix the attached testcase, so that's good.  It looks like the latter option (removing the UpdateOpacity() calls from ::WillRefresh) breaks scrollbar fading -- I don't see any fading at all after making that change & viewing a tall page. The scrollbars just blink out of existence after a short delay -- no fading. (So, the "transition" style doesn't seem to actually be transitioning them, in a user-visible way.)

The former option (dropping the "transition" declaration from content.css) doesn't seem to break anything, though, so we should probably go with that, both for simplicity & consistency-with-other-platforms reasons. (If we want to try to do scrollbar-fading using a CSS transition, we should probably do that on all platforms w/ fading scrollbars -- b2g, mac, android -- and rip out the UpdateOpacity stuff when we do.)

Comment 20

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Looks like the CSS opacity transition dates back to bug 778810 (where we initially made scrollbars hide in the first place, on b2g), and it's unclear to me whether it worked in the first place. (At least, there's no discussion of it there, except for a suggestion that a 1s fade-out might be too long, in bug 778810 comment 4).

vingtetun, do you recall if that transition styling actually worked for fading scrollbars out at that point?

Comment 21

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v1: backout f687cf5c1bb5 (no longer functional, interacts poorly with ScrollbarActivity.cpp)

This effectively backs out mozilla-central changeset f687cf5c1bb5 (from bug 778810).

As noted in previous comment, we have c++ code that does this fading on other platforms (mac, android), and this CSS currently only seems to mess with that by starting micro-transitions -- it doesn't actually do any fading on its own, if I locally rip out the C++ fading code.  So, this makes us consistent with Android & Mac.

(As further support: this is the *only* place where we have a style rule for xul|scrollbar[disabled] and xul|scrollbar:not([active=, as shown here:
http://mxr.mozilla.org/mozilla-central/search?string=xul|scrollbar%5Bdisabled
http://mxr.mozilla.org/mozilla-central/search?string=xul|scrollbar%3Anot%28%5Bactive
)

I've tested this change in a desktop b2g build, and it doesn't seem to impact scrollbar behavior. I tried scrolling in the homescreen, browser, & gallery app, and the scrollbar still shows up when I'm scrolling, and then fades out after a short delay.

Comment 22

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> vingtetun, do you recall if that transition styling actually worked for
> fading scrollbars out at that point?

(poking around a bit at bug 778810's dependencies -- I found bug 636564 comment 136, which says "the opacity fade out CSS transition from bug 778810 works in B2G but not for me [on Mac]". So at that point in time, it sounds like this CSS transition did indeed work.)

Comment 23

[Daniel Holbert [:dholbert] (PTO until 2/24)]

...and bug 636564 comment 215 suggests that its patches [which added fading-overlay-scrollbars on Mac] intentionally changed the ways that scrollbars work on B2G as well -- in particular, tests failed (& were disabled) because "we're now hiding the scrollbars unless the user is actively scrolling".

So, I'll bet this CSS has been ineffective [aside from triggering micro-transitions] ever since bug 636564 landed.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Btw in Fennec we draw scrollbars separately in the java code and don't use the ones that Mac uses.

Comment 25

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8505754 [details] [diff] [review]
fix v1: backout f687cf5c1bb5 (no longer functional, interacts poorly with ScrollbarActivity.cpp)

NOTE: There's one tweak that I'm making to the patch before landing:

>-xul|scrollbar:not([active="true"]),
>-xul|scrollbar[disabled] {
>-  opacity: 0;
>-  transition: opacity 1s ease;
>-}

After testing this patch on an actual device (unagi), it looks like the "opacity:0" here is actually needed (though I didn't need it in a desktop b2g build, for some reason). Without that declaration, I see the scrollbars fade out and then snap back into existence, whenever I scroll the homescreen or browser on my unagi.

So, I'm tweaking the patch to preserve this CSS rule, just with "opacity: 0". (not with the transition, since that's the part that's redundant & causes problems here.)

I was able to verify that the main bug here (with the Messages app's "To" field) becomes fixed, with the attached patch & also with this tweak, so that's good news.

Comment 26

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v2: Mostly backout f687cf5c1bb5 (transition no longer functional, interacts poorly with ScrollbarActivity.cpp)

Here's the updated patch. Carrying forward r+.

(Note that I'm still removing the "opacity:1" on the non-[disabled] rule, because it's unnecessary; it's the initial value of the property.)

Comment 27

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed: https://hg.mozilla.org/integration/b2g-inbound/rev/a76d62002a2f

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a76d62002a2f

Comment 29

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(I'll request backport approval for 2.1 in a day or two, once this has gotten some testing on trunk builds.  Oleg: if you're running trunk, could you verify that this is fixed for you?)

Comment 30

[Daniel Holbert [:dholbert] (PTO until 2/24)]

[needinfo=me to remember to request backport approval]

Comment 31

[Oleg Zasypkin [:azasypkin]]

I'm on PTO currently and don't have access to any FxOS device to verify. ni? Steve to help with this.

Steve, I've noticed this "jumping" issue in multiline recipients input and report panel refreshed by patch for bug 1021608. Could you please check if the issue has gone on the latest build? I hope the fix will reach pvt by Monday, otherwise we'll need to build master b2g locally...

Thanks!

Comment 32

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #31)
> I'm on PTO currently and don't have access to any FxOS device to verify. ni?
> Steve to help with this.
> 
> Steve, I've noticed this "jumping" issue in multiline recipients input and
> report panel refreshed by patch for bug 1021608. Could you please check if
> the issue has gone on the latest build? I hope the fix will reach pvt by
> Monday, otherwise we'll need to build master b2g locally...
> 
> Thanks!

Hi Oleg/Daniel,

I've verified this with latest (10/19) build and this jumping issue seems fixed, thanks for the great help!

Comment 33

[Daniel Holbert [:dholbert] (PTO until 2/24)]

[Marking verified per comment 32]

Comment 34

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8506487 [details] [diff] [review]
fix v2: Mostly backout f687cf5c1bb5 (transition no longer functional, interacts poorly with ScrollbarActivity.cpp)

Approval Request Comment
[Feature/regressing bug #]: bug 1074030 (which was backported to aurora for b2g-v2.1)

[User impact if declined]: Jumping content in Messages "choose recipients" field, and in other UI with a flex container that has set an 'overflow/overflow-y/overflow-x' property.

[Describe test coverage new/current, TBPL]: Flex containers have good code-coverage reftests/mochitests. Fading scrollbars presumably do not have good test coverage, unfortunately (at least, based on the fact that bug 1074030 didn't need to change any tests), because they're inherently hard to test.  I have a test for the original bug here, but it doesn't seem to actually reproduce the issue in our test harnesses, possibly because scrollbars behave differently there vs. in an actual live B2G + gaia environment.  I'll post the test soon, but I'm not sure it's useful to actually land it.

[Risks and why]: It's conceivable that this might break scrollbar fading in some scenarios, but that seems unlikely. "Why": before this patch, we attempt to fade scrollbars in two different ways, simultaneously, and they interact poorly. This patch removes the one older way that doesn't actually seem to work on its own.  From local testing (& lack of bugs filed since this landed), scrollbars continue to fade correctly, so I think this is low-risk.

[String/UUID change made/needed]: None.

Comment 35

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: reftest, as patch

Here's a reftest with a simplified version of the original testcase.  The testcase reproduces the bug for me in my local b2g-desktop build. I haven't been able to locally run reftests with that b2g build, so I'm not sure if this actually triggers the bug in the reftest harness.

(An earlier version of this reftest passed TBPL, even without the fix, which I thought meant that it was useless as a reftest; but I think that might be because the reference case was *also* affected by the bug too at that point. I'll update this bug after I've gotten this updated reftest-version tested via TBPL.)

Comment 36

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Yeah, this reftest unfortunately doesn't reproduce the problem in the reftest harness. :-/

Try run, with the fix reverted & the reftest applied:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9be6e65a17c4

I verified that the test is being run as part of "R7" in that run, but it passes. Not sure why. If I directly load it in my b2g browser app in a desktop b2g build, it definitely fails (and asserts per comment 7, which should also make it fail in debug builds at least).

I'm going to land the reftest anyway, for additional coverage; even if it passes in affected builds right now, it'll be useful as a regression-test if & when we add a testsuite that behaves more like the b2g browser app & actually exercises the bug here.  (Maybe we even already have such a test configuration, and it's just hidden or disabled.)

Comment 37

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed the reftest discussed in comment 36:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa3ff2c518e

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ffa3ff2c518e

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed6e7b46e77a
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bd4fce1c9ad

This needs b2g34 approval if it's wanted for B2G v2.1.

Comment 40

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8506487 [details] [diff] [review]
fix v2: Mostly backout f687cf5c1bb5 (transition no longer functional, interacts poorly with ScrollbarActivity.cpp)

Thanks, Ryan. This fix is indeed needed for 2.1, per comment 1, so requesting approval for b2g34.

[Approval Request Comment]: See comment 34 for answers to questions.

Comment 41

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #36)
> I verified that the test is being run as part of "R7" in that run, but it
> passes. Not sure why. If I directly load it in my b2g browser app in a
> desktop b2g build, it definitely fails (and asserts per comment 7, which
> should also make it fail in debug builds at least).

Mystery solved -- dbaron says that our test harness sets a pref to disable scrollbar-fading.  That would explain why the test isn't failing (since the bug here is *triggered* by scrollbar-fading).

Looks like the pref is "layout.testing.overlay-scrollbars.always-visible". I'll see if tweaking that in the reftest manifest exposes the bug, in a build without the fix:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9883dd01090a

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8ed5f04d596c
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8bed3a4da63f

Comment 43

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: reftest tweak

Tweaking that pref does indeed turn the test into something more useful -- that makes it still pass w/ the fix, & fail a significant fraction of the time without the fix.

Specifically: here's a Try run with the tweaked test, & with this bug's fix:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4cfff06e8bac
  (Test still passes 100% of the time, with ~30 test-runs)

...and here's a similar Try run, with this bug's fix *backed out*:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9573622a9edf
  (> 50% failure rate, with ~20 test-runs. Failure is desirable here.)

I'll land this tweak on m-i. We may want to backport it (with a=test-only), too.

Comment 44

[Daniel Holbert [:dholbert] (PTO until 2/24)]

One potential test problem -- there's a slight chance that this pref-tweak will make this reftest intermittently fail, *if* it makes the toplevel-viewport's scrollbar fade by a random amount (depending on the reftest snapshot timing). Given the green Try run from comment 43 (and the fact that we already have a snapshot delay in the testcase vs. no delay in the reference case), I think we probably don't have to worry about this hypothetical problem.

(I tried (in some other Try runs) to suppress the toplevel scrollbar completely with "overflow:hidden" on the <body>, but that didn't seem to have any effect; the scrollbar was still visible. So, I'm inclined not to worry about this unless/until we actually start seeing intermittent-orange problems.)

Comment 45

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed reftest tweak (to make the reftest actually test the bug, by enabling scrollbar fading):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6381e5f905

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b6381e5f905