Closed Bug 1091709 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan
Tracking Status
firefox33 --- unaffected
firefox34 + wontfix
firefox35 + wontfix
firefox36 + verified
firefox37 + verified
firefox38 --- verified

People

(Reporter: ge3k0s, Assigned: mats)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
(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...
(and I mean, is this on nightly? anywhere else you see this?)
(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.
Reproducible on Nightly, Aurora, Beta but not on release. So you may be right about the concerned bug.
(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.
(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? :-(
(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.
This regressed end of August.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
(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?
Flags: needinfo?(mconley)
Flags: needinfo?(jaws)
Wow, nothing at all stands out in that range. :/ I think we need to bisect.
Flags: needinfo?(mconley)
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?
Flags: needinfo?(jaws) → needinfo?(mstange)
[Tracking Requested - why for this release]: perf/visual regression.

(Markus, if you have time to look at this, that'd still be much appreciated)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(mstange)
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-\
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.
Blocks: 1060165
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(dzbarsky)
(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. :-)
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.
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.
Flags: needinfo?(nical.bugzilla)
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?
Flags: needinfo?(bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(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...
Flags: needinfo?(gijskruitbosch+bugs)
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.
Jet, could you find someone to help us on this? Thanks
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
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch fixSplinter Review
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.
Assignee: nobody → mats
Attachment #8551342 - Flags: review?(matt.woodrow)
Attachment #8551342 - Flags: review?(matt.woodrow) → review+
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
Attachment #8551342 - Flags: approval-mozilla-beta?
Attachment #8551342 - Flags: approval-mozilla-aurora?
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
https://hg.mozilla.org/mozilla-central/rev/538af3cc79b2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.1 - 26 Jan
Flags: needinfo?(dzbarsky)
Attachment #8551342 - Flags: approval-mozilla-beta?
Attachment #8551342 - Flags: approval-mozilla-beta+
Attachment #8551342 - Flags: approval-mozilla-aurora?
Attachment #8551342 - Flags: approval-mozilla-aurora+
QA Contact: cornel.ionce
Flags: needinfo?(bugs)
(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Jet, could you find someone to help us on this? Thanks

Mats! \o/
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.
Status: RESOLVED → VERIFIED
Keywords: qawanted
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).
You need to log in before you can comment on or make changes to this bug.