575218 - Bookmarks button position is not updated on startup on Mac

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

This is due to the fact the button checks for isElementVisible("personal-bookmarks") and finds its size is 0 because bookmarks toolbar is inited just later.
On other platforms personal-bookmarks has a min-height that makes this working even if bookmarks toolbar has not been inited.

Comment 1

[Marco Bonardo [:mak]]

So, possibilities:
- move min-height in pinstripe (Actually min-height in pinstripe is set on #PersonalToolbar rather than on #personal-bookmarks like in other themes)
- move toolbar initialization to BrowserStartup (but this would delay ui showing, that is for sure an unwanted effect)
- move back BMB.init() to delayed startup, just ensure that it runs before sessionstore is inited.

Comment 2

[Marco Bonardo [:mak]]

doing 3 would most likely mean users will see the button move on startup.
So I'd go for the simplest solution that is 1.

Comment 3

[Marco Bonardo [:mak]]

looks like on Mac also width is 0, I'd say that dropping a new bookmark on an empty toolbar is pretty hard there!

Comment 4

[Marco Bonardo [:mak]]

And 0 width is also reason for bug 575194, when element is moved to menubar flex=1 does not make it take up available space. We either need to set a min-width on all platforms, or a more robust way to detect if element is visible. And figure out why flex=1 on Mac does not work as expected.

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #3)
> looks like on Mac also width is 0

Not for me. Only the height is 0 over here.

> I'd say that dropping a new bookmark on an
> empty toolbar is pretty hard there!

Indeed!

(In reply to comment #1)
> So, possibilities:
> - move min-height in pinstripe

Sounds good to me. Do we also need a min-height on #PlacesToolbar, i.e. the hbox inside #personal-bookmarks? Without it, dropping a bookmark on an empty bookmarks toolbar doesn't work for me.

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #5)
> (In reply to comment #3)
> > looks like on Mac also width is 0
> 
> Not for me. Only the height is 0 over here.

Oh, you're probably talking about the state before bookmarks bar initialization, right? Don't know about that.

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> (In reply to comment #5)
> Oh, you're probably talking about the state before bookmarks bar
> initialization, right? Don't know about that.

yes, you can also try wit ha profile with no bookmarks in the bar, it should still be possible to drop a new bookmark there.

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #5)
> Sounds good to me. Do we also need a min-height on #PlacesToolbar, i.e. the

yes, it's probably better to only set it on #PlacesToolbar in all themes, places-bookmarks will inherit it.
Still on Mac before toolbar is initialized I find bo.width = 0, while with the min-height fix height is now correct

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.0

or something like this, since the height changes based on theme, but the fact internal hbox should inherit it is related to functionality and should not be something themes can play with.

Comment 10

[Marco Bonardo [:mak]]

note: the above does not fix the 0-width issue still.

Comment 11

[Marco Bonardo [:mak]]

Attachment: different visibility check

using a different check solves both the problems, isElementVisible is a but bogus for our case, not only on Mac but also on Windows if you move the bookmarks item to the menubar and the menubar has autohide, the width ends up being zero when the menubar is hidden.

Comment 12

[Marco Bonardo [:mak]]

different visibility check
http://hg.mozilla.org/mozilla-central/rev/e6055b0ebd2a

will move the other patch to another bug.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 454537 [details] [diff] [review]
different visibility check

(In reply to comment #11)
> Created an attachment (id=454537) [details]
> different visibility check
> 
> using a different check solves both the problems, isElementVisible is a but
> bogus for our case, not only on Mac but also on Windows if you move the
> bookmarks item to the menubar and the menubar has autohide, the width ends up
> being zero when the menubar is hidden.

This is intentional. If the bookmarks toolbar stuff is hidden in the menu bar, the button needs to be present in the nav bar.

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> This is intentional. If the bookmarks toolbar stuff is hidden in the menu bar,
> the button needs to be present in the nav bar.

it did not work reliably bringing to the button to appear sometimes and not appear other times, even with that expectation. The width was sometimes evaluated to 0 even if the element was visible and in place.
we can evaluate a better fix after the freeze, it was was just appearing not consistent across windows (plus causing this weird Mac issue)

Comment 15

[Marco Bonardo [:mak]]

filed Bug 575437 to handle the edge case in a better way that does not regress pinstripe.

Comment 16

[Marco Bonardo [:mak]]

To clarify, in some window I was seeing the button even if menu was not hidden.