932317 - Image preview tooltip arrow position incorrect

Status: RESOLVED DUPLICATE of bug 1276876

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

Attachment: Screen Shot 2013-10-29 at 3.29.20 PM.png

Depending on how the devtools are docked or resized and how the firefox window is resized or moved around the screen, it may happen that the image preview tooltip arrow is incorrectly positioned.

One way to reproduce (although there seems to be others):

- Go to https://wiki.mozilla.org/Modules/All
- Open the devtools, docked to the right-hand side
- Make sure Firefox is maximized on screen
- Inspect the top left corner of the page
- In the css rule-view panel, hover over the background image

=> The tooltip appears but the arrow is incorrectly positioned

See the attachment.

Comment 1

[Patrick Brosset <:pbro>]

Here is another incorrect tooltip position scenario that seems to occur:

- inspect an image node in the markup-view
- hover over its src attribute
- make sure there's a DOM mutation on the image node before the tooltip appears (an attribute change will do)
- notice that when the tooltip appears, it is misplaced.

STRs reported by Paul Rouget:

- go to http://www.mozilla.org/en-US/
- first, inspector the moving banner (to make sure it's open in the markup view)
- then, inspector the firefox log
- hover the image to see the popup
- repeat until the banner moves right between the moment where you over the image and the popup shows up

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Patrick - can you add an estimate of difficulty to this?

Comment 4

[Patrick Brosset <:pbro>]

I'm going to go with medium because I'm suspecting a xul bug here, but I can't be sure.

Comment 5

[Julian Descottes]

Spent some time debugging this and I think I found a lead. 

In the XUL popupPanel used by Tooltip.js (toolkit/content/widgets/popup.xml#panel), the "arrow" is positioned using "adjustArrowPosition". In the flow used to display the Image preview tooltip on mouseover, the call to adjustArrowPosition comes from an event handler on the "popupshowing" event.

At that time, popupPanel.alignmentPosition is still "before_start", which is the default value.

If the popup can't be displayed at "before_start", the position will be changed (see nsMenuPopupFrame::SetPopupPosition). But this will be done after the "popupshowing" event is fired.

This is the basic problem : we need to wait for SetPopupPosition to be called on the popupPanel.popupBoxObject, otherwise we don't know in which position the popup will really be displayed and we can't create the arrow correctly.

We have another event, "popupshown" which occurs after the popup is displayed, but still right before the popup position is updated (see nsMenuPopupFrame::LayoutPopup and nsMenuPopupFrame::ReflowFinished).

On my machine, moving the adjustArrowPosition to "popupshown" already improves the situation. Only the first display of a tooltip preview is "wrong", afterwards the arrow is displayed correctly.
On top of that, firing another popupShown event in nsMenuPopupFrame::ReflowFinished seems to fix the issue.

I see two options :
- do adjustArrowPosition on "popupShown" and fire popupShown only after the popup is completely initialized and positioned
- create a new event "popupPositionChanged" (or any other name) that is triggered when the position of the popup is updated and plug adjustArrowPosition on this event

@Patrick : I could try to work on this bug but will need mentoring on the XUL/C++ parts.

Comment 6

[Patrick Brosset <:pbro>]

Based on the number of commits on related xul c++ files, I'm going to forward this to Neil Deakin.
Neil: could you take a look a Julian's investigation of this problem in comment 5 and hopefully guide him from here?

Comment 7

[Neil Deakin]

The issue generally is that the adjustArrowPosition function which determines the position of the arrow is dependent on knowing the position and size of the popup before doing this, yet the adjustArrowPosition function can cause the position and size of the popup to change due the adjustment of the arrow position/visibility, resulting in the process needing to be repeated, possibly infinitely.

We can't move this into popupshown as it is too late, but a new event may be a possibility, but it should only fire once and only while showing the popup. It might fix this specific case but wouldn't fix the overall problem.

Comment 8

[Julian Descottes]

Thanks Neil. You're right we need to have the arrow properly displayed to position the popup properly the first time !

Let's say we call adjustArrowPosition both on "popupshowing" and "popupshown".

As I understand, the arrow visibility and popup orientation (vertical/horizontal) are fixed for a given popup. What can change is the relative position "up or down" / "left or right", but this cannot impact the size of the popup, right ?
In this case, could adjustArrowPosition trigger infinite position recalculation ?

Following this idea, :
- on "popupshowing" we care about setting visibility and orientation of the arrow (it impacts the popup size)
- on "popupshown" we care about positioning the arrow up/down or left/right depending on the popup position

=> What about splitting the arrow initialization in 2 separate steps, one for each event ?

Comment 9

[Neil Deakin]

(In reply to Julian Descottes from comment #8)
> Thanks Neil. You're right we need to have the arrow properly displayed to
> position the popup properly the first time !
> 
> Let's say we call adjustArrowPosition both on "popupshowing" and
> "popupshown".
> 
> As I understand, the arrow visibility and popup orientation
> (vertical/horizontal) are fixed for a given popup.

So far we have been relying on this to avoid most problems. It isn't actually true though. The arrow could be moved to be horizontal and vertical at different times, and a theme could make the upwards pointing arrow a different size than the downward pointing one.

> - on "popupshown" we care about positioning the arrow up/down or left/right
> depending on the popup position

The popupshown event shouldn't be fired until after the popup is fully positioned, sized, and visible, and doesn't fire until any opening transitions are finished, so it is too late to make additional adjustments to the popup.

You could add another event that fires in-between but I suspect that, since the repositioning will need to happen again after waiting for the event to occur, it will either cause the popup to flicker as its size is adjusted, or significant work will need to be done to change the code flow in this case.

This bug and related issues have never been fully fixed because there isn't really an ideal solution, short of hard-coding arrow behaviour into nsMenuPopupFrame, or finding some way for SetPopupPosition to be implemented in script.

Comment 10

[Julian Descottes]

Thanks for the detailed information ! 

> The popupshown event shouldn't be fired until after the popup is fully
> positioned, sized, and visible, and doesn't fire until any opening
> transitions are finished, so it is too late to make additional adjustments
> to the popup.

Might be too late to avoid flickering but it still repositions the arrow properly in my test scenarios. 

> The arrow could be moved to be horizontal and vertical at different times, 
> and a theme could make the upwards pointing arrow a different size than the 
> downward pointing one.

If this is supported, I don't see any way to fix the issue completely. If I use another event fired in between as you suggested, we still have to deal with the popup changing size when updating the arrow ?

> This bug and related issues have never been fully fixed because there isn't
> really an ideal solution

I still think that having this additional event fire after SetPopupPosition is done in LayoutPopup to readjust the arrow position will fix issues with the regular cases. For complex themes or popups that change orientation, it might not fix anything. But it also shouldn't make things worse ?

I will (try to) write a simple patch to showcase this and will send it for feedback, up to you to decide if we should go forward :)

Comment 11

[Neil Deakin]

Sure, let's try it.

Comment 13

[arni2033]

Well, yes, this is duplicate. Sorry for not helpful report. But, if anyone still working on this,
please pay attention that it's happening in Debugger and Netmonitor as well (see info in bug 1198251).

Comment 14

[arni2033]

Attachment: It also happens in Debugger when you manually expand list of properties in tooltip.png

And in some cases it happens in Debugger by itself. (sorry for bug spam)

Comment 15

[arni2033]

Attachment: This is also happening with any popups, not only in devtools.png

On this screen: At first, the popup with info about add-on appears. It's enough place for it, so it determines the direction - "from top to bottom".
Then the Sync block is being added. Tooltip somehow determines its new position and moves according to new coordinates (!!!). But doesn't update the direction.

Maybe a major fix posible, which will add new function - "update the direction if needed" to the existing function - "compute new coordinates" ?   I don't know real name for that function, but think it could be implemented like this. That would fix this for all popups.

Comment 16

[Neil Deakin]

I added a patch in bug 1206133 to add an extra event for arrow panels, that might help here. Since I can't reproduce the original issues, would someone care to test?

Comment 17

[arni2033]

(In reply to Neil Deakin from comment #16)
> can't reproduce the original issues
I can't compile on my machine, so I'll just wait for a ready build to check my (a little hacky) testcase with AMO tooltip. Well, I bet that if using str in bug 1198251 you get arrow at the top left corner of tooltip, - everything's fixed (just mentioned this in case you don't mind trying another STR)

Comment 18

[Julian Descottes]

(In reply to Neil Deakin from comment #16)
> I added a patch in bug 1206133 to add an extra event for arrow panels, that
> might help here. Since I can't reproduce the original issues, would someone
> care to test?

Thanks for implementing this ! I imported your patch from 1206133.

On Windows, I tested the inspector (markup & rule view) and the network tab : the arrow is correctly positioned now.

Do you need other platforms to be tested ?

Comment 19

[Neil Deakin]

No, thanks for testing.

Comment 21

[Julian Descottes [:jdescottes]]

All the image preview tooltips have been migrated to HTMLTooltip, and another bug is tracking the underlying XUL panel arrow issue.

Closing as duplicate of Bug 1276876