994194 - Widget subview panels show unnecessary scrollbar when opened in the toolbar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[u428464]

When the history widget is put into the toolbar, the history panel shows a scrollbar even if there is no need for it (ie even if there are only a few entries).

Comment 1

[u428464]

Very probably caused by bug 610545

Comment 2

[:Gijs (he/him)]

Is this only on trunk? Screenshot? Really need more details here. :-(

Comment 3

[u428464]

Attachment: History panel unnecessary scrollbar.png

The issue appeared in today's build. It's probably the known issue related to new arrowpanel animation (bug 610545) that prevented the menu panel to be animated too, see bug 989991.

Comment 4

[Tim Nguyen :ntim]

Confirmed on Windows 8.1

The new panel animation also added a scrollbar during the animation for the developer widget, but I'll file a new bug about that in case there isn't one yet.

Comment 5

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #4)
> Confirmed on Windows 8.1
> 
> The new panel animation also added a scrollbar during the animation for the
> developer widget, but I'll file a new bug about that in case there isn't one
> yet.

Please don't. The cause is very likely the same. Can we just keep these in one bug for now? Thanks!

Comment 6

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment
> #4)
> > Confirmed on Windows 8.1
> > 
> > The new panel animation also added a scrollbar during the animation for the
> > developer widget, but I'll file a new bug about that in case there isn't one
> > yet.
> 
> Please don't. The cause is very likely the same. Can we just keep these in
> one bug for now? Thanks!

Well, the developer widget bug only happens during the animation.
While the history widget bug happens at any time.
Not the same cause.

Comment 7

[Tim Nguyen :ntim]

Also, the sidebar widget is unaffected.

Comment 8

[Tim Nguyen :ntim]

But the Open With addon is affected by this.

Comment 9

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #8)
> But the Open With addon is affected by this.

All of this is useful information. The problem is, we've had several (like more than 3) bugs with scrollbars in these views in the past. I'm not sure if the "Open with" add-on uses our types of subviews, because I'm not familiar with the add-on, but it seems clear that there is a general problem where the animation work is causing scrollbars.

It's possible that all the individual consumers that do break need to be fixed on their own rather than fixing something general about panels on Windows 7/8 (does this reproduce on XP luna/classic? What about Win7 classic?), but first, we should investigate. We can file extra bugs later, but splitting the investigation into separate bugs means we split the information, which isn't a good idea as long as we don't know exactly what's causing the problem(s).

Comment 10

[Elbart]

Attachment: rss.png

RSS-panel (i.e. https://blog.mozilla.org/nnethercote/ ) and the Sidebars-panel (for a fraction of a second) are affected, too.

Comment 11

[Tim Nguyen :ntim]

Open With uses CustomizableUI widgets, with the subview model.

I think we should back the animation out until we figure out how to fix this.

Comment 13

[:Gijs (he/him)]

Neil, do you know what's causing this and where to look for a fix? We're doing maths to size the panel (same as for the main menu panel) and that's getting messed up by the animations. The current problem isn't really shippable, and AIUI you disabled the animation for the main panel menu for this reason.

We should really figure this out, because if we disable the animation for these panels too (which I guess we can do if this reaches the end of Aurora and we still don't have a fix), that's most of the high-visibility panels affected (with the downloads and bookmarks panel also planned to transition to the same standalone panel model that the 'real' subviews are using already), which halfway defeats the point of having a general panel animation, and also makes the UI look inconsistent.

Comment 14

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #13)
> Neil, do you know what's causing this and where to look for a fix? We're
> doing maths to size the panel (same as for the main menu panel) and that's
> getting messed up by the animations. The current problem isn't really
> shippable, and AIUI you disabled the animation for the main panel menu for
> this reason.
> 
> We should really figure this out, because if we disable the animation for
> these panels too (which I guess we can do if this reaches the end of Aurora
> and we still don't have a fix), that's most of the high-visibility panels
> affected (with the downloads and bookmarks panel also planned to transition
> to the same standalone panel model that the 'real' subviews are using
> already), which halfway defeats the point of having a general panel
> animation, and also makes the UI look inconsistent.

Maybe we calculate the dimensions before the transition ends.

Comment 16

[Neil Deakin]

When this history panel is on the toolbar, why does the history panel need to set the height at all?

If I disable the _setMaxHeight and _syncContainerWithMainView methods in panelUI.xml, everything seems to work fine. Is there some specific edge case I'm missing?

Comment 17

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #16)
> When this history panel is on the toolbar, why does the history panel need
> to set the height at all?
> 
> If I disable the _setMaxHeight and _syncContainerWithMainView methods in
> panelUI.xml, everything seems to work fine. Is there some specific edge case
> I'm missing?

It might not. The use of the panelmultiview in the toolbar's panel is a hack to begin with. Mike would know for sure.

Comment 19

[Neil Deakin]

Attachment: Don't set height of panels when on the toolbar

Comment 20

[Tracy Walker [:tracy]]

Per just duped bug 1006503, this affects Fx30.

Comment 21

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

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Neil Deakin from comment #16)
> > When this history panel is on the toolbar, why does the history panel need
> > to set the height at all?
> > 
> > If I disable the _setMaxHeight and _syncContainerWithMainView methods in
> > panelUI.xml, everything seems to work fine. Is there some specific edge case
> > I'm missing?
> 
> It might not. The use of the panelmultiview in the toolbar's panel is a hack
> to begin with. Mike would know for sure.

Enn and I talked IRL. I'm not convinced the _setMaxHeight and _sync functions are necessarily needed for widgets in the toolbar. I'll review this patch shortly.

Comment 22

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

Comment on attachment 8418188 [details] [diff] [review]
Don't set height of panels when on the toolbar

Review of attachment 8418188 [details] [diff] [review]:
-----------------------------------------------------------------

I took a look at panelUI.js, which is the content script that handles most of our panel logic, and it looks like we set a "nosubviews" attribute to "true" when we open widgets like this on the toolbar.

I think we should use that instead to decide whether or not to run those functions.

Can we use that instead?

Comment 23

[Neil Deakin]

Attachment: noscrolltoolbar

Comment 24

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

Comment on attachment 8418895 [details] [diff] [review]
noscrolltoolbar

Review of attachment 8418895 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks Enn!

Comment 25

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/003470a559cb

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/003470a559cb

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Uplift nominations for Beta/Aurora please?

Comment 29

[Neil Deakin]

Comment on attachment 8418895 [details] [diff] [review]
noscrolltoolbar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 610545
User impact if declined: unneeded scrollbars appear and disappear in bookmarks/history menus and other menus 
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 30

[Neil Deakin]

Comment on attachment 8418895 [details] [diff] [review]
noscrolltoolbar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 610545
User impact if declined: scrollbars appears on menus
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): I can't reproduce the problem described by bug 1006503 which got this bug marked for firefox30, so I'm not 100% sure it fixes it, but I don't think the patch itself has any risk
String or IDL/UUID changes made by this patch: none

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracy, would be good to have some idea of both verification on the issue being resolved but also making sure that even if we can't reproduce, the patch doesn't regress any existing QA in this area of the UI.

Comment 32

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

I've verified this issue on Windows 8.1 32bit, Windows 7 64bit, Ubuntu 13.04 and Mac OS X 10.9 using latest Firefox Nightly, build ID: 20140513030201.

Also checked that the scenario from bug 1006503 is fixed for Fx 32.

Marking this bug as verified and won't change the [qa+] tag until I can confirm this fix on Aurora and Beta.

Comment 33

[Neil Deakin]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1009e9e8a64e
https://hg.mozilla.org/releases/mozilla-beta/rev/9bd4a69ee2e0

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9bd4a69ee2e0

Comment 35

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

Verified as fixed on latest Aurora (build ID: 20140515004001) and on Firefox 30 beta 5 (build ID: 20140515140857).

Comment 36

[Chris Tomlinson]

My addon (KeeFox[1]) has been broken by the change introduced in this bug because I set my addon panel height to the same value as set in the property removed by this bug.

The reason I do this is that the default behaviour of the PanelUI in FF29 is to produce panels that do not have the correct size and/or scrollbar presence for the (XHTML) content my addon includes in the panel.

By adjusting my addon panel height after the panel has been opened (and via a setTimeout) I managed to work around the problem. The workaround is in this bit of source code: https://github.com/luckyrat/KeeFox/blob/v1.4.1/Firefox%20addon/KeeFox/chrome/content/panel.js#L1601-L1627

I notice that there are a variety of related bugs being targeted at the nightly channel so I hope that FF32 will resolve the underlying problems that cause me to require this workaround. I'm just not sure how else I can work around the problem in 30 and 31 if this bug is not reverted.

Should I be trying to work around this change in another way? I'm happy to implement an alternative workaround and/or help refine a test case for the underlying unreliability of the panel size but considering there are already several changes being worked on in this area, I would appreciate some guidance on the right way to approach this.

[1]: KeeFox - https://addons.mozilla.org/en-US/firefox/addon/keefox/

Comment 37

[TheRave]

@chris Would recommend to you to open a new bug regarding this issue. Here will nothing happen.

Comment 38

[:Gijs (he/him)]

(In reply to Chris Tomlinson from comment #36)
> My addon (KeeFox[1]) has been broken by the change introduced in this bug
> because I set my addon panel height to the same value as set in the property
> removed by this bug.
> 
> The reason I do this is that the default behaviour of the PanelUI in FF29 is
> to produce panels that do not have the correct size and/or scrollbar
> presence for the (XHTML) content my addon includes in the panel.
> 
> By adjusting my addon panel height after the panel has been opened (and via
> a setTimeout) I managed to work around the problem. The workaround is in
> this bit of source code:
> https://github.com/luckyrat/KeeFox/blob/v1.4.1/Firefox%20addon/KeeFox/chrome/
> content/panel.js#L1601-L1627
> 
> I notice that there are a variety of related bugs being targeted at the
> nightly channel so I hope that FF32 will resolve the underlying problems
> that cause me to require this workaround. I'm just not sure how else I can
> work around the problem in 30 and 31 if this bug is not reverted.
> 
> Should I be trying to work around this change in another way? I'm happy to
> implement an alternative workaround and/or help refine a test case for the
> underlying unreliability of the panel size but considering there are already
> several changes being worked on in this area, I would appreciate some
> guidance on the right way to approach this.
> 
> [1]: KeeFox - https://addons.mozilla.org/en-US/firefox/addon/keefox/

Hey Chris, thanks for making us aware. The best thing to do would be to file a new bug, ideally (but not necessarily) with a somewhat reduced testcase. I'm unsure why your add-on needs to set its panel size to begin with, and after looking at the code you linked for some time, I'm still not sure. :-)

I did notice you had to hack around another problem to do with when onViewShowing is fired. Would you mind filing a separate bug about that as well? We should really just have working APIs for all the add-ons out there. :-)

Comment 39

[Chris Tomlinson]

Thanks for the replies. I've created two new bugs (#1012925 and #1012920).

I'll also try to create a bug for the mentioned onViewShowing bug but might not get time to do that for a couple of days. In the mean time, it might be useful to know that I think it is related to the issue mentioned in the "Australis for add-on developers post part 2"[1] (specifically the bit near the end where we have to use a timer to work around the fact that onViewShowing is called at an inconvenient point in the view showing process).

[1]: https://blog.mozilla.org/addons/2014/03/06/australis-for-add-on-developers-2/