392160 - Wrong menupopup positioning inside HTML table

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Wladimir Palant]

Attachment: Testcase

It seems that positioning of the menulist popup is wrong when this menulist element is placed inside an HTML table (or element with display:table-cell) next to a multiline text block. To test, save the testcase and open it as a dialog using this command:

  window.openDialog("file:///.../popup_positioning.xul", "_blank", "width=500,height=500,resizable");

Click the dropdown arrow of the menulist - the position of the popup will only be correct if you resize the dialog to make sure the text doesn't wrap.

Tested in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081320 Minefield/3.0a8pre. Firefox 2.0 generally doesn't wrap the text here so that the testcase doesn't work.

Comment 1

[Wladimir Palant]

The regression range is 2007070404 to 2007070504 (http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-04-02&maxdate=2007-07-05-06&cvsroot=%2Fcvsroot). Prime suspect is bug 279703, bug 385423 might be related as well. Cc'ing roc and Neil Deaking to comment.

Testing shows that the parent window of the dialog makes a difference for some reason. If I open the dialog from about:config the popup will only be slightly off, when it is opened from the Error Console however it will be entirely outside the dialog window.

Comment 2

[Neil Deakin]

The offset returned by
  containingView = aAnchorFrame->GetClosestView(&offset);
in nsMenuPopupFrame::SetPopupPosition in being returned incorrectly as the offset as if the <description> text was all of one line (the preferred width of the text), so it isn't accounting for the actual size after reflow.

This is only a problem when inside a table cell though.

Not sure why this worked before however.

Comment 3

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

Perhaps there's a bug where view bounds aren't being updated properly.

The right thing to do, however, would be to not use views here, just use frame->GetOffsetTo.

Comment 4

[Neil Deakin]

Attachment: improve nsMenuPopupFrame::SetPopupPosition

This patch improves the popup positioning to not use widgets and views and just use the screen position and offsets. It fixes positioning errors when the anchor is in a different document, a test for this is included.

It also fixes the testcase as given, although doesn't fix the actual bug in the case where the testcase is loaded inside a child iframe. It appears that the menulist next to a block inside a box inside a table-cell gets positioned incorrectly when inside a child frame. That is, the frame rect of the menu button is set as if the block text was all on one line. This causes the popup to appear offset horizontally.

Asking for two sets of eyes here, although maybe Eli is a better reviewer here?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, I'd ask Eli instead.

Comment 6

[Eli Friedman]

Comment on attachment 277436 [details] [diff] [review]
improve nsMenuPopupFrame::SetPopupPosition

+  nsIFrame* rootFrame = presContext->PresShell()->GetRootFrame();
Nit: presContext->PresShell()->FrameManager()->GetRootFrame() is faster because it's non-virtual.

+    nsIPresShell *presShell = presContext->PresShell();

Nit: use the rootframe that you already cached earlier.

Does this work correctly with page zoom?  It might need to use UnscaledAppUnitsPerDevPixel().

r=me assuming this doesn't break popups in zoomed pages.

Comment 7

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

GetScreenRect can be expensive (on X) so can you make sure we minimize the calls to it. Seems like aAnchorFrame->GetScreenRect() can be called twice in SetPopupPosition.

I think this should be OK with page zoom (although it should be tested). DevPixelsToAppUnits and vice versa should always be converting to/from real device pixels.

Comment 8

[Neil Deakin]

Attachment: address issues

This patch also fixes an issue where xpos/ypos wasn't being added when calculating screenViewXLoc/screenViewYLoc.

Menus in zoomed documents work ok. There are other issues with zoomed content though, but they work or don't work the same with or without this patch.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 277919 [details] [diff] [review]
address issues

a1.9=dbaron

Comment 10

[Dave Camp (:dcamp)]

This test seems to be failing intermittently on qm-centos5-01:

*** 632 INFO Running chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_popup_anchor.xul...
*** 633 ERROR FAIL | popup left | got -83, expected -88
*** 634 ERROR FAIL | popup top | got 10, expected -14

Comment 11

[Neil Deakin]

Not sure why the test is failing. The only thing I can think of is that maybe the linux machine has some OS chrome like a menubar which appears over the window sometimes.

Comment 12

[Mats Palmgren (inactive)]

(filed bug 394421 on the test failure on qm-centos5-01)

Comment 13

[Wladimir Palant]

Neil, your patch didn't fix the issue I reported :-(

If I do the following from the Error Console in Minefield:

  window.openDialog("https://bugzilla.mozilla.org/attachment.cgi?id=276610", "_blank", "width=500,height=500,resizable");

I still see the dropdown list being displayed way off the position where it should be. Tested in Gecko/2007092405 Minefield/3.0a9pre as well as in 1.9a9pre-2007092510 XULRunner build.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The patch looks like it was checked in -- is this bug still in progress.

Comment 15

[Wladimir Palant]

Vlad, I reopened the bug because the patch didn't fix the problem I reported. I hope that Neil is going to give this a second look since we would like to have the fix in TomTom HOME 2.1 - and I don't know the code well enough to fix it myself.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 277919 [details] [diff] [review]
address issues

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 17

[Reed Loden [:reed]]

Comment on attachment 277919 [details] [diff] [review]
address issues

Already landed on 2007-08-27 09:23.

Comment 18

[Damon Sicore (:damons)]

Comment on attachment 277919 [details] [diff] [review]
address issues

Reset approval flag to + as it was already checked in.

Comment 19

[Neil Deakin]

The issue here is essentially the same as bug 228673.

When calculating the width of the <description>, the preferred width is as if the text was all on one line. However, when reflowing, the width is calculated with the text broken onto multiple lines.

Unfortunately, some code ends up using the former and some the latter calculation. After debugging this, the actual rectangle of the <menulist> keeps shuffling about due to this. That is, when painting it is using the with-line-breaks-width, but when opening the popup, a layout occurs which ends up setting the rectangle to the single-line-width.
 

Comment 20

[Wladimir Palant]

Still, bug 228673 is old. This bug only occurs since bug 279703 landing however.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

This landed, but the reported bug still occurs. We want to get it off the list for "isn't closed but has approved patch for FF3", so I'm closing this and I've created the followup bug 433037. This issue should be fixed in that bug.