Closed Bug 575218 Opened 12 years ago Closed 12 years ago

Bookmarks button position is not updated on startup on Mac

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b1

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
looks like on Mac also width is 0, I'd say that dropping a new bookmark on an empty toolbar is pretty hard there!
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.
(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.
(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.
(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.
(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
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
note: the above does not fix the 0-width issue still.
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.
Attachment #454537 - Flags: review?(dao)
Attachment #454508 - Flags: review?(dao)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #454537 - Flags: review?(dao) → review+
different visibility check
http://hg.mozilla.org/mozilla-central/rev/e6055b0ebd2a

will move the other patch to another bug.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7b1
Attachment #454508 - Attachment is obsolete: true
Attachment #454508 - Flags: review?(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.
Attachment #454537 - Flags: review-
(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)
Blocks: 575437
filed Bug 575437 to handle the edge case in a better way that does not regress pinstripe.
To clarify, in some window I was seeing the button even if menu was not hidden.
No longer blocks: 575437
Depends on: 575437
You need to log in before you can comment on or make changes to this bug.