Closed Bug 311675 Opened 19 years ago Closed 17 years ago

Personal bookmarks disappear if location bar is added to the same toolbar.

Categories

(Firefox :: Toolbars and Customization, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED WORKSFORME

People

(Reporter: mossop, Unassigned)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

If the location bar is placed on the same toolbar as the personal bookmarks toolbar the bookmarks is no longer visible, not even the overflow chevrons. I believe that this is fallout from bug 243078 since it does not occur in branch, but does occur in the branch builds with that patch that are floating around.
This is, for the most part, the correct behaviour with the existing (stupid) flex values. The urlbar is set to flex 1000 and the bookmark toolbar item is just 1. The only relevant fix to my patch is to set a minimum width on the bookmark toolbar to make sure the chevron appears.
Here is what I see with what revealed the problem to me. I have the personal bookmarks and another toolbar item supplied by an extension. Both have a flex of 1. The toolbar is wide enough to display both happily, but now it seems to display the personal bookmarks using a third of the toolbar. The second item uses the rest of the toolbar, even though its actual content only takes up about half of that.
That's not the same as the bookmarks toolbar needing a min-width at all...
Attached patch get rid of flex="1000" (deleted) — Splinter Review
Why was the flex ever set to 1000? Why can't it be 1? Setting it to 1 has no negative effect AFAIK on the default arrangement of toolbars, and moving the urlbar down to the bookmarks one with a flex of 1 gives both toolbars equal spacing.
Assignee: nobody → tonglebeak
Status: NEW → ASSIGNED
Attachment #199135 - Flags: review?(mconnor)
Attachment #199135 - Flags: superreview?(bryner)
Comment on attachment 199135 [details] [diff] [review] get rid of flex="1000" Can you ask ben to sr this instead? This might be fragile, and he'd have a better idea.
Attachment #199135 - Flags: superreview?(bryner)
Attachment #199135 - Flags: superreview?(bugs)
This breaks anyone foolish enough to have a flexible space on the same toolbar as the location bar, right?
a bit more common use case: location bar and bookmarks toolbar item on the same toolbar?
(In reply to comment #6) > This breaks anyone foolish enough to have a flexible space on the same toolbar > as the location bar, right? I'm not sure what you mean by break, because it seems to me that space is merely distributed evenly. (In reply to comment #7) > a bit more common use case: location bar and bookmarks toolbar item on the same > toolbar? That's what I tested after setting the flex to 1, and both toolbars got equal space in their parent toolbar. That said, I don't know if 1:1 is what we're looking for or not, but since I've yet to be able to get an answer from drivers...
The problem as I see it is that flex should only affect the free space on the toolbar, so a flex of 1000 on the location bar would be fine and would fill up all the remaining free space on a toolbar. This is what I thought used to happen and what xulplanet suggests the flex attribute does. But this does not seem to happen in this case. This is now broken on the 1.8 branch with the native theme landing.
Flags: blocking1.8rc1?
Blocks: 243078
This should be fixed on the branch. If it's not fixed in today's respins, then please renominate this.
Flags: blocking1.8rc1?
Well the chevron now shows. Personally I don't consider that as fixed but resolve as you will.
James, Why did you add "overflow: hidden;" to bookmarks-stack? Without it, the bookmarks toolbar doesn't expand to fill the available width as it did before, which I'd argue is better behavior, if not more "correct".
I've already explained that, and I'm not doing so again.
(In reply to comment #13) > I've already explained that, and I'm not doing so again. Unless you count bug 243078 comment 151 as an explanation, I certainly haven't seen one. Seeing as how various patches and explanations for various problems are being thrown all over the place in different bugs (311798, 311675, 243078), it sure wouldn't hurt to have a clear explanation of why this behavior regressed and what the alternative is.
I think James was referring to comment #1 which says the problem isn't overflow. The problem has to do with flex. I've tried other values for overflow using userChrome.css and none of them correct this problem.
David Hyatt seems to have been onto something in bug 105607 - Implement the ability for flex to only apply to expansion or contraction.
Alright, I'll explain again. The overflow setting is needed on the bookmarks toolbar item so that its size is not affected by its current contents, which would otherwise prevent it correctly working out what items can fit (with my non-hacky code). The old code tried to guess at how much space was 'free' by checking the size of [almost] all items found on the same toolbar row. This had two problems: 1. It screwed up when the toolbars got borders in the corrected Classic theme (it assumed, for example, that the toolbar's content area was the same size as the window's width). 2. It was majorly broken in RTL, mis-calculating the items that fitted almost every time. To fix that, I re-wrote the code, removing the hacks, so it simply used the size of itself to work out how many items can be shown. Clearly, this would normally run in to trouble if you make the window smaller, as the toolbar item would still be as wide as the items currently visible. Setting overflow: hidden dealt with that, by removing that constraint of layout. The result was that the bookmark toolbar item sized naturally, and displayed whatever items fitted in to its area. Adding the min-width to stop the chevron ever being squashed into nothing is a good fix that is appropriate. Whether any flex values needs to be changed is a separate, and much harder, issue.
(In reply to comment #17) > Alright, I'll explain again. Thanks for explaining. The new behavior is much more reliable, I'm sure (I remember trying to fix the various issues with chevron placement and resize events, it was a mess), but it does cause problems in some cases. As an example, the old code allowed placing the toolbar between two flexible spaces to have it centered on the toolbar but still take up as much space as it needed. This new code results in it taking up a third of the toolbar, leaving spaces on either side. While the new behavior is correct, given the flex values for the elements, I think it's less useful. I'm not going to make a bigger deal out of this than I already have, if it's my only issue with the fix I can surely live with it.
Yes, the change in behaviour in some situations is not really desirable, and perhaps the code could be refined to work more like it used to. My concern at the time was that the code was layer upon layer of hack and tweak, especially the attempts to make it work in RTL, and adding another hack on top (which I wasn't even sure I could manage to make work) seemed the wrong way to deal with it. I think someone who is a peer of Firefox needs to state definatively what behaviour they expect in cases where the bookmarks share a row with: - flexible space, and - the urlbar Then we can move forward and I or someone else can figure out the cleanest way to do it, and proceed to fix it.
This is the incorrect behavior with bookmarks and address bar on same toolbar with flexible spaces. I will post the correct behavior next. :-)
This is the correct behavior with bookmarks and address bar on same toolbar with flexible spaces. Flexible spaces when used should continue to press the bookmarks further towards the left (in this setup). Before this bug occured the address bar seemed to have a min width which is fine (if you can't seer the word attachment when making an attachment at 1024 then time to add a second bookmarks toolbar, perhaps a new feature or extension?).
Incrementally updated to Gecko/20051015 and it seems fixed to me! :-)
it's now worksforme since 243078 was backed out (unfortunately).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → WORKSFORME
(In reply to comment #23) > it's now worksforme since 243078 was backed out (unfortunately). This issue still exists on trunk.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee: tonglebeak → nobody
Status: REOPENED → NEW
please disregard and remove my "blank strip" attachment. It was for another bug, but apparently I was viewing the wrong bug when I submitted it.
Attachment #202086 - Attachment is obsolete: true
Comment on attachment 199135 [details] [diff] [review] get rid of flex="1000" I don't think this is the right fix, and the consequences aren't clear. Not having a minimum width for the BTF is a problem that this doesn't solve.
Attachment #199135 - Flags: superreview?(bugs)
Attachment #199135 - Flags: review?(mconnor)
Attachment #199135 - Flags: review-
*** Bug 312203 has been marked as a duplicate of this bug. ***
I see this bug on branch. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8) Gecko/20051111 Firefox/1.5 Reproducable: Always Steps to reproduce: 1) Create a new firefox profile and start Firefox. For me, Firefox comes up about 2/3 of the screen (I'm running 1280x1024 96dpi.) 2) Maximise Firefox. 3) View -> Toolbars -> Customise. Drag the bookmark toolbar items to the very end of the navigation toolbar, after the google search box. 4) "Restore down" (not quite sure the correct term for that) Firefox so the window gets smaller, the bookmarks disapear, replaced by two little arrow things (apparently called a chevron.) 5) Maximise the window. This shouldn't happen in the first place - the location bar should be compressed instead of the bookmarks, and secondly, the bookmarks don't come back after maximising the window until you restart Firefox.
This works fine for me on Vista running Firefox 2 Beta 2.
Summary: Personal bookmarks disappear if location bar is added to the same toolbar. → Personal bookmarks
Summary: Personal bookmarks → Personal bookmarks disappear if location bar is added to the same toolbar.
Attached image Screenshot of current trunk behavior (deleted) —
This appears to be fixed on the trunk nowadays. WORKSFORME?
This bug was resolved for me about a week ago. I manually check for updates almost daily. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070527 Minefield/3.0a5pre
Resolving as WORKSFORME based on comments. Please reopen if you can reproduce in current trunk builds.
Status: NEW → RESOLVED
Closed: 19 years ago17 years ago
Resolution: --- → WORKSFORME
marking VERIFIED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: