512353 - Unnecessary chevron in bookmarks toolbar

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Drew Willcoxon :adw]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090824 Minefield/3.7a1pre

When I open Minefield with a new profile (with the default three bookmarks in the toolbar), the chevron appears at the right side.  When I click it nothing happens.  Doesn't occur on 3.6.

Comment 1

[Marco Bonardo [:mak]]

So this is probably regression from bug 382466, the only thing that can cause the chevron to appear is an overflow event on the scrollbox that is containing items (a pinstripe problem?). I tested the patch on Mac but it was not causing this, could be still that some last minute change broken that.

Comment 2

[Marco Bonardo [:mak]]

So, looking  at this with Drew, looks like it just happens witha new profile, but as soon as you add any bookmark and restart browser, then everything works like expected.

Comment 3

[Dão Gottwald [:dao]]

There might be a vertical overflow. event.detail will expose that.

Comment 4

[Marco Bonardo [:mak]]

sure, that's a really plausible cause, could be on pinstripe the bar has nota min height, so we overflow vertically.
Drew can you try patching the toolbar.xml code to ignore overflow/underflow events if they are vertical overflow? I can't test on Mac from here.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch

This ignores overflows that are purely vertical (detail == 0).  As Marco says, it's conceptually correct, but combined horizontal-vertical overflows (detail == 2) sometimes occur when it seems like they shouldn't.  e.g., with a new profile, hide the nav bar and restart.  A detail == 2 overflow occurs, quickly followed by an underflow.  With this patch, that makes the chevron briefly appear until the underflow occurs.

Comment 6

[Drew Willcoxon :adw]

Attachment: right patch this time...

Comment 7

[Marco Bonardo [:mak]]

both winstripe and gnomestripe define
#PersonalToolbar {
  min-height: 26px;
}
probably the value is wrong in pinstripe, but could you check if adding a min-height changes anthing about that flickering issue?

Comment 8

[Drew Willcoxon :adw]

Well, setting min-height does indeed get rid of the vertical portion of the combined overflow; now there's only a horizontal overflow. :\

Comment 9

[Marco Bonardo [:mak]]

the previous implementation had a min-width:1px, uncommented and apprently useless. could be that was needed for mac... so i'd say to try with that too.

Comment 10

[Dão Gottwald [:dao]]

I don't think the min-width would make a difference.
We should probably both take the min-height and ignore vertical overflow events.

Comment 11

[Marco Bonardo [:mak]]

Dao, that is fine, but we get an useless horizontal overflow that is still causing the useless chevron. See comment 8.

Comment 12

[Dão Gottwald [:dao]]

That's fine, as long as we get an underflow event as well.

Comment 13

[Marco Bonardo [:mak]]

we do, but we also get a Ts hit when that happens, because we apply a Places result to the chevron, that we don't need to. So if we can avoid that, we should do it.

Comment 14

[Dão Gottwald [:dao]]

I don't think a min-width will solve this. Maybe the Places result should only be applied when the chevron is clicked? Otherwise you get the same Ts hit when bookmarks actually overflow the toolbar.

Comment 15

[Dão Gottwald [:dao]]

Also, I suggest handling this in a separate bug.

Comment 16

[Marco Bonardo [:mak]]

it's applied on view, opened (queried) on popup opening. Yeah we could even apply it on first use i guess, but that would be a Tsnappiness issue than (minor one since it's mostly an if check).
Even if we cannot be sure about min-width, worth a try, if it just works won't be a problem having it. will take just 2s to try.

Comment 17

[Marco Bonardo [:mak]]

well i merely tried in a vm, so i could be wrong, i admit. Btw, adding a min-width: 1px completely solved the problem here. i  added it to as a scrollbox style, but actually could be that adding it to the personalToolbar style could be enough.
So please, someone on a real Mac, test that :)

Comment 18

[Dão Gottwald [:dao]]

The horizontal overflow is Mac-only? If so, why?

Comment 19

[Marco Bonardo [:mak]]

dunno, but if we can workaround it for now, i'd take a patch for that with a clear comment on why we are doing it and followup bug ref, and file a followup to discover why happens on Mac.

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Drew, could you please try this patch (is intended also to fix bug 515106 and another drop glitch i found about dropping between buttons)

I don't see a chevron flickering but i'm unsure about testing that, since this patch does not have any min-width set.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 399181 [details] [diff] [review]
patch v1.0

>             case "overflow":
>+              // Ignore purely vertical overflows.
>+              if (aEvent.detail == 0)
>+                return;

vertical underflows should probably ignored as well.

> .toolbar-drop-indicator {
>   list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
>+  /* set -moz-margin-start to -1/2 indicator width: */
>+  -moz-margin-start: -4px;
> }

Why that? Wasn't the code expected to deal with that? The indicator looks properly aligned to me without that margin.

