947488 - Bookmarks list too short and no scrollbar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

STR
Move Bookmarks Widget into PanelUI

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Attachment: bookmrksmax.jpg Bookmarks in PanelUI - browser maximized

See Attached image Bookmarks Star in the PanelUI. The scroll-bar on the bookmarks panel is partly hidden behind the page scrollbar. 

I don't see the list 'cut-off' as noted by Alice. 

If I un-maximize the browser the scroll-bar on the Bookmarks panel display's properly.

Comment 2

[Alice0775 White]

STR
1. Remove all widgets from PanelUI
2. Move Bookmarks Widget into PanelUI
3. Open Bookmarks

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Attachment: bookmarks un-maximized.jpg

See attached for Bookmarks PanelUI with browser un-maximized.  

This on win7 with Aero enabled. 

Latest m-c hourly build cset:
https://hg.mozilla.org/mozilla-central/rev/b3806ae5399d

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

Confirmed, STR in comment #2, I can repo the 'short-list' of Bookmarks.

Comment 5

[:Gijs (he/him)]

Jim, can you still reproduce this? There've been a couple of fixes to panel placement in the meantime, so I am hopeful this has gone away, but if not we need to investigate further.

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to :Gijs Kruitbosch from comment #5)
> Jim, can you still reproduce this? There've been a couple of fixes to panel
> placement in the meantime, so I am hopeful this has gone away, but if not we
> need to investigate further.

Yes, the jumpiness has gone away, but the list is still to long, and extends below the Windows Taskbar and I cannot even see the 'show all bookamrks' link due to being buried behind the Taskbar. 

The best solution IMO, would be to restore the flyout for Recent Closed Tabs and Recent closed windows thus consolidating the list as it used to do, and as it does now from the Menubar -> History.

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

Gijs, oops I misread and got this bug mixed up with the History list, please disregard comment #6 and proceed.

I will retest later this evening, my time EST when I get home from work.

Comment 8

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to :Gijs Kruitbosch from comment #5)
> Jim, can you still reproduce this? There've been a couple of fixes to panel
> placement in the meantime, so I am hopeful this has gone away, but if not we
> need to investigate further.

Yes, I can still repo the 'short-list' following the STR in comment #2.

Comment 9

[Charles Evans]

FYI: On linux, bookmark submenus cover the taskbar, even if not full height.
Is this a bug?
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20140120132616 CSet: c1ef58534e98

Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140121063910 CSet: 1b52aa569ced

Comment 10

[:Gijs (he/him)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #8)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Jim, can you still reproduce this? There've been a couple of fixes to panel
> > placement in the meantime, so I am hopeful this has gone away, but if not we
> > need to investigate further.
> 
> Yes, I can still repo the 'short-list' following the STR in comment #2.

So the weird thing is that we have both this bug filed and bug 963078. It seems weird that both happen. :-\

Comment 11

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #8)
> > (In reply to :Gijs Kruitbosch from comment #5)
> > > Jim, can you still reproduce this? There've been a couple of fixes to panel
> > > placement in the meantime, so I am hopeful this has gone away, but if not we
> > > need to investigate further.
> > 
> > Yes, I can still repo the 'short-list' following the STR in comment #2.
> 
> So the weird thing is that we have both this bug filed and bug 963078. It
> seems weird that both happen. :-\

Bug 963078 happens mostly with the history menu (which extends all the way to the bottom). But it really affects the entire panel and any of its subviews (when the window is moved to the bottom or the screen is small).

Comment 12

[Drew Willcoxon :adw]

As far as I can tell, the problem is simply that there's nothing telling panelMenu_bookmarksMenu to be at least a certain height, nor apparently its ancestor nodes.

Gijs, I can write a patch that sets a min-height or removes its flex=1, but I'm guessing it's more complicated than that.  (Like, we don't want the panel to grow past the edge of the screen.)  It looks like the bookmarks menu is unique, and there are no other widgets that expand into menu subviews in the panel, so there's no precedent for me to go on.

Comment 13

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #12)
> As far as I can tell, the problem is simply that there's nothing telling
> panelMenu_bookmarksMenu to be at least a certain height, nor apparently its
> ancestor nodes.
> 
> Gijs, I can write a patch that sets a min-height or removes its flex=1, but
> I'm guessing it's more complicated than that.  (Like, we don't want the
> panel to grow past the edge of the screen.)  It looks like the bookmarks
> menu is unique, and there are no other widgets that expand into menu
> subviews in the panel, so there's no precedent for me to go on.

I'm confused. There are several other widgets, such as the history subview and the character encoding subview. I don't know why they don't have the problem and the bookmarks one does, because AFAIK we use toolbarbuttons for the bookmarks menu as well, when it's used as a subview in the menupanel...

Comment 14

[Drew Willcoxon :adw]

Attachment: don't flex panelMenu_bookmarksMenu

Thanks for pointing out the other two similar subviews.  I overlooked them.

Maybe I'm missing something, but it looks like panelMenu_bookmarksMenu simply shouldn't flex.  PanelUI-historyItems and PanelUI-characterEncodingView's various vboxes don't flex.  And it doesn't look like there's any special handling required to prevent the panel from growing past the edge of the screen, and indeed with this patch the bookmarks subview scrolls when necessary.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8374349 [details] [diff] [review]
don't flex panelMenu_bookmarksMenu

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

Nice, ship it!

Comment 16

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/c3d4784309df

Comment 17

[Drew Willcoxon :adw]

Backed out for possibly causing bc test failures on Windows only:
https://hg.mozilla.org/integration/fx-team/rev/1ae495a16414

https://tbpl.mozilla.org/php/getParsedLog.php?id=34518905&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34518699&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=34519157&tree=Fx-Team

Comment 18

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #17)
> Backed out for possibly causing bc test failures on Windows only:
> https://hg.mozilla.org/integration/fx-team/rev/1ae495a16414
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34518905&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34518699&tree=Fx-Team
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34519157&tree=Fx-Team

And relanded because I'm pretty sure this was bug 969376's fault instead:

remote:   https://hg.mozilla.org/integration/fx-team/rev/0dbaff20d440

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0dbaff20d440

Comment 20

[:Gijs (he/him)]

Comment on attachment 8374349 [details] [diff] [review]
don't flex panelMenu_bookmarksMenu

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Bookmarks toolbar button menu doesn't display correctly
Testing completed (on m-c, etc.): on m-c for a week
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 21

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6797819e5d3

Comment 22

[svbabukumar]

Test day 24/2/2014

validated the bug in Aurora B29 and Nightly B30 

It is resulting as expected, I can see the scroll bar properly displaying