1085628 - browser_880164_customization_context_menus.js fails in OSX when the toolbar size is smaller than default

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alexandre Poirot [:ochameau]]

The following test fails to execute against Firefox dev edition, with a custom theme:
  browser/components/customizableui/test/browser_880164_customization_context_menus.js

The fake right click dispatched over here:
  http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_880164_customization_context_menus.js#119
is being done at a wrong position.

I don't know exactly why, the (100, urlbarRect.height - 1) doesn't work on FF dev ed.
The size of the toolbar is really smaller and there might be some padding/margin differences around the UI element being tested.

Clicking on (100, 1) works fine...
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f943af7cb111

Comment 1

[Brian Grinstead [:bgrins]]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1053434#c101.  Gijs, is clicking on (100, 1) instead of (100, urlbarRect.height-1) a reasonable fix for this?

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: patch

Comment 3

[:Gijs (he/him)]

Comment on attachment 8508220 [details] [diff] [review]
patch

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

Assuming tests pass on fx-team/try without further changes, it's an OK fix. As I said elsewhere, the proper fix might be adding a separate item with removable="false" set. If we don't need to do that here/yet, we can postpone that to get stuff in working order for devedition.

Comment 4

[:Gijs (he/him)]

(can someone fix the bug and patch summary, though?)

Comment 5

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=81387bbd4b28.  Here is a push without the patch: https://tbpl.mozilla.org/?tree=Try&rev=b48499fdc04e

Comment 6

[Brian Grinstead [:bgrins]]

The try runs are going as expected (orange without the patch and green with it)

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch

New commit message, with a try run with the patch on top of m-c instead of gum:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bcde157f4a6

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8508562 [details] [diff] [review]
patch

Carrying over r+

Comment 9

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/1af0f51fc2e1

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1af0f51fc2e1

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8508562 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: it's a test improvement that will be required once we uplift some other patches that will make the failure permanent
[User impact if declined]: none, only test failures
[Describe test coverage new/current, TBPL]: existing test
[Risks and why]: none, it's a test-only change
[String/UUID change made/needed]: none

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8508562 [details] [diff] [review]
patch

Apparently test-only fixes don't need approval.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c118e9e23c3