Closed Bug 422607 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1a1

People

(Reporter: stonybulgaria, Assigned: kliu)

References

()

Details

(Keywords: regression, verified1.9.0.2)

Attachments

(3 files, 2 obsolete files)

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.
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.
Blocks: 392160
Status: UNCONFIRMED → NEW
Component: Bookmarks → Places
Ever confirmed: true
Flags: blocking-firefox3?
Keywords: regression
QA Contact: bookmarks → places
Version: unspecified → Trunk
This is worksforme, using current trunk build, using the classic theme. Maybe it depends on the theme?
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.
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?
(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.
This will not block the final release of Firefox 3.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Duplicate of this bug: 425810
Duplicate of this bug: 421657
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...
Component: Places → XP Toolkit/Widgets: Menus
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Product: Firefox → Core
QA Contact: places → xptoolkit.menus
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.
Assignee: nobody → kliu
Status: NEW → ASSIGNED
Attachment #321907 - Flags: superreview?(roc)
Attachment #321907 - Flags: review?(enndeakin)
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)
Attachment #321907 - Attachment is obsolete: true
Attachment #322085 - Flags: superreview?(roc)
Attachment #322085 - Flags: review?(enndeakin)
Attachment #321907 - Flags: superreview?(roc)
Attachment #321907 - Flags: review?(enndeakin)
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.1?
Duplicate of this bug: 407094
Blocks: 436779
+  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
Attachment #322085 - Attachment is obsolete: true
Attachment #323971 - Flags: superreview?(roc)
Attachment #323971 - Flags: review?(roc)
Attachment #322085 - Flags: superreview?(roc)
Attachment #322085 - Flags: review?(enndeakin)
Attachment #323971 - Flags: review?(roc) → review?(enndeakin)
Comment on attachment 323971 [details] [diff] [review]
addresses comments

great!
Attachment #323971 - Flags: superreview?(roc) → superreview+
No longer blocks: 422626
Attachment #323971 - Flags: review?(enndeakin) → review+
Keywords: checkin-needed
7e550904db35
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
(In reply to comment #18)
> 7e550904db35

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

https://hg.mozilla.org/mozilla-central/index.cgi/rev/7e550904db35
Yeah, sorry, I thought bugzilla would linkify it.
Attachment #323971 - Flags: approval1.9.0.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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 442595
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
Keeping the approval flag at ? so I remember to approve this for 1.9.0.2
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-
Attachment #323971 - Flags: approval1.9.0.1? → approval1.9.0.2?
Comment on attachment 323971 [details] [diff] [review]
addresses comments

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #323971 - Flags: approval1.9.0.2? → approval1.9.0.2+
Keywords: checkin-needed
Duplicate of this bug: 448279
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
Is there a reason we didn't rev the nsIFrame IID when we made changes to the interface here?
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.
(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?
See bug 455283, it was fixed there.
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?
Hurrah! Finally gone in 3.0.2. It's been quite annoying.
(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.
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.
(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.
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.
You need to log in before you can comment on or make changes to this bug.