980374 - Adjust height of buttons in the nav-bar to match the height of urlbar and search box

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Mike de Boer [:mikedeboer]]

This is a follow-up from bug 909349.

See https://bugzilla.mozilla.org/show_bug.cgi?id=909349#c31

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: adjust OSX toolbar buttons height to match urlbar and search bar

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: adjust OSX toolbar buttons height to match urlbar and search bar

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

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 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: adjust OSX toolbar buttons height to match urlbar and search bar

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3: adjust OSX toolbar buttons height to match urlbar and search bar

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4: adjust OSX toolbar buttons height to match urlbar and search bar

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Mike de Boer [:mikedeboer]]

Pushed as: https://hg.mozilla.org/integration/fx-team/rev/74568436ab02

Follow-up bugs coming up!

Comment 12

[Ed Morley [:emorley]]

(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

Comment 13

[:Gijs (he/him)]

:-(

Comment 14

[:Gijs (he/him)]

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.

Comment 15

[Mike de Boer [:mikedeboer]]

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.

Comment 16

[Mike de Boer [:mikedeboer]]

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

Comment 17

[Mike de Boer [:mikedeboer]]

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

Comment 18

[Mike de Boer [:mikedeboer]]

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.

Comment 19

[Mike de Boer [:mikedeboer]]

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.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: update tests

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.

Comment 21

[Mike de Boer [:mikedeboer]]

(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 22

[Matthew N. [:MattN]]

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.

Comment 23

[Mike de Boer [:mikedeboer]]

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

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Mike de Boer [:mikedeboer]]

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 26

[Mike de Boer [:mikedeboer]]

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 27

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 28

[Mike de Boer [:mikedeboer]]

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

Comment 29

[Mike de Boer [:mikedeboer]]

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