Closed Bug 890374 Opened 7 years ago Closed 7 years ago

incorrect tooltips in the sidebar when interface are scaled

Categories

(Core :: Layout, defect)

22 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 + fixed
firefox24 + verified
firefox25 + verified

People

(Reporter: lain.halfbit, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

Attached image firefox_tooltip_bug.png
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Steps to reproduce are specific to Windows 7:
1. Go to Control Panel > Appearance and Personalization > Display.
2. Set scale to Medium (125%) or Larger (150%), apply and follow instructions (you will have to logout and login back).
3. Start Firefox and open History or Bookmarks in the sidebar (Ctrl+H, Ctrl+B).
4. Point mouse over any item with multiple items above it in the list.


Actual results:

Tooltip are shown but it contains information relevant to item somewhere above in the list and not that one you are pointing at.


Expected results:

Tooltip must show information relevant to the item you are pointing at.
Hardware: x86_64 → All
Blocks: win-hidpi
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
Component: Untriaged → Bookmarks & History
Replicated at 200% with Aurora 24 and Nightly 25.
Blocks 844604
Tracking and passing this on to Johnathan as this is recent regression.
I can also reproduce this on a Retina Mac running OS X. (The exact behavior also depends on the position of the window on the screen.) Appears to be caused by mixing device-pixel and CSS-px coordinates when initializing the popup window for the tooltip. This leads to the wrong coordinates being sent in the popupshowing event, so the Places code doesn't find the correct item to populate the tooltip. This patch fixes the problem for me on OS X; not yet tested on Windows, but I've pushed a tryserver job at https://tbpl.mozilla.org/?tree=Try&rev=95f51e788966 to see how that goes.
Comment on attachment 774617 [details] [diff] [review]
[hidpi] show the correct tooltip for items in the Bookmarks or History sidebar

Also fixes the bug for me on Windows, and tryserver reports that mochitests are happy with it (in particular, the assertion about having no rootPresContext is not hit). Is that a case that we should be concerned about, or should it be unreachable?
Attachment #774617 - Flags: review?(roc)
Component: Bookmarks & History → Layout
Product: Firefox → Core
Blocks: osx-hidpi
Comment on attachment 774617 [details] [diff] [review]
[hidpi] show the correct tooltip for items in the Bookmarks or History sidebar

Review of attachment 774617 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/nsXULPopupManager.cpp
@@ +660,5 @@
>        mCachedMousePoint -= widget->WidgetToScreenOffset();
> +  } else {
> +    // XXX can this case actually occur, or is it an error?
> +    NS_ASSERTION(false, "no rootPresContext, coordinates may be wrong");
> +    mCachedMousePoint = nsIntPoint(aXPos, aYPos);

Why aren't we using popupFrame->PresContext() to get the CSSPixelsToDevPixels ratio? I think we should be.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 774617 [details] [diff] [review]
> [hidpi] show the correct tooltip for items in the Bookmarks or History
> sidebar
> 
> Review of attachment 774617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/xul/base/src/nsXULPopupManager.cpp
> @@ +660,5 @@
> >        mCachedMousePoint -= widget->WidgetToScreenOffset();
> > +  } else {
> > +    // XXX can this case actually occur, or is it an error?
> > +    NS_ASSERTION(false, "no rootPresContext, coordinates may be wrong");
> > +    mCachedMousePoint = nsIntPoint(aXPos, aYPos);
> 
> Why aren't we using popupFrame->PresContext() to get the
> CSSPixelsToDevPixels ratio? I think we should be.

Uhh... just because that's how we've always done it? :) Since bug 595570, anyhow.

But yes, maybe we can simplify this. I was a bit concerned there might be cases where the popupFrame's presContext doesn't (yet) have the appropriate ratio set up. But in some testing of various popups on a retina Mac with external display, I was not able to see any problems.
Simplified patch that uses the popupFrame's presContext directly, instead of asking for the rootPresContext.
Attachment #775610 - Flags: review?(roc)
Attachment #774617 - Attachment is obsolete: true
Attachment #774617 - Flags: review?(roc)
Comment on attachment 775610 [details] [diff] [review]
[hidpi] show the correct tooltip for items in the Bookmarks or History sidebar

Review of attachment 775610 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/nsXULPopupManager.cpp
@@ +650,5 @@
>    // coordinates are relative to the root widget
> +  nsPresContext* presContext = popupFrame->PresContext();
> +  mCachedMousePoint.x = presContext->CSSPixelsToDevPixels(aXPos);
> +  mCachedMousePoint.y = presContext->CSSPixelsToDevPixels(aYPos);
> +  nsIWidget *rootWidget = presContext->GetRootWidget();

No, you still need to get the root widget from the root pres context.

I think it makes sense to use the popup's prescontext here for the coordinate conversion, since aXPos and aYPos should probably use the scale factors of aPopup's document.
Attachment #775610 - Flags: review?(roc) → review-
Attachment #775610 - Attachment is obsolete: true
Comment on attachment 776284 [details] [diff] [review]
(v.3)  - [hidpi] show the correct tooltip for items in the Bookmarks or History sidebar

Review of attachment 776284 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/nsXULPopupManager.cpp
@@ +654,3 @@
>    // coordinates are relative to the root widget
>    nsPresContext* rootPresContext =
>      popupFrame->PresContext()->GetRootPresContext();

use pc here
Attachment #776284 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/8304e62e12d4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 776697 [details] [diff] [review]
[hidpi] fix CSS/device coordinate mismatch in XUL popup manager, so that we show the correct tooltip for items in the Bookmarks or History sidebar.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Hi-DPI support

User impact if declined: Incorrect (or no) tooltip displayed when mouse hovers over items in the History or Bookmarks sidebar.

Testing completed (on m-c, etc.): Tested locally on both OS X and Windows hi-dpi configurations; now landed on m-c.

Risk to taking this patch (and alternatives if risky): The only change is to the coordinates passed to popup items from XUL, so risk of regressing anything else is minimal; and on non-hidpi configurations, there should be no change in behavior from existing code.

String or IDL/UUID changes made by this patch: none
Attachment #776697 - Flags: approval-mozilla-beta?
Attachment #776697 - Flags: approval-mozilla-aurora?
Attachment #776697 - Flags: approval-mozilla-beta?
Attachment #776697 - Flags: approval-mozilla-beta+
Attachment #776697 - Flags: approval-mozilla-aurora?
Attachment #776697 - Flags: approval-mozilla-aurora+
Marking this qawanted as I think it'd be good to get manual verification of the fix (across all of Fx23/24/25) once builds are available; AFAIK we don't yet have any significant test coverage for UI behavior on hidpi configurations.

See comment #0 for STR; applies to both Windows hi-dpi configurations and Retina Mac systems.
Keywords: qawanted
This will be regression tested as part of our next Beta (tonight).
Keywords: qawantedverifyme
Duplicate of this bug: 899933
I confirm the fix is verified on FF 24b1. BuildID: 20130806170643
Verifid fixed on Latest Aurora 25 BuildID: 20130908004001
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.