1208564 - Inconsistent bookmark button highlight

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Brian Nicholson (:bnicholson)]

The bookmark button has a black highlight when tapped, while all of the other toolbar buttons turn blue.

Comment 1

[Stephan Leroux [:sleroux]]

Looks like we never had a 'highlighted' state for the bookmark button - only normal and selected. Do we want to also include a highlighted one? This would be the empty bookmark image just with a blue tint.

Comment 2

[Robin Andersen [:tecgirl]]

(In reply to Stephan Leroux [:sleroux] from comment #1)
> Looks like we never had a 'highlighted' state for the bookmark button - only
> normal and selected. Do we want to also include a highlighted one? This
> would be the empty bookmark image just with a blue tint.

Yes, it would be nice to have the highlighted state.

Comment 3

[alx91]

I would like to add this, can you assign this issue to me?

Comment 4

[Stephan Leroux [:sleroux]]

Sure thing! 

Robin, do you want to provide a highlighted image or have us tint the existing one? Either one is fine.

Comment 5

[alx91]

Yes, both works for me

Comment 6

[Robin Andersen [:tecgirl]]

(In reply to Stephan Leroux [:sleroux] from comment #4)
> Sure thing! 
> 
> Robin, do you want to provide a highlighted image or have us tint the
> existing one? Either one is fine.

Stephan, tint the existing. Less is more!

Comment 7

[alx91]

Attachment: Pull request

Comment 8

[Stephan Leroux [:sleroux]]

Could you update your branch from latest master? Looks a bit out of date.

Comment 9

[alx91]

I thought I got the latest code when I forked the Repo

Comment 10

[Stephan Leroux [:sleroux]]

Ah you know what - you opened the PR from Mozilla's master into your master. You're commit isn't in that PR!

Comment 11

[Stephan Leroux [:sleroux]]

Oops Your*

Comment 12

[alx91]

Ok second try

Comment 13

[alx91]

Attachment: Pull request

Comment 14

[Stephan Leroux [:sleroux]]

Comment on attachment 8682782 [details] [review]
Pull request

Looks good! When I mentioned tinting, the alternative approach would be to use the already existing bookmark image and apply a color tint to it instead of generating another image but this works just as well. The only thing I notice is that when unselecting a bookmarked page, the icon goes from filled blue to black unfilled. Is that something we want :tecgirl?

Comment 15

[alx91]

Since I wanted to fix a consistency bug I didn't want to tint the greyscale image with CG, because then I would have to do it with the other icons as well (of you don't have to). If you mean tinting the UIButton: I tried that but it only worked with a UIButton subclass overriding the didSet of the highlighted variable. So I just did this, because it was the easiest way. If you suggest another way (I probably oversaw something regarding tinting), please let me know.

I don't know what the highlighted image changed about the state transition from filled blue to black unfilled. Do suggest to have a highlighted state as well? The longPressGestureRecognizer seems to interfere somehow.

Comment 16

[Stephan Leroux [:sleroux]]

I meant tinting the greyscale image but I noticed we don't do that for any of the other toolbar buttons so you can ignore that suggestion :)  I don't think your patch regressed the selected bookmark -> black icon - I think it was just never was highlighting to blue. I'm not sure if it should. It kind of makes sense that it would go from filled blue -> black since we are deselecting it but I'll leave that up to :tecgirl.

Also, thanks for the contribution :D Are you interesting in looking at another bug in the meantime?

https://bugzilla.mozilla.org/show_bug.cgi?id=1220301

Comment 17

[alx91]

I am currently waiting for input on this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1164173
But I will take a look :)

Comment 18

[Stephan Leroux [:sleroux]]

Also figured I would mention it but if you're looking for discussion about Firefox for iOS/Android, we're in the #mobile IRC channel -> https://wiki.mozilla.org/IRC

Comment 19

[Robin Andersen [:tecgirl]]

Comment on attachment 8682782 [details] [review]
Pull request

I'm not able to review this PR, it has been closed.

Comment 20

[alx91]

It should be this PR
https://github.com/mozilla/firefox-ios/pull/1223

Comment 21

[Robin Andersen [:tecgirl]]

Comment on attachment 8682782 [details] [review]
Pull request

LGTM! Thank you!

Comment 22

[Aaron Train [:aaronmt]]

Did this land? Resolve?

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: Updated PR

Comment 24

[Brian Nicholson (:bnicholson)]

The PR has unrelated commits for another bug. Split this out and merged it manually.

https://github.com/mozilla/firefox-ios/commit/5b15103b1c832d60a69c08b30ac03d9456b39086