> toolbarbutton.bookmark-item {
>   font-weight: bold;
>-  margin: 0 1px;
>-  padding: 0;
>+  margin: 0;
>+  padding: 0 1px;

This looks wrong. The min-height should be all what's needed to fix bug 515106.

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #21)
> (From update of attachment 399181 [details] [diff] [review])
> >             case "overflow":
> >+              // Ignore purely vertical overflows.
> >+              if (aEvent.detail == 0)
> >+                return;
> 
> vertical underflows should probably ignored as well.

doesn't make a difference, in underflow we just collpase the chevron, if it's already collapsed nothing will happen. If you want that just for completeness no problem.

> 
> > .toolbar-drop-indicator {
> >   list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
> >+  /* set -moz-margin-start to -1/2 indicator width: */
> >+  -moz-margin-start: -4px;
> > }
> 
> Why that? Wasn't the code expected to deal with that? The indicator looks
> properly aligned to me without that margin.

Yes but it's flickering on first drop on OS X, all buttons move right by half of the indicator, then they move back, but we uncollapse only after moving the indicator. Can't say why offhand, the code deals with the margin doing the same thing, i'm just adding the initial value consistent with code.

> > toolbarbutton.bookmark-item {
> >   font-weight: bold;
> >-  margin: 0 1px;
> >-  padding: 0;
> >+  margin: 0;
> >+  padding: 0 1px;
> 
> This looks wrong. The min-height should be all what's needed to fix bug 515106.

This is to fix another problem, actually when you drag on the empty part of the toolbar it consider that as a drop at the end of the toolbar, the problem is that pinstripe adds a 1px margin between buttons, that mens while you drag over this small 2px part, the drop indicator flickers between the right position and the end of the toolbar.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> > vertical underflows should probably ignored as well.
> 
> doesn't make a difference, in underflow we just collpase the chevron, if it's
> already collapsed nothing will happen.

If it's not collapsed yet, it will be collapsed incorrectly.

Comment 24

[Marco Bonardo [:mak]]

hm, i'm not sure when we could receive a spurious vertical underflow without a spurious vertical overflow... Btw, adding that ignore to underflow is just a minor thing, we can do that for code consistency, without hitting any issue.

Comment 25

[Dão Gottwald [:dao]]

The chevron actually disappeared from my bookmarks toolbar yesterday and never showed up again during that session, although I'm not sure it was caused by this.

Comment 26

[Marco Bonardo [:mak]]

assigning, per comparison with an old nightly the correct min-height must be 23px.
I still have to test the overflow/underflow flickering issue.

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v1.1

sounds like the min-height got rid of all spurious overflows.
I'm unable to reproduce the flicker, i tried with new profiles, old profiles, windowed or fullscreen. if they will still be reproduceable after this, we will need some more STRs.

i got rid of the start-margin changes, i'm unable to reproduce the flickers above atm, so no need to add something that i'm unsure fixes anything.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 399661 [details] [diff] [review]
patch v1.1

> #PersonalToolbar {
>   -moz-appearance: none;
>   background: url("chrome://browser/skin/bookmark_toolbar_background.png") repeat-x center center -moz-mac-chrome-active;
>   border-top: 1px solid rgba(255, 255, 255, 0.4);
>   border-bottom: 1px solid rgba(0, 0, 0, 0.57);
>-  padding: 2px 0 3px;
>+  min-height: 23px;
> }

I think the min-height belongs to #personal-bookmarks, since the item can be moved to a different toolbar. (The correct value will be 21px there, but you should test this yourself.)

> toolbarbutton.bookmark-item {
>   font-weight: bold;
>-  margin: 0 1px;
>-  padding: 0;
>+  margin: 0;

This makes the first item touch the toolbar border. Could be fixed by adding -moz-margin-start:1px to #personal-bookmarks.

>+  padding: 0 1px;

This should remain padding:0, I think. -moz-padding-start overrides part of it anyway:

>   -moz-padding-start: 7px;

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> I think the min-height belongs to #personal-bookmarks, since the item can be
> moved to a different toolbar. (The correct value will be 21px there, but you
> should test this yourself.)

*I think* the correct value will be 21px there. But as I said, please test this yourself.

Comment 30

[Marco Bonardo [:mak]]

Attachment: patch v1.2

sorry for late, i was recompiling on other platforms, i compared the heights on all three platforms with 3.5.3. On gnomestripe the min-height was low, was probably copied over from winstripe, notice this change does not increase the size.

afaict, everything looks fine

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 399725 [details] [diff] [review]
patch v1.2

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

> #PersonalToolbar {

>-  padding: 2px 0 3px;
> }

Note that the bottom padding was 1px larger than the top padding...

> toolbarbutton.bookmark-item {
>   font-weight: bold;
>-  margin: 0 1px;
>+  margin: 0;

Since you removed the paddings, this needs to be margin: 0 0 1px; in order to align the buttons properly.

Windows looks good, still need to test Linux.

Comment 32

[Marco Bonardo [:mak]]

i'll compare labels v-positions on Mac then to ensure they are fine

Comment 33

[Marco Bonardo [:mak]]

Attachment: patch v1.3

fixed the margin, compared labels v.position, everything correct.

Comment 34

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/73bf3507b3b1

Comment 35

[Marco Bonardo [:mak]]

Comment on attachment 399873 [details] [diff] [review]
patch v1.3

asking approval 1.9.2 because:
- it's a regression of already approved bug 382466
- it's fixing and blocking blocker Bug 515106

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 399873 [details] [diff] [review]
patch v1.3

a192=beltzner

Comment 37

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1616267e8153

Comment 38

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv