983653 - UITour: Make the highlight effect a circle on the bookmarks combo button

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

The previous patch was meant to make the bookmarks combo button have a circular highlight (and I _swear_ it did when I tested it), but apparently on nightly it doesn't anymore, so this patch is here to give me a chance to fix that.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug983653.patch

The bookmarks button now has a ratio of 2.6363636363636362 (instead of the 2.01) it had when I wrote the patch), so I think "3" will give us enough room to handle any future padding changes.

Comment 2

[Stephen Horlander [:shorlander]]

Comment on attachment 8391202 [details] [diff] [review]
bug983653.patch

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

Looks good to me!

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8391202 [details] [diff] [review]
bug983653.patch

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

Holly is also fine with trying this.

::: browser/modules/UITour.jsm
@@ +810,5 @@
>        let highlightWidth = targetRect.width;
>        let minDimension = Math.min(highlightHeight, highlightWidth);
>        let maxDimension = Math.max(highlightHeight, highlightWidth);
>  
>        // If the dimensions are within 110% of each other (to include the bookmarks button),

You forgot to update the comment again :)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug983653.patch

Fixed the comment.  :)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): (I can't find it, but something changed the padding or margin on the bookmarks button.)
User impact if declined: Less noticeable bookmarks highlight in the UI tour.
Testing completed (on m-c, etc.): Manual.  Also the video in bug 983737.
Risk to taking this patch (and alternatives if risky): Very low.
String or IDL/UUID changes made by this patch: None.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

I will approve it once it is in m-c.

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:
Firefox Aurora 29.0a2
Build Id:20140317004002
OS: Win 8.1 x 32, Mac Os x 10.8.5, Ubuntu 13.04 x64