Widget subview panels show unnecessary scrollbar when opened in the toolbar

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ge3k0s, Assigned: enndeakin)

Tracking

(Blocks 1 bug)

Trunk
Firefox 32
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox29 unaffected, firefox30+ verified, firefox31+ verified, firefox32 verified)

Details

(Whiteboard: [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa!])

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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).
Reporter

Updated

5 years ago
Whiteboard: [australis:P?]
Reporter

Updated

5 years ago
Whiteboard: [australis:P?] → [Australis:P?]
Reporter

Updated

5 years ago
Summary: History widget panel has unnecessary scrollbar → History widget panel shows unnecessary scrollbar
Reporter

Comment 1

5 years ago
Very probably caused by bug 610545
Blocks: 610545

Comment 2

5 years ago
Is this only on trunk? Screenshot? Really need more details here. :-(
Flags: needinfo?(ge3k0s)
Reporter

Comment 3

5 years ago
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.
Flags: needinfo?(ge3k0s)

Comment 4

5 years ago
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

5 years ago
(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!

Updated

5 years ago
Summary: History widget panel shows unnecessary scrollbar → Widget subview panels show unnecessary scrollbar when opened in the toolbar

Comment 6

5 years ago
(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

5 years ago
Also, the sidebar widget is unaffected.

Comment 8

5 years ago
But the Open With addon is affected by this.

Comment 9

5 years ago
(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

5 years ago
Posted image 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

5 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: firefox-backlog+
Whiteboard: [Australis:P?] → [Australis:P?] p=0

Updated

5 years ago
Duplicate of this bug: 996971

Comment 13

5 years ago
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.
Flags: needinfo?(enndeakin)

Comment 14

5 years ago
(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.
Whiteboard: [Australis:P?] p=0 → [Australis:P4] p=0

Updated

5 years ago
Duplicate of this bug: 998553
Assignee

Comment 16

5 years ago
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?
Flags: needinfo?(enndeakin) → needinfo?(gijskruitbosch+bugs)

Comment 17

5 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
Duplicate of this bug: 1006503
Assignee

Comment 19

5 years ago
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Per just duped bug 1006503, this affects Fx30.
Assignee

Updated

5 years ago
Attachment #8418188 - Flags: review?(mconley)
Flags: needinfo?(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 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?
Attachment #8418188 - Flags: review?(mconley) → review-
Assignee

Comment 23

5 years ago
Attachment #8418188 - Attachment is obsolete: true
Attachment #8418895 - Flags: review?(mconley)
Comment on attachment 8418895 [details] [diff] [review]
noscrolltoolbar

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

This looks good to me. Thanks Enn!
Attachment #8418895 - Flags: review?(mconley) → review+

Updated

5 years ago
Blocks: 1009116
https://hg.mozilla.org/mozilla-central/rev/003470a559cb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Whiteboard: [Australis:P4] p=0 → [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa?]

Updated

5 years ago
Duplicate of this bug: 1009461
Whiteboard: [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa?] → [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa+]
Uplift nominations for Beta/Aurora please?
Flags: needinfo?(enndeakin)
Assignee

Comment 29

5 years ago
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
Attachment #8418895 - Flags: approval-mozilla-aurora?
Flags: needinfo?(enndeakin)
Assignee

Comment 30

5 years ago
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
Attachment #8418895 - Flags: approval-mozilla-beta?
Attachment #8418895 - Flags: approval-mozilla-beta?
Attachment #8418895 - Flags: approval-mozilla-beta+
Attachment #8418895 - Flags: approval-mozilla-aurora?
Attachment #8418895 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(twalker)
Keywords: verifyme
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(twalker)
Keywords: verifyme
Verified as fixed on latest Aurora (build ID: 20140515004001) and on Firefox 30 beta 5 (build ID: 20140515140857).
Whiteboard: [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa+] → [Australis:P4] p=0 s=it-32c-31a-30b.2 [qa!]

Comment 36

5 years ago
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

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

Comment 38

5 years ago
(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. :-)
Flags: needinfo?(luckyrat)

Comment 39

5 years ago
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/
Flags: needinfo?(luckyrat)

Updated

5 years ago
See Also: → 1014229
You need to log in before you can comment on or make changes to this bug.