Closed Bug 418175 Opened 15 years ago Closed 15 years ago

popup attribute test fails if context menu is too big

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: ajschult784)

Details

Attachments

(3 files, 2 obsolete files)

One on of my boxes, the native theme seems to space the context menu items out a lot and then context menu in test_popup_attribute is too tall to fit in the bottom half of the window (see attached screenshot).  This causes failures on multiple parts of the test.

The simple solution is to make the window taller.  620px is sufficient for me, but perhaps it ought to be 700px or something to ensure it works in cases where other things (large system font perhaps) cause similar problems.  The other solution would be to have the window resize itself based on the size of the popup.
Attached image screenshot
this from a point where the popup should be below
Attached patch just make it taller (obsolete) — Splinter Review
Attachment #303965 - Flags: review?(enndeakin)
Attached patch fix test_popup_button too (obsolete) — Splinter Review
popup_button needs the same fix.  I also made the margins bigger to recenter the text/button.
Attachment #303965 - Attachment is obsolete: true
Attachment #304160 - Flags: review?(enndeakin)
Attachment #303965 - Flags: review?(enndeakin)
Comment on attachment 304160 [details] [diff] [review]
fix test_popup_button too

This is the patch for some other bug.
Attachment #304160 - Flags: review?(enndeakin) → review-
Attached patch the right patchSplinter Review
Attachment #304160 - Attachment is obsolete: true
Attachment #304245 - Flags: review?(enndeakin)
Comment on attachment 304245 [details] [diff] [review]
the right patch

Looks OK, but you will probably want to check if any of the test machines are running on a resolution 1024x768 or less. It's possible that the screen height minus the taskbar/menubar/etc + and window titlebar are less than 700 and would cause this to fail.
Attachment #304245 - Flags: review?(enndeakin) → review+
The Windows test boxes are a little weird, in that the desktop inherits the properties of the RDP client that last connected to them (see: my headaches in bug 405384 and bug 414720).

It would probably be best to add an explicit check to |screen.width| and/or |screen.height|, and fail if you're below the minimum size. At least then the reason for the test failure will be obvious, after when someone (inevitably) connects at 640x400.
OK, I landed the patch as-is and the tinderboxen didn't have a problem with it.  I'd be happy to add checks for resolution, although I'm not sure a) what min resolution to check for b) how to warn the user and/or get something into the logs about.
I'd say:

1. Just pick a resolution that's known to work to start with.
2. Treat a lower resolution as a test failure.

I'd expect the effect of #2 to be that you'd see an error about needing a larger resolution, and then you'd see errors about the popup tests failing (because the screen really is too small).

Bonus points for trying the tests with your desktop at a couple different resolutions, to confirm that 800x600 or whatever is really too small. The check doesn't need to be precise to the exact dimension that starts to cause a failure.
For me, the popup attribute tests pass at 1024x768 and fails at 800x600.
Attachment #305124 - Flags: review?(enndeakin)
Attachment #305124 - Flags: review?(enndeakin) → review+
OK, resolution check is in too
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.