This is a follow-up from bug 909349. See https://bugzilla.mozilla.org/show_bug.cgi?id=909349#c31

Comment on attachment 8387186 [details] [diff] [review] Patch v1: adjust OSX toolbar buttons height to match urlbar and search bar Cancelling review based on a couple of glitches I saw when testing. Discussed with mikedeboer in meatspace.

This fixes stuff with the Bookmarks menu button. Was there another question you had that I can't remember?

Comment on attachment 8387333 [details] [diff] [review] Patch v1.1: adjust OSX toolbar buttons height to match urlbar and search bar Cancelling review because there's still that one issue with the Bookmarks Toolbar Items in the nav-bar causing the back button to be clipped.

Comment on attachment 8387764 [details] [diff] [review] Patch v1.3: adjust OSX toolbar buttons height to match urlbar and search bar Review of attachment 8387764 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/browser.css @@ +116,5 @@ > #PersonalToolbar { > padding: 0 4px 4px; > } > > +#main-window[customize-entered] #PersonalToolbar { As per discussion, we're going to yank this particular rule because it makes the customize mode transition look worse with the bookmarks toolbar enabled.

Comment on attachment 8387799 [details] [diff] [review] Patch v1.4: adjust OSX toolbar buttons height to match urlbar and search bar Review of attachment 8387799 [details] [diff] [review]: ----------------------------------------------------------------- Let's roll! We should also file follow-up bugs for the bookmarks toolbar items stuff we noticed.

Pushed as: https://hg.mozilla.org/integration/fx-team/rev/74568436ab02 Follow-up bugs coming up!

(In reply to Mike de Boer [:mikedeboer] from comment #11) > Pushed as: https://hg.mozilla.org/integration/fx-team/rev/74568436ab02 > > Follow-up bugs coming up! Backed out test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=35892399&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=35892543&tree=Fx-Team https://hg.mozilla.org/integration/fx-team/rev/403ef36f7d35

:-(

I'm guessing this is breaking because: http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js#34 no longer clicks the right thing. I'm not sure if that's a bug in this patch and how it affects the nested buttons, or if it's an issue with the test. In the latter case, it might be possible to switch to synthesizeKey with VK_ESCAPE, but I've not tried that.

Yup, but my try push with another 'fix' yielded other test failures, in UITour code this time. I'm currently investigating why that would be the case.

Personal bookmarks toolbar placeholder follow-up bug filed as bug 982215.

Try push I was referring to earlier: https://tbpl.mozilla.org/?tree=Try&rev=730c70bb7396

Matt, do you know why `browser/modules/test/browser_UITour_panel_close_annotation.js` would fail when the toolbar buttons on OSX are made smaller by this bug/ patch? The test fails with the message "Timeout waiting for popup at anchor: Highlight should move to the appMenu button and still be visible", see the TBPL link I posted in comment 17.

Pushed another try: https://tbpl.mozilla.org/?tree=Try&rev=7c947a7ca776 This disables the UITour test on Mac and uses Gijs' suggested test update to hide the panel with VK_ESCAPE.

Try run is green, so posting the patch for review. - For browser_toolbarbutton_menu_context.js I made the panel hide with VK_ESCAPE. - I disabled browser_UITour_panel_close_annotation.js on OSX, because a) it runs without failure locally (cannot reproduce the failure) and b) to fix the test I need input from MattN and having this test work for OSX on our infra is lower prio than resolving this P3, IMHO.

(In reply to Mike de Boer [:mikedeboer] from comment #20) > - I disabled browser_UITour_panel_close_annotation.js on OSX, because a) it > runs without failure locally (cannot reproduce the failure) and b) to fix > the test I need input from MattN and having this test work for OSX on our > infra is lower prio than resolving this P3, IMHO. In other words: I'm suggesting to move fixing this test for OSX in a follow-up bug.

Comment on attachment 8389793 [details] [diff] [review] Patch 2: update tests Review of attachment 8389793 [details] [diff] [review]: ----------------------------------------------------------------- r=me after reverting browser.ini and updating the commit message ::: browser/modules/test/browser.ini @@ +14,5 @@ > [browser_UITour_availableTargets.js] > [browser_UITour_detach_tab.js] > [browser_UITour_annotation_size_attributes.js] > [browser_UITour_panel_close_annotation.js] > +skip-if = os == "mac" # Popup handling failures due to bug 980374 This isn't necessary as the test failed because you did a try push on fx-team instead of m-c and pushed with a test failure from bug 971116 in it. You can remove this line and it'll be fine.

Alright! Thanks Matt, good thing I asked your for info ;) Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/3f47ab139eb4 https://hg.mozilla.org/integration/fx-team/rev/ab6e51e115bc

https://hg.mozilla.org/mozilla-central/rev/3f47ab139eb4 https://hg.mozilla.org/mozilla-central/rev/ab6e51e115bc

Comment on attachment 8387799 [details] [diff] [review] Patch v1.4: adjust OSX toolbar buttons height to match urlbar and search bar [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 909349 User impact if declined: The toolbar buttons on OSX will appear to be too large when hover/ clicked. Testing completed (on m-c, etc.): landed on m-c Risk to taking this patch (and alternatives if risky): minor String or IDL/UUID changes made by this patch: n/a

Comment on attachment 8389793 [details] [diff] [review] Patch 2: update tests [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 909349 User impact if declined: Mochitest b-c test will fail if the patch in this bug is landed without updating the erroneous unit test. Testing completed (on m-c, etc.): landed on m-c Risk to taking this patch (and alternatives if risky): minor String or IDL/UUID changes made by this patch: n/a

Comment on attachment 8389793 [details] [diff] [review] Patch 2: update tests Mike, you didn't explicitly required the uplift flag. I guess it is a mistake.

Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/d0d8439772f7

And the test update: https://hg.mozilla.org/releases/mozilla-aurora/rev/2e9b078177e4