1091709 - A scrollbar appear at the bottom of the menu panel when entering or exiting customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

Attachment: Scrollbar during animation at the bottom of the panel

At least on Windows a scroll bar briefly flickers at the bottom of the menu panel when entering or exiting customization mode. I don't have a regression range but I would say about a few weeks back.

Comment 1

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #0)
> Created attachment 8514363 [details]
> Scrollbar during animation at the bottom of the panel
> 
> At least on Windows a scroll bar briefly flickers at the bottom of the menu
> panel when entering or exiting customization mode. I don't have a regression
> range but I would say about a few weeks back.

A few like multiple? A regression window here would be super useful. I'd be worried about this being caused by bug 1074520 if it's more recent than 2 weeks ago...

Comment 2

[:Gijs (he/him)]

(and I mean, is this on nightly? anywhere else you see this?)

Comment 3

[u428464]

(In reply to :Gijs Kruitbosch from comment #1)
> (In reply to Guillaume C. [:ge3k0s] from comment #0)
> > Created attachment 8514363 [details]
> > Scrollbar during animation at the bottom of the panel
> > 
> > At least on Windows a scroll bar briefly flickers at the bottom of the menu
> > panel when entering or exiting customization mode. I don't have a regression
> > range but I would say about a few weeks back.
> 
> A few like multiple? A regression window here would be super useful. I'd be
> worried about this being caused by bug 1074520 if it's more recent than 2
> weeks ago...

No I really think it's older than that. I may have the time to find a regression range in a few days. I see this on Nightly. Will try Aurora now.

Comment 4

[u428464]

Reproducible on Nightly, Aurora, Beta but not on release. So you may be right about the concerned bug.

Comment 5

[u428464]

(In reply to Guillaume C. [:ge3k0s] from comment #4)
> Reproducible on Nightly, Aurora, Beta but not on release. So you may be
> right about the concerned bug.

No the issue is at least one month old.

Comment 6

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #5)
> (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > Reproducible on Nightly, Aurora, Beta but not on release. So you may be
> > right about the concerned bug.
> 
> No the issue is at least one month old.

but it was uplifted, then, if beta is affected? :-(

Comment 7

[u428464]

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Guillaume C. [:ge3k0s] from comment #5)
> > (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > > Reproducible on Nightly, Aurora, Beta but not on release. So you may be
> > > right about the concerned bug.
> > 
> > No the issue is at least one month old.
> 
> but it was uplifted, then, if beta is affected? :-(

Sorry I wasn't clear it's at least a month old on Nightly.

Comment 8

[:Gijs (he/him)]

This regressed end of August.

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eed9fe35a00d&tochange=1db35d2c9a2f

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=eed9fe35a00d&tochange=1db35d2c9a2f

I... have no idea what caused this, actually. Mike/Jared?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Wow, nothing at all stands out in that range. :/ I think we need to bisect.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

My guesses:

9c0d8ef32b3e	Markus Stange — Bug 1021564 - Invalidate filtered frames when they move in certain ways. r=roc
4911e706d559	Botond Ballo — Bug 1058884 - Update drawing of borders to account for multi-layer-apz. r=kats,BenWa
bdcf3a2da55c	Neil Rashbrook — Bug 1054289 Scroll to the current ref, not the original one r=smaug

But out of those I think it may be the first one. Markus, could this be fallout from your change?

Comment 13

[:Gijs (he/him)]

[Tracking Requested - why for this release]: perf/visual regression.

(Markus, if you have time to look at this, that'd still be much appreciated)

Comment 14

[:Gijs (he/him)]

local testing says the regression range is:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5fd6428ae179&tochange=983cf2175495

trying to bisect further at the minute... but this excludes Markus' change.

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> local testing says the regression range is:
> 
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=5fd6428ae179&tochange=983cf2175495
> 
> trying to bisect further at the minute... but this excludes Markus' change.

I'm no longer sure about this. :-\

Comment 16

[:Gijs (he/him)]

So, believe it or not... it seems this was regressed by bug 1060165, specifically, by https://hg.mozilla.org/mozilla-central/rev/2123851421e6 . Verified by building:

https://hg.mozilla.org/integration/fx-team/rev/c98e45bed5e0 + ffxbld patches: works

https://hg.mozilla.org/integration/fx-team/rev/c98e45bed5e0 + ffxbld patches + 2123851421e6: broken

Nical/David: any ideas? My only hunch here (as someone who totally doesn't know graphics/layout) is that this:

https://hg.mozilla.org/mozilla-central/rev/2123851421e6#l15.2

has unintended consequences.

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> Nical/David: any ideas? My only hunch here (as someone who totally doesn't
> know graphics/layout) is that this:
> 
> https://hg.mozilla.org/mozilla-central/rev/2123851421e6#l15.2
> 
> has unintended consequences.

This hunch is wrong, please disregard. :-)

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm tracking this visual regression but unless we have a safe fix by Wed, Nov 12, we're going to ship with this bug as it doesn't look critical enough to hold the release.

Comment 19

[Nicolas Silva [:nical]]

This is in the realm of layout which I don't know as well as I would like to. I don't know what could cause this.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm marking 34 as wontfix. This is still a visual regression that we should fix.

Jet - FYI, Nical says that this is layout related. Can you please help find an owner to investigate further?

Comment 21

[Jet Villegas (inactive)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > Nical/David: any ideas? My only hunch here (as someone who totally doesn't
> > know graphics/layout) is that this:
> > 
> > https://hg.mozilla.org/mozilla-central/rev/2123851421e6#l15.2
> > 
> > has unintended consequences.
> 
> This hunch is wrong, please disregard. :-)

Is the regression window still correct?

Comment 22

[:Gijs (he/him)]

(In reply to Jet Villegas (:jet) from comment #21)
> (In reply to :Gijs Kruitbosch from comment #17)
> > (In reply to :Gijs Kruitbosch from comment #16)
> > > Nical/David: any ideas? My only hunch here (as someone who totally doesn't
> > > know graphics/layout) is that this:
> > > 
> > > https://hg.mozilla.org/mozilla-central/rev/2123851421e6#l15.2
> > > 
> > > has unintended consequences.
> > 
> > This hunch is wrong, please disregard. :-)
> 
> Is the regression window still correct?

Yes! I'm very sure about the regression window, just clueless as to how that caused this issue...

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

We're now too late for speculative fixes on 35, tracking for 36/37 still since we have identified a regression window - we need an assignee here and for either a backout option or a forward fix to deal with this visual regression.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Jet, could you find someone to help us on this? Thanks

Comment 25

[Mats Palmgren (inactive)]

gfxPoint3D was using gfxFloat (double) internally.  Point3D is using Float (float),
so we lost precision in some operations when changing gfxPoint3D => Point3D.
I'm a bit surprised the conversion didn't cause more bugs, but I guess gfxPoint3D
wasn't used much.  Anyway, please test these builds when they are ready:
https://tbpl.mozilla.org/?tree=Try&rev=0150ce95a7af

Comment 26

[:Gijs (he/him)]

(In reply to Mats Palmgren (:mats) from comment #25)
> gfxPoint3D was using gfxFloat (double) internally.  Point3D is using Float
> (float),
> so we lost precision in some operations when changing gfxPoint3D => Point3D.
> I'm a bit surprised the conversion didn't cause more bugs, but I guess
> gfxPoint3D
> wasn't used much.  Anyway, please test these builds when they are ready:
> https://tbpl.mozilla.org/?tree=Try&rev=0150ce95a7af

Yay! That does indeed fix it for me.

Comment 27

[Mats Palmgren (inactive)]

Attachment: fix

This just inlines Transform3D and use local gfxFloats (double) instead
of passing point.x/y via a Point3D (float) to avoid losing precision
(also skipping the z component which we don't need).  I intend to 
uplift this to Aurora/Beta too.

Comment 28

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/538af3cc79b2

Comment 29

[Mats Palmgren (inactive)]

Comment on attachment 8551342 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: bug 1060165
[User impact if declined]: (rare) layout errors (triggering a scrollbar when not needed etc) when using transforms.
[Describe test coverage new/current, TreeHerder]: green on Try
[Risks and why]: low risk; the patch basically reverts gfx3DMatrix::Transform(const gfxPoint&) to its pre-1060165 behaviour.
[String/UUID change made/needed]: none

Comment 30

[Mats Palmgren (inactive)]

FWIW, it seems like it improves performance as well:

Improvement: Mozilla-Inbound-Non-PGO - Customization Animation Tests - WINNT 6.2 x64 - 3.68% decrease
-----------------------------------------------------------------------------------------------------
    Previous: avg 29.022 stddev 0.337 of 12 runs up to revision 973ede87dcdd
    New     : avg 27.954 stddev 0.334 of 12 runs since revision 538af3cc79b2
    Change  : -1.067 (3.68% / z=3.167)
    Graph   : http://mzl.la/1Cffu6O

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/538af3cc79b2

Comment 32

[Jet Villegas (inactive)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Jet, could you find someone to help us on this? Thanks

Mats! \o/

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/409521ef3f98
https://hg.mozilla.org/releases/mozilla-beta/rev/0b22d12d0736

Comment 34

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0

Confirming the fix on latest Nightly, build ID: 20150121030203, the scrollbar is no longer displayed when entering/exiting Customize Mode.
Marking verified as the target Milestone is reached.

Will verify tomorrow on latest Aurora and Beta.

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0

Verified fixed on latest Aurora (build ID: 20150125004007) and on Firefox 36 beta 3 (build ID: 20150122214638).