Closed Bug 1227120 Opened 9 years ago Closed 8 years ago

Filled bookmark star does not easily convey bookmarked state

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: mfinkle, Assigned: liuche)

References

Details

Attachments

(7 files, 1 obsolete file)

mov_bookmarkanim1_device.mov
75.14 KB, video/quicktime
Details
tablet_icon_bluestar.zip
4.74 KB, application/zip
Details
icon_bluestar_tabletphone.zip
6.63 KB, application/zip
Details
blue-bookmark-star v0.1
8.89 KB, patch
Details | Diff | Splinter Review
Screenshot: Blue star in toolbar and history items
195.84 KB, image/png
Details
MozReview Request: Bug 1227120 - Filled bookmark star does not easily convey bookmarked state. r=mcomella
58 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1227120 - Use new star resource for home panel bookmarks. r=mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details 
We use a filled gray star to indicate a page is bookmarked. An unfilled gray star means a page is not bookmarked. Feedback indicates that a person has not bookmarked recently, and seen the unfilled to filled transition, it's not obvious that a filled gray star means 'bookmarked' since it blends in well with the menu theme.

Desktop uses a filled blue star to indicate bookmarked. Maybe we should too.
NI'ing Anthony and Darrin for thoughts
Flags: needinfo?(dhenein)
Flags: needinfo?(alam)
iOS uses a blue star for a similar reason. I'm open to seeing how this looks, its probably a change worth making!
Flags: needinfo?(dhenein)
oops, didn't see the mid air collision before I boarded haha

I've been thinking about this too and have a couple mocks for both the panels and the menu that uses a blue color. 

I think it works. But, if we're talking about improving this "connection" in the users mind (reading the bug title to myself), we should also look at interaction of tapping the bookmark star. 

Desktop's animations are nice in that they feel whimsical, intentional and also tell the user where the bookmark's are located. On mobile, the menu dismisses and we get a Snackbar. I think we should improve our "feedback" here as well. Perhaps a 250ms delay to see an animation and the fill before dismissing the menu.
Flags: needinfo?(alam)
Our "Bookmark" option in the share overlay has a little animation when pressed. Can we get a build up to see what that would look like if we did the same thing in our menu before we closed it?

Breakdown: 
1) change to a blue filled state
2) animate star from unfilled to blue filled state
3) close menu after animation ends
(In reply to Anthony Lam (:antlam) from comment #4)

> Breakdown: 
> 1) change to a blue filled state
> 2) animate star from unfilled to blue filled state
> 3) close menu after animation ends

Keep in mind that the main reason we got feedback for this bug was a problem with coming back to a website that was already bookmarked. The filled gray star does not convey "I bookmarked this site last month." The blue color would, since it's noticeably different from the gray menuitem colors.

The animation when bookmarking is nice, but less of an immediate issue. We could even file a separate bug, since it might be more work to get the animation landed.
I think a blue star would be a good improvement, especially since it's consistent with the desktop UI. It's also consistent with the little blue star that we show in the home panel list items.
For context (maybe not this bug) - This is what I'm thinking for adding a bit of delight and whimsy to our bookmark star. It uses the rotation element of our iOS counter parts but keeps its position in the menu.

Couple points to avoid possible freak out:

 - Animation does take 500ms, so we should test out how delaying the dismissal of the menu will feel
 - For comparison, closing the menu immediately could still work, we can reuse the confirmation screen style from our Share overlay work in bug 1147653 (imagine a star instead of a check)
 - Improving the experience of bookmarking overall and could go a long way
For this bug, I was just about to attach assets but I think we can just tint our current star right?

