222274 - Up and down arrows displayed in Bookmarks menu even when it is impossible to scroll

Status: NEW

(Toolkit :: UI Widgets, defect, P4)

Description

[Gids Goldberg]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

When you have enough bookmarks to fill the page, an up arrow appears at the top
of the bookmarks menu even though it is impossible to scroll up.

Reproducible: Always

Steps to Reproduce:
1.Add enough bookmarks until they go below the bottom of the screen
2.Open the bookmarks menu
3.

Actual Results:  
An up arrow appears at the top of the bookmarks even though it is impossible to
scroll up.

Expected Results:  
The up arrow should only be shown once the user has scrolled down in the
bookmarks menu.

Comment 1

[Mike Connor [:mconnor]]

Both arrows appear, regardless of when in the scrolling range they are, not
quite perfect UI, but its minor

Comment 2

[Asa Dotzler [:asa]]

This sure would be a nice little cleanup item for 1.0. Probably not a blocker,
but if it's not difficult, we should try to do it.

Comment 3

[Martijn Wargers (dead)]

Attachment: Lame hack

This is just a lame hack, but it seems to work for me here. 

The onoverflow event gets fired too often, because it gets fired every time a
autorepeatbutton gets collapsed. I think I already saw a bug on this somewhere.


Unfortunately the getScrolledSize method of nsIScrollBoxObject is not
implemented in Mozilla. I suspect it would make it possible to write a better
patch.

Comment 4

[Blake Ross]

This isn't really a bookmarks bug, but maybe vlad can take a look at it anyways.
Vlad, if this isn't something you feel like looking into, feel free to go ahead
and minus it.

Comment 5

[Ross Shannon]

This bug has already been filed as Bug 76828, so one or other is a dupe. I don't
know which should have precedence, this one has a patch and the attention of
some productive engineers, but the other was reported earlier and is about both
the up and down scroll arrows being visible when they're not required, so it has
a broader scope; no point fixing one and not the other.

Comment 6

[Gids Goldberg]

Fair enough, but I was under the impression that Firefox Bookmarks UI code is
seperate from Seamonkey in which case there should be two bugs open.

Comment 7

[Ross Shannon]

Indeed, my mistake. However, the other bug's summary is better - this applies to
both the top and bottom arrows on the bookmarks menu, not just the top.

Comment 8

[José Jeria]

*** Bug 243365 has been marked as a duplicate of this bug. ***

Comment 9

[C. Fox]

I'm not going to try the blocking-? flag on this. But it would be nice to have
this squashed in 1.1

Comment 10

[Phil Ringnalda (:philor)]

*** Bug 307318 has been marked as a duplicate of this bug. ***

Comment 11

[Mats Palmgren (inactive)]

*** Bug 307647 has been marked as a duplicate of this bug. ***

Comment 12

[Phil Ringnalda (:philor)]

*** Bug 251723 has been marked as a duplicate of this bug. ***

Comment 13

[Phil Ringnalda (:philor)]

*** Bug 287542 has been marked as a duplicate of this bug. ***

Comment 14

[Andre Sihera]

Actually, I can confirm this bug, and found an instance where I thought I could
reproduce it. It is a general bug with context menus where, once the scrollers
are added to a menu they don't get removed, even if the number of items on the
menu is reduced to below the limit where they are required.

For example, I browsed to a page where there was an image-link. The image-link
placement was toward the bottom right corner of the screen and then I right-
clicked on it to Save it. In addition to the standard link options, there was
also the image options. The menu grew but for some reason was not rendered
upwards from the mouse point where it would have had space. Instead, it was
rendered downwards. Because there was not enough real-estate to display the
menu the scrollers were displayed in the menu.

However, when I went to click on a normal link in the middle of the screen again
I found that the scrollers were still on the menu, even though the menu items
were back to their normal quantity again.

I think this is definitely the same problem as this bug. If it isn't then let
me know and I'll file a new bug.

Cheers.

Comment 15

[Håkan Waara]

Almost finished with a patch for this.

Comment 16

[Håkan Waara]

Attachment: patch to firefox and xul core code

So here's the fix!

I have implemented nsScrollBoxObject::GetScrolledSize and created a new
updateArrows method in scrollbox.xml, that will keep track of when to show/hide
the arrows. 

It also works for horizontal <arrowscrollbox>es (which is, so far, only
half-implemented; see bug 103723).

Comment 17

[Martijn Wargers (dead)]

Håkan, that's great! I think you can safely remove all the parseInt() code, though.

Comment 18

[[e-mail]]

Comment on attachment 200282 [details] [diff] [review]
patch to firefox and xul core code

As well as these nits and questions I also agree with the parseInt comment.

>-          this.mScrollBoxObject.scrollByIndex(lines);
>+          
>+          this.mScrollBoxObject.scrollByIndex (lines);
>+          this.updateArrows ();
Please don't put spaces before function parens :-P

>+          var realWidth={}, realHeight={};
But please put them around = signs...

>+          this.mScrollBoxObject.getScrolledSize (realWidth, realHeight);
Not that I object to you implementing getScrolledSize but I how (if at all)
does it differ from the size of
document.getAnonymousNodes(kids[1])[0].boxObject?

>+            var shownHeight = this.boxObject.height;       
I think you would get more accurate results by using the
kids[1].boxObject.height here.

>+            this.beginscroll.collapsed = (dTop <= upArrHeight);
I don't see the need to adjust the top scroll position for the arrow height.
(For the bottom scroll postion I guess the problem is that you're using the
wrong height?)

Comment 19

[Martijn Wargers (dead)]

I've tried the patch, it works ok, but there is a small issue when I'm at the
bottom and the bottom autoscrollmarker disappears. In that case the last
menu-item is not directly fully shown (only after a mousemove).

Comment 20

[Martijn Wargers (dead)]

Ok, this is what I get directly after the autoscrollmarker has disappeared:
http://wargers.org/mozilla/Bug_222274/Clipboard1.gif
This is after I've moved the mouse a little:
http://wargers.org/mozilla/Bug_222274/Clipboard2.gif
See the difference? In the first image the "Open in Tabs" menu-item is not
completely shown as it should be.

I found another (small) issue, see:
http://wargers.org/mozilla/Bug_222274/Clipboard3.gif
I get a autoscrollmarker down arrow in this case, when it should not happen, but
only when I open it the first time. See also bug 285476, it sort of resembles
that bug.

Comment 21

[Håkan Waara]

Thanks for the comments, I'll see what I can do.

Comment 22

[Håkan Waara]

So working on this I am realizing that maybe the best solution would be making the arrows float above the actual <scrollbox>, that would then take up all the space in the <arrowscrollbox>. I tried with position:fixed and z-ordering, but haven't gotten it to work.

That way we'd avoid the problems of hiding/showing the arrows actually changing the size of the menu, which makes things kind of complex.

Comments?

Comment 23

[[e-mail]]

That won't work with the current XUL layout because a) XUL doesn't support fixed position or Z-ordering except for the special case of <stack> and b) scrolled areas use views which screw up the paint order.

The other issue is that the scrolled area would need to allow for the floating arrow so that you could scroll through menuitems with the keyboard.

Comment 24

[Håkan Waara]

Attachment: patch v2

Alright, all the issues are now resolved and also the code looks and works much better! 

I skipped the implementation of getScrolledSize() to, though it -- after some testing -- gives the same values (and also should).

Comment 25

[Martijn Wargers (dead)]

