422607 - Bookmarks menu - there is space next to the lefthand submenu which makes selection hard

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[stony]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

A third submenu on the bookmarks menu, appearing on the left, has a significant space from the second submenu. In most cases when I try to select a link from the third submenu, the pointer "falls" into that space and the third submenu disappears - I can not select the desired link. In order to fulfill the task, I have to move the pointer much faster than usual, so it doesn't "fall" into that space. Might be valid for other menus as well.

Reproducible: Always

Steps to Reproduce:
1.Create two submenues with folders
2.Create a third submenu with links - there has to be sufficient number of links, so that the third submenu appears on the left, not on the right
3.

Comment 1

[stony]

Attachment: This is how the bookmarks menu third submenu looks like.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031302 Minefield/3.0b5pre

Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2007-08-26+19%3A00&maxdate=2007-08-28+00%3A00
and I have indications that it is from Bug 392160.

Comment 3

[Martijn Wargers (dead)]

This is worksforme, using current trunk build, using the classic theme. Maybe it depends on the theme?

Comment 4

[Ria Klaassen (not reading all bugmail)]

I'm using Luna but I also saw it on Vista. It is only a gap of 1 pixel I think and when you hover over that pixel the menu collapses. It is hard to see the difference.

It regressed around 09:00 so I think it is indeed from Bug 392160.

Comment 5

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

I can't reproduce on Vista Aero with latest trunk. I even got the cursor to change to a link-cursor when the gap was over a link, but the menu didn't collapse.

Ria: when you say it's hard to see the difference, do you mean that it's hard to trigger?

Comment 6

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #5)
No, I thought it was hard to see beforehand, but I tried again and it is not as difficult to see as I thought. I'll post a screen shot in a minute.

Comment 7

[Ria Klaassen (not reading all bugmail)]

Attachment: Comparison screen shot

Comment 8

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

This will not block the final release of Firefox 3.

Comment 11

[Kai Liu]

From bug 421657 comment 9, I see this too, but the reproduction is a little weird.  It happens at some screen resolutions but not others, and it happens with some submenus but not others.  But if a particular screen resolution + submenu combination does exhibit the bug, then it will happen every time (and on every version that I tried).

Moving to Core...

Comment 12

[Kai Liu]

Attachment: avoid unnecessary twip/pixel conversions

Yay, my guess was right: it's a problem with rounding.

Bug 392160 changed the popup repositioning code so that it was getting screen coordinates from GetScreenRect, which performs a twip-to-pixel conversion and returns the results in device pixels.  The popup positioning code uses twips, not pixels, so what gets returned from GetScreenRect needs to be converted back to twips.  For certain (typically large) values, a rounding error from these two conversions crops up and causes this bug that we see here.  This explains why reproduction was sporadic: rounding errors like this are by nature unpredictable.

What this patch does:
* Implement a new GetScreenRectInAppUnits function that avoids the twip-to-pixel conversion and returns a rect in twips.
* Make anchorScreenRect and rootScreenRect always be in twips in the popup repositioning code.  This avoids the unnecessary double-conversion and cleans up the popup positioning code slightly.

What this patch does not do:
* I wasn't sure if I should leave GetScreenRect alone or if I should re-implement that using a call to the new GetScreenRectInAppUnits to avoid code duplication between the two functions.  Because doing that would change when the conversion/rounding in GetScreenRect takes place, I decided to not touch that.

I don't have a Mac or Linux box to do builds with, so this patch been tested only on Windows.

Comment 13

[Kai Liu]

Attachment: use nsRect, eliminate GetScreenRect code duplication

Same patch as above, except...
* Since GetScreenRectInAppUnits isn't returning dev pixels, it should return nsRect instead of nsIntRect
* Reimplement GetScreenRect using a call to GetScreenRectInAppUnits to avoid code duplication (see comment above for why I didn't do this at first)

Comment 15

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

+  if (presContext->AppUnitsPerDevPixel() != aAnchorFrame->PresContext()->AppUnitsPerDevPixel()) {

Don't do this, do the conversion uncondtionally. There should be no precision loss here.

-      anchorScreenRect = aAnchorFrame->GetScreenRect();
-      xpos = presContext->DevPixelsToAppUnits(anchorScreenRect.x - rootScreenRect.x);
-      ypos = presContext->DevPixelsToAppUnits(anchorScreenRect.y - rootScreenRect.y);
+      anchorScreenRect = aAnchorFrame->GetScreenRectInAppUnits();
+      xpos = anchorScreenRect.x - rootScreenRect.x;
+      ypos = anchorScreenRect.y - rootScreenRect.y;

Isn't this changing the results? We used to use the menu frame's prescontext to do the devpixel-to-appunit conversion, now we're using aAnchorFrame's prescontext.

-  if (mInContentShell) {
-    rootScreenRect.ScaleRoundIn(presContext->AppUnitsPerDevPixel());
+  if (mInContentShell)
     rect.IntersectRect(rect, rootScreenRect);
-  }

You need the braces here

Comment 16

[Kai Liu]

Attachment: addresses comments

Comment 17

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

Comment on attachment 323971 [details] [diff] [review]
addresses comments

great!

Comment 18

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

7e550904db35

Comment 19

[Martijn Wargers (dead)]

(In reply to comment #18)
> 7e550904db35

What?

Comment 20

[Kai Liu]

(In reply to comment #19)
> (In reply to comment #18)
> > 7e550904db35
> 
> What?
> 

https://hg.mozilla.org/mozilla-central/index.cgi/rev/7e550904db35

Comment 21

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

Yeah, sorry, I thought bugzilla would linkify it.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified by using the default Bookmarks Toolbar sub menu under the Bookmarks Menu with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008062703 Minefield/3.1a1pre

Comment 24

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

Keeping the approval flag at ? so I remember to approve this for 1.9.0.2

Comment 25

[Samuel Sidler (old account; do not CC)]

Comment on attachment 323971 [details] [diff] [review]
addresses comments

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 27

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

Checked into 1.9.0 branch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Is there a reason we didn't rev the nsIFrame IID when we made changes to the interface here?

Comment 29

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

That was a mistake.

Of course, landing the vtable change on 1.9.0.x with or without an IID change was an even bigger mistake.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #29)
> That was a mistake.
> 
> Of course, landing the vtable change on 1.9.0.x with or without an IID change
> was an even bigger mistake.

So what has to be done to correct this mistake? Has it to be reverted?

Comment 31

[Martijn Wargers (dead)]

See bug 455283, it was fixed there.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tried to verify this bug with Firefox 3.0.2 on WinXP but wasn't able to reproduce this issue with Firefox 3.0.1. Even with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre I don't see it on the 3.0 branch. Anyone who can verify this one?

Comment 33

[stony]

Hurrah! Finally gone in 3.0.2. It's been quite annoying.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #33)
> Hurrah! Finally gone in 3.0.2. It's been quite annoying.

Thanks stony! Based on your verification I'll mark this bug as verified1.9.0.2.

Comment 35

[Boyd Brown]

This problem still exists for me. I am using FF 3.5.3 on Windows XP. Just updated today. It is the same problem I have experienced for a very long time. My previous version was 3.0.14.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #35)
> This problem still exists for me. I am using FF 3.5.3 on Windows XP. Just
> updated today. It is the same problem I have experienced for a very long time.
> My previous version was 3.0.14.

Please file a new bug if it is still present for you and attach a screenshot. Thanks.

Comment 37

[Boyd Brown]

Just checked again, and it only occurs when using the Pitchdark theme. No problem when using the default theme. I have already sent a message to Pitchdark developer.