The color is our Link Blue (#0096DD)
We do not currently tint the icon. We have two different icons:
ic_menu_bookmark_add.png
ic_menu_bookmark_remove.png

We switch them in code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3066
aaaaaand here they are for Tablets
(In reply to Mark Finkle (:mfinkle) from comment #12)
> (In reply to Anthony Lam (:antlam) from comment #10)
> > Created attachment 8700758 [details]
> > icon_bluestar.zip
> > 
> > Assets!
> 
> Here are the sizes we need:
> 36px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-hdpi-v11/ic_menu_bookmark_remove.png
> 48px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xhdpi-v11/ic_menu_bookmark_remove.png
> 72px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xxhdpi-v11/ic_menu_bookmark_remove.png
> 
> 20px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xlarge-mdpi-v11/ic_menu_bookmark_remove.png
> 30px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xlarge-hdpi-v11/ic_menu_bookmark_remove.png
> 40px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xlarge-xhdpi-v11/ic_menu_bookmark_remove.png
> 60px
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xlarge-xxhdpi-v11/ic_menu_bookmark_remove.png
> 
> Based on the images in the ZIPs, we still need 36px, 48px, 72px and 20px

Weird, not sure what happened in comment 11. But here they are, in 36, 48, and 72.

As for the 20px one, I thought we don't ship MDPI assets anymore? That would mean we don't require the 20px one for Tablets. But, I've included it anyways.
Attachment #8700758 - Attachment is obsolete: true
Flags: needinfo?(mark.finkle)
This patch replaces all of the ic_menu_bookmark_remove.png files in the tree. For some reason, I still see the gray star in local builds.
Assignee: nobody → mark.finkle
Flags: needinfo?(mark.finkle)
Mike says this is being tinted in the themes, so off to him.
Assignee: mark.finkle → michael.l.comella
(In reply to Mark Finkle (:mfinkle) from comment #15)
> This patch replaces all of the ic_menu_bookmark_remove.png files in the
> tree. For some reason, I still see the gray star in local builds.

09:23 <mcomella> mfinkle: Which files did you replace and on which device?
09:23 <mcomella> Nothing jumps out immediately, but we may be tinting the menu items grey...
09:23 <mcomella> Actually, yeah, that's probably what it is – I think we use the tint for "enabled/disabled" state
09:25 <mcomella> mfinkle: I think it's https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/styles.xml#99 though it could be the "MenuItemActionBar" style just above it
09:26 <mcomella> mfinkle: I think tinting transparent just makes the menu item transparent, which is why we used an explicit color

I think there are two approaches here:
 1) Fix transparent tinting in a state list so that setting a transparent tint will use the original color of the image
  - Setting color as null, rather than transparent, may just work
  - We could rewrite the state changing code ourselves such that if we see a null or transparent color, we remove the tint from the image
 2) Set the appropriate tint on the bookmarks star specifically
  - This is hard because the menu code is extremely generic but we may be able to add an, "if (id == R.id.menu...)" and override the tint. Alternatively, since we have our own menu inflater, we may be able to add a custom attribute to the menu files denoting the desired tint and override the later passed in value.

We do the tinting in the ThemedView template [1], and call out to DrawableUtil.tintDrawableWithStateList [2].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedView.java.frag#184
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/DrawableUtil.java#39
Assignee: michael.l.comella → liuche

    
    

    
  
I pngquanted all these images, as well as removed the mdpi versions of this that (I think) we don't need anymore.

I tried using a single image in drawable-nodpi, but the menu code is very general (and complicated) and I couldn't figure out a clean way to specify the dimensions of this resource. (InsetDrawable didn't resize anything, and I didn't try very **** ScaleDrawable after the image just didn't show up.)

        Attachment #8726023 -
        Flags: review?(michael.l.comella) → review+

    
    

    
  
Comment on attachment 8726023 [details]
MozReview Request: Bug 1227120 - Filled bookmark star does not easily convey bookmarked state. r=mcomella

https://reviewboard.mozilla.org/r/37759/#review34843

Nice hack.

I would prefer if you didn't remove the unrelated (bookmark_add) mdpi assets in this patch because it can have side effects like https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-mdpi-v11/ic_menu_reader_add.xml

::: mobile/android/base/java/org/mozilla/gecko/widget/themed/ThemedImageButton.java:183
(Diff revision 1)
> +            // The bookmarked state uses a blue star, so it shouldn't be tinted.

I'd make it clearer that this is intended to be a hack.

nit: newline above this line to separate comments, e.g.

// comment 1
//
// comment 2

        Attachment #8726024 -
        Flags: review?(michael.l.comella) → review+

    
    

    
  
Comment on attachment 8726024 [details]
MozReview Request: Bug 1227120 - Use new star resource for home panel bookmarks. r=mcomella

https://reviewboard.mozilla.org/r/37761/#review34845

(/me wonders why that asset was in nodpi...)

    
    

    
  
Comment on attachment 8726023 [details]
MozReview Request: Bug 1227120 - Filled bookmark star does not easily convey bookmarked state. r=mcomella

https://reviewboard.mozilla.org/r/37759/#review34857

We spoke in person.

By not including an xlarge-mdpi asset, we run the risk of breaking xlarge-mdpi builds (where those devices match `drawable-hdpi` or similar before they match `drawable-xlarge-hdpi-v11` or whatever). Chenxia's verifying it now.

    
    

    
  
Adding an mdpi resource for API 9 devices that can't use drawable-hdpi-v11 resources as a fallback.

    
    

    
  
Actually I realized that I forgot to rename one of the API-9 resources. I'll be backing these patches out if for some reason this doesn't stick.
Product: Firefox for Android → Firefox for Android Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: