180156 - bookmark overflow chevron can overlap bookmark name text

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[djk]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021112 Phoenix/0.4
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021112 Phoenix/0.4

If you resize the window to the 'correct' width, the bookmark overflow chevron
can overlap the bookmark item to the left of it.

Reproducible: Always

Steps to Reproduce:
1. Horizontally resize the window so the chevron appears.
2. Continue to resize the window until the chevron overlaps the bookmark to the
left of the chevron.


Actual Results:  
The chevron overlaps the bookmark.
More specifically, the calculation which determines whether to put a bookmark
item into the overflow menu doesn't take the chevron button's width into account.

Expected Results:  
The chevron should not overlap the bookmark.
The calculation to determine if a bookmark item overflows should take the
chevron's width into account, if the chevron is being displayed.

Comment 1

[djk]

Attachment: screenshot of bug

Comment 2

[djk]

Attachment: possible fix

I'm not too sure if this is efficient, but it seems to work.

Comment 3

[David P James]

Another Bookmarks toolbar chevron problem :)

Possibly related to;
bug 177266
bug 177507

Thanks for the screenshot and the patch.

-->Confirming, marking as new, OS to all.

Comment 4

[Asa Dotzler [:asa]]

Comment on attachment 106246 [details] [diff] [review]
possible fix

dave, can you look at this patch and see if it's useful? thanks.

Comment 5

[djk]

Attachment: de-XBLified version of the fix

Comment 6

[djk]

maybe pch could look at this?  he's the px bookmark guru :-)

Comment 7

[Pierre Chanial]

djk: thx for the patch, I'll look at it as soon as I have time.
Currently... I haven't... :(

Comment 8

[djk]

Attachment: more efficient code

moved chevronWidth calculation out of the loop; no need to
uncollapse/recollapse the chevron, either.
(compared to previous patch).

Comment 9

[djk]

Attachment: hello, edge case (tweaked patch for an edge case I found)

The previous patch assumed that the chevron would always be uncollapsed when
the code to read its width was reached; that's not always the case.

This new patch gets the width, even if the chevron is collapsed at the time.

Comment 10

[[e-mail]]

Comment on attachment 118289 [details] [diff] [review]
hello, edge case (tweaked patch for an edge case I found)

>+    var chevronWidth = 0;
>+    if (chevron.collapsed) {
>+      chevron.collapsed = false;
>+      chevronWidth = chevron.boxObject.width;
>+      chevron.collapsed = true;
>+    } else {
>+      chevronWidth = chevron.boxObject.width;
>+    }
>     chevron.collapsed = true;
Why not just:
    chevron.collapsed = false;
    var chevronWidth = chevron.width;
    chevron.collapsed = true;
?

Comment 11

[[e-mail]]

Oops I meant chevron.boxObject.width of course :-) /me needs sleep

Comment 12

[djk]

neil:

Duh, of course!  I was so concerned with fixing the edge case I found, I missed
that obvious flaw.

I guess I was still thinking of my first patch on this bug, where the .collapsed
state needed to be restored after getting the width.

Comment 13

[djk]

Attachment: address redundant logic...

Comment 14

[Colin Ramsay]

This is still there in the FB 0.6.1rc1 - why is it that I keep seeing patches by
djk which aren't getting checked in?

Anyway, this problem still exists.

Comment 15

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 16

[djk]

> why is it that I keep seeing patches by
> djk which aren't getting checked in?

Hopefully because nobody has looked at them; not because they suck. ;-)

I've seen one of my patches get checked in recently, so it's probably just a
matter of time before we hear more about the others.

David

Comment 17

[José Jeria]

shouldnt this bug be assigned to jdk then?

Comment 18

[Simon Paquet [:sipaq]]

Attachment: djk's patch with bitrot removed

This is djk's patch in the de-bitrotted state.
djk, thanks for the patch. I hope you don't mind my doing.

Comment 19

[Simon Paquet [:sipaq]]

Comment on attachment 128692 [details] [diff] [review]
djk's patch with bitrot removed

Noririty, can you please review?

Comment 20

[djk]

> This is djk's patch in the de-bitrotted state.
> djk, thanks for the patch. I hope you don't mind my doing.

Not at all!  It's nice to see FB development picking up again.

David

Comment 21

[[e-mail]]

Comment on attachment 128692 [details] [diff] [review]
djk's patch with bitrot removed

>+      if (i == buttons.childNodes.length-1) {
>+        chevronWidth = 0;  // last ptf item...
>+      }
Two style nits:
1. Spaces before and after the - operator.
2. Style in this file is to have no {}s for a single statement.

Comment 22

[basic]

Attachment: basic's patch (a different approach)

here's a patch that uses a different approach. What it does it to check if the
previous button will still fit if the chevron is displayed.

Comment 23

[noririty]

Comment on attachment 128692 [details] [diff] [review]
djk's patch with bitrot removed

I like this way. checked in.

Comment 24

[noririty]

.

Comment 25

[Simon Paquet [:sipaq]]

verified