(In reply to comment #24)
> Alright, all the issues are now resolved and also the code looks and works much
> better! 
Are you sure?
With this patch, I'm still getting the issue mentioned in comment 19.
Some code that fixes it (but not necessarely a good way):
Instead of:
            this.endscroll.collapsed = (dBottom <= 0);

I used:
            this.endscroll.collapsed = false;
            var dBottom = contentSize.height - (scrollY.value + upArrHeight + scrollHeight + downArrHeight);
            if (dBottom <= 0) {
              this.endscroll.collapsed = true;
              this.mScrollBoxObject.scrollToElement(this.parentNode.lastChild);
            }
I suspect bug 119738 is the cause of this problem.

Also, Autorepeatbuttons can have a margin and that's not something that is calculated in the boxObject height/width, I believe.

> I skipped the implementation of getScrolledSize() to, though it -- after some
> testing -- gives the same values (and also should).
Well, this is something that Mozilla needs to have anyway, regardless whether you use it or not. So please keep it in the patch or make a separate patch for it.

Comment 26

[Martijn Wargers (dead)]

Also:
       <method name="scrollByIndex">
         <parameter name="lines"/>
         <body><![CDATA[
+            this.updateArrows ();
+            this.mScrollBoxObject.scrollByIndex (lines);
I think you can remove the this.updateArrows () (remember, no space before function parens), since scrollByIndex triggers a scroll event, which should call updateArrows 'automatically'.

Comment 27

[Håkan Waara]

Thanks Martijn for your comments.

1. Regarding the issue mentioned in comment 19, are you sure your tree isn't patched in some way? I haven't been able to reproduce it. What platform / theme are you using?

I will have a closer look at your solution once home.

2. I was working on the event handling yesterday, and I'm not sure scroll is triggered in all cases. If possible I will remove this extra call though, will confirm it once home.

3. What is the way to also include margins in the height/width calculation?

Comment 28

[Martijn Wargers (dead)]

(In reply to comment #27)
> 1. Regarding the issue mentioned in comment 19, are you sure your tree isn't
> patched in some way? I haven't been able to reproduce it. What platform / theme
> are you using?
I'm quite sure my tree is correctly patched. I'm using winxp default theme and _not_ using the luna theme. Try it once with a different window position and see if you then encounter the bug.

> 3. What is the way to also include margins in the height/width calculation?
Well, I'm not sure if that's really something that you need to handle. Maybe I was talking rubbish. Probably Neil would know more. 

Comment 29

[Håkan Waara]

I have not been able to reproduce the bug you're seeing Martijn. Can you try with another tree to confirm? Or if someone else could reproduce your bug with or without my patch.

I've found that the "ensureLineIsVisible"-stuff for menus is broken though, which I will look into this weekend.

Comment 30

[Håkan Waara]

Attachment: WIP patch

Ok so this is just a work-in-progress patch for testing.

I've been fighting with this once again -- it's so much messier than you might think.

Basically the problem has been that since we're listening on scroll events, which might hide/show the scroll arrows, we actually mess up the scroll when we do that so menuitems end up getting scrolled slightly wrong.

The fix is to re-scroll to account for our arrows.

I've been worried about this since it triggers another scroll event, that we - once again - pick up, but there doesn't seem to be any trouble.

Note to Martijn: you'll need to hardcode the height of the scroll arrows to test this patch. Other than that it should hopefully fix your issues.

Comment 31

[Håkan Waara]

Attachment: Best patch so far

Alright, finally.  

News in this patch:

* I have now fixed the calculation of the scroll arrows' sizes to include margins.
* The bug Martijn pointed out with some space at the bottom is fixed after much hair-tearing and sleepless nights.

The patch has been tested against a testcase in bug 95413 (both trying out a horizontal <arrowscrollbox> and a vertical one). It has also been tested against the bookmarks menu, both by using the mouse and using the keyboard.

Comment 32

[Martijn Wargers (dead)]

I've tried the "Best patch so far" patch and it seems to work fine without any errors.

Comment 33

[Håkan Waara]

Attachment: Revised patch

Revised patch with Neil's review comments:

* Changed to in-house coding style.
* Removed some unneeded code checking for the orient.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+                if (this.beginscroll.collapsed) {
+                    toggleBeginArrow = true;
+                    dScroll += this.upScrollHeight;
+                }

So when you show the up-arrow, you scroll *down* to compensate for its height? Shouldn't you be scrolling *up*? Showing the arrow moves the body down so you need to scroll the body up to compensate.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

this.upScrollHeight += 
+                    parseInt(this.beginscroll.ownerDocument.defaultView.getComputedStyle(this.beginscroll, "").getPropertyValue("margin-top"));

I think you could write a couple of helper functions to avoid duplicating this nasty bit of code.

Comment 36

[Håkan Waara]

(In reply to comment #34)
> +                if (this.beginscroll.collapsed) {
> +                    toggleBeginArrow = true;
> +                    dScroll += this.upScrollHeight;
> +                }
> So when you show the up-arrow, you scroll *down* to compensate for its height?
> Shouldn't you be scrolling *up*? Showing the arrow moves the body down so you
> need to scroll the body up to compensate.

No, this is correct. When the up-button is revealed is always during a down-scroll - and since we make the scroll area exactly |this.upScrollHeight| smaller we need to compensate for this by scrolling it down so we scroll to the position we were told to scroll to.

Comment 37

[Håkan Waara]

Attachment: Patch v6

This patch adresses the comment by roc and implements a _getMargin() helper function, which cleans up the code a bit. Other comments?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 202799 [details] [diff] [review]
Patch v6

>Index: toolkit/content/widgets/scrollbox.xml

>     <implementation>
>+      <property name="scrollBoxObject">
>+      <getter><![CDATA[
>+        if (!("mScrollBoxObject" in this)) {
>+            var kids = document.getAnonymousNodes(this);
>+            this.mScrollBoxObject = kids[1].boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+        }

nit: toolkit convention is to initialize mScrollBoxObject as a <field>null</field>, and then just check if (!this.m...). Two space indentation instead of four space, too (same for the rest of the file).

...
>+      <method name="updateArrows">
>+      <body><![CDATA[
>             var kids = document.getAnonymousNodes(this);
>-            this.mScrollBoxObject = kids[1].boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+        var toggleBeginArrow=false, toggleEndArrow=false;

Nit: spaces around "=".

...
>       <handler event="overflow"><![CDATA[
>         var kids = document.getAnonymousNodes(this);
>-        kids[0].collapsed = false;
>-        kids[2].collapsed = false;
>+           
>+        // cache the up/down/left/right arrows' sizes for later.
>+        if (!("beginscroll" in this)) {

Same comment as above, and this should be mBeginScroll (and mEndScroll), no?

Comment 39

[Håkan Waara]

This patch is now entering its second month of waiting for second-review...

Comment 40

[[e-mail]]

Comment on attachment 202799 [details] [diff] [review]
Patch v6

I'm mainly marking this - because I can't seem to use the mouse to scroll back up to the first menuitem - the arrow disappears when it scrolls to the second menuitem. I also wrote a test case with a variable sized scrolling region and it doesn't hide the arrows when I think it should, but I guess that's our lame overflow handling.

>+      <method name="_getMargin">
I don't actually think this is necessary. No skin gives the buttons a margin and that is because the margin would remain when the button is collapsed.

>+        var toggleBeginArrow=false, toggleEndArrow=false;
Nit: spaces around =s
Actually I find these variables confusing. I'd prefer either a) collapseBegin/EndArrow variables which would be set to what you will set the collapsed property to later b) set the collapsed property immediately, if this is practical.

>+               In the case that the y position is less than or equals the up-button, we hide it.
I think this is your issue - I feel that the up (left) button should only be hidden when the y position becomes zero. On the other hand your calculation for the bottom (right) arrow looks to be correct.

>+           We scroll to accommodate for the change we make. This is important, because for example a 
>+           menupopup might have a triggered the scroll in order to show a certain item. If we hide/show 
>+           the up or down arrow, then this item might not be visible unless we re-scroll exactly 
>+           the amount we changed. 
Just to ensure that we're on the same wavelength,
a) scrolling down, hiding the down arrow, need to scroll up
b) scrolling down, showing the up arrow, need to scroll down
c) scrolling down, simultaneous hide and show, no change is needed
d) scrolling up, no change is needed

>+        if (dScroll != 0) {
>+            if (this.orient == "vertical")
>+                this.scrollBoxObject.scrollBy(0, dScroll);
>+            else if (this.orient == "horizontal")
>+                this.scrollBoxObject.scrollBy(dScroll, 0);
>+        }
Might it be worth having separate hScroll and vScroll variables?

Comment 41

[Håkan Waara]

(In reply to comment #40)
> (From update of attachment 202799 [details] [diff] [review] [edit])
> I'm mainly marking this - because I can't seem to use the mouse to scroll back
> up to the first menuitem - the arrow disappears when it scrolls to the second
> menuitem. I also wrote a test case with a variable sized scrolling region and
> it doesn't hide the arrows when I think it should, but I guess that's our lame
> overflow handling.

I haven't seen this bug - nor am I unfortunately able to test it now, being on Mac OS X. :-/

> 
> >+      <method name="_getMargin">
> I don't actually think this is necessary. No skin gives the buttons a margin
> and that is because the margin would remain when the button is collapsed.

This is necessary, because I need to know exactly how high/wide the scroll arrows are in order to be able to know if a menuitem would fit in that area. Actually, if I don't take that into account the width/height diffs about 4px, so you get very buggy behavior.

> 
> >+               In the case that the y position is less than or equals the up-button, we hide it.
> I think this is your issue - I feel that the up (left) button should only be
> hidden when the y position becomes zero. On the other hand your calculation for
> the bottom (right) arrow looks to be correct.

It would be easy to have this behavior (hide arrow when y position is zero) but it would look bad, I think. Because then you scroll all the way until the last item is totally visible, and only *then* the arrow is hidden. This makes all items jump upwards a step when the arrow is hidden.

> 
> >+           We scroll to accommodate for the change we make. This is important, because for example a 
> >+           menupopup might have a triggered the scroll in order to show a certain item. If we hide/show 
> >+           the up or down arrow, then this item might not be visible unless we re-scroll exactly 
> >+           the amount we changed. 
> Just to ensure that we're on the same wavelength,
> a) scrolling down, hiding the down arrow, need to scroll up
> b) scrolling down, showing the up arrow, need to scroll down
> c) scrolling down, simultaneous hide and show, no change is needed
> d) scrolling up, no change is needed

Yes, that's actually what the code is modelled after.

All your other comments I could incorporate.  Unfortunately, like I said, I'm right now on OS X so it's kinda hard to make major changes to the patch and test them without help.  I'll see if I can find some other box to work on -- or if anyone else wants to volunteer to help, please contact me.

Comment 42

[Håkan Waara]

I'm thinking of maybe making a firefox 2-only patch that will be simpler than the current patch. 

It would not include the advanced calculations of when to show/hide the arrows when the first or last menuitem is only partially visible. This case is what is causing weird bugs and headache-prone code.

So, personal target for some version of this is ff2.

Comment 43

[Asaf Romano (gone)]

In bug 342841 we decided to disable the arrows (in the tabbrowser case). This is now implemented and has also affected the menupopup case. I see this as an improvement, but hiding the arrows is the right thing to do for menus (at least on OS X & Windows).

We should probably add a property to the scrollbox binding for toggling between the two behaviors.

Comment 44

[Ray Kiddy]

The patch above is from version 1.3 of the file. That file is now at version 1.23.

Is anything from this patch meaningful? I am not sure how it can be. Waiting long enough to take a patch will make it invalid.

Should the review approvals be removed at some point?

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S4. However, the bug has 5 duplicates.
:mstriemer, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 46

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.