961727 - Adjust popup padding on Windows to 4px

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Mike de Boer [:mikedeboer]]

This is a follow-up of bug 859751, where I removed this change due to mochitest-chrome test failures.

We still want the reduced padding, so we'll deal with that separately here.

Comment 1

[Mike de Boer [:mikedeboer]]

Neil, I tried to fix the tests here, as I think a reduction of the padding on Windows should be possible without test failures, but I got stuck at trying to understand what's going on.

The objective is to change the padding defined at http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/popup.css#78 to 4px, instead of 10px.

The arrowpanel test fails at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_arrowpanel.xul#80, for example with the messages
'anchored on right - got 498, expected 490' and 'anchored on left - got 2, expected 10'.

As a test, when I reduce all the panel offsets with 6px as well to 14px - starting from http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/popup.css#50 - the tests do pass, but the panel positioning is off. 

I'd like to understand where the 8px difference is coming from and how I can incorporate that in test logic. My hope is that this is a 'duh' thing for you, but I just couldn't see the light after staring at the issue for a while... Can you tell me?

Comment 2

[u428464]

The black boders are a bit ugly too. I don't see them on the Win 8 mockup.

Comment 3

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> The black boders are a bit ugly too. I don't see them on the Win 8 mockup.

Black borders ?

Comment 4

[u428464]

(In reply to Tim Nguyen [:ntim] from comment #3)
> (In reply to Guillaume C. [:ge3k0s] from comment #2)
> > The black boders are a bit ugly too. I don't see them on the Win 8 mockup.
> 
> Black borders ?

I saw them on a Win 7 classic screenshot, but in fact it was normal and it's perfectly fine on Win 7 aero.

Comment 5

[Neil Deakin]

> I'd like to understand where the 8px difference is coming from and how I can
> incorporate that in test logic.

The test is asking the panel to be aligned along the bottom edge and along the left side of the anchor 'bottomright' which appears in the lower-right corner of the test page. The test is expecting the popup to be large enough that it won't fit either horizontally or vertically, so that the panel appears flipped in both directions, instead aligning along the top edge and along the right side of the anchor.

However, as you decreased the padding on the popup, the popup is now not as wide, so it no longer flips horizontally.

I'd suggest just making the content of the popup in the test a bit bigger such that it is 12 pixels wider. I just did a test on Windows by adding an extra period to the label and the test worked again. Unfortunately, this test is quite finicky though, so that might break on another platform, but if so, you could set the label differently on Windows.

Comment 6

[Mike de Boer [:mikedeboer]]

AHA! Neil, thanks so much for that info, it makes complete sense to me now!

Comment 7

[Mike de Boer [:mikedeboer]]

Neil, would giving the label and button inside the <panel> a fixed width not make the test less finicky? Or would it defeat the purpose of this test altogether?

Comment 8

[Mike de Boer [:mikedeboer]]

Oh, btw, after testing with a longer label text, some assertions indeed fail on OSX with off-by-1px errors. So I _guess_ there's no other way than to only add more characters on Windows.

Comment 9

[Neil Deakin]

You could set a specific width as well. The issue here is that arrow panels are styled a bit differently on each platform and the test relies on too precise a placement. Ideally, the test needs to be reworked to be less dependent on this.

Does the patch in bug 941051 help?

Comment 10

[Mike de Boer [:mikedeboer]]

Alright, I'll try! In the meantime, can you land the patch in bug 941051 on fx-team?

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: adjust popup padding to 4px on Windows

Looks like Neil's updates to the chrome test solved the issue for me, double-checking with a Try push: https://tbpl.mozilla.org/?tree=Try&rev=45389bff184b

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8369452 [details] [diff] [review]
Patch v1: adjust popup padding to 4px on Windows

bah, now it's test_popupanchor.xul giving a party :/

Comment 13

[Mike de Boer [:mikedeboer]]

Another try run, now only changed the padding, not the arrow positions.

https://tbpl.mozilla.org/?tree=Try&rev=d3e14889a1c4

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: adjust popup padding to 4px on Windows

Comment 15

[Mike de Boer [:mikedeboer]]

Thanks!

remote: https://hg.mozilla.org/integration/fx-team/rev/eae472070610

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eae472070610

Comment 17

[u428464]

I may have wrongly understood the purpose of this bug but I don't see the reduced padding in latest Fx-Team build on Win7.

Comment 18

[Stephen Horlander [:shorlander]]

I think this broke the Sync promo: http://cl.ly/image/2A0g0Y2O410j

Comment 19

[u428464]

Ok now I see the change, but I don't understand why the padding of the menu panel hasn't been reduced too.

Comment 20

[Mike de Boer [:mikedeboer]]

Stephen, I think you're right. The fix should be simple, but will be dealt with in another bug.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8370794 [details] [diff] [review]
Patch v1.1: adjust popup padding to 4px on Windows

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: invalid amount of spacing around the panel edges on Windows.
Testing completed (on m-c, etc.): landed on m-c, passed all tests.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df84666ed37f

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #20)
> Stephen, I think you're right. The fix should be simple, but will be dealt
> with in another bug.

Did you file another bug for this? What is it?

Comment 24

[Mike de Boer [:mikedeboer]]

Right, I forgot to file it. Will do that right away.