1258176 - Bookmark star is missing in mdpi devices

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Jan Manthay]

Attachment: Screenshot_2016-03-20-08-51-13.png

I installed Firefox on my new Amazon Fire tablet. (From the .apk files)

I just realised that the star to add bookmarks is missing. I oben the menu, and normaly on top are the three icons: star, book, sharing icon.

But here, the star is missing, the place left of the book is free. 

BUT:

The funcionality ist still given! So I can add bookmarks when I tap on the free space. It is realy only the icon thats missing.

OS is Fire OS 5.1.2, screen resolution is 1024x600

Happens on 45 release and 48 nightly.

Comment 1

[alex_mayorga]

¡Hola!

This is still a thing on Nightly as of today.

Also a fellow user asking in the SuMo forum over at https://support.mozilla.org/questions/1152691

¡Gracias!
Alex

Comment 2

[Tom Klein]

Attachment: Bug 1258176 - Remove bookmark star stub drawables in drawable/ that were formerly needed for API 9.

They're getting selected on mdpi (and presumably ldpi) devices.

Review commit: https://reviewboard.mozilla.org/r/102808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102808/

Comment 3

[Tom Klein]

I think this is more generally an issue on any mdpi (or ldpi) device - I see it on mdpi emulators, for example.  I don't have a Fire tablet to test on, so I'll hold off on updating the bug title until we can verify that what I'm seeing on emulators is indeed the same issue as on the Fires.

Sebastian: I'm not entirely certain what I've done here is the correct solution.  I think what happened was:
a) all mdpi drawables were removed in bug 1171946; and then
b) bug 1209967 added in stubs in drawable/ for ic_menu_bookmark_add and ic_menu_bookmark_remove (now known as star_blue), which, I suspect, are what android's resource selection algorithm then started choosing on mdpi (and ldpi) devices: the fact that those additions in drawable/ are references to null is why no star appears.

If the stubs were only added to satisfy some API 9 requirement, as the comment in those stubs suggests:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/ic_menu_bookmark_add.xml#7

then I think the patch is fine, since the two stubs being deleted here are still selectable from, for example, drawable-hdpi-v11.  If the stubs were added for some additional reason then we may need to rethink this.

Comment 4

[Tom Klein]

Meant to mention: the data at

https://developer.amazon.com/public/solutions/devices/kindle-fire/app-development/01--screen-layout-and-resolution

suggest the 1024x600 Fire devices are all mdpi, so I'm assuming Jan's device is mdpi.  Kevin: do you still have yours?  Is it mdpi?

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

I don't have a Kindle Fire.

Comment 6

[Tom Klein]

I can recreate this in both (non-kindle) tablet and phone mdpi emulators, so removing the "kindle-specific" block and updating the title.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824284 [details]
Bug 1258176 - Remove bookmark star stub drawables in drawable/ that were formerly needed for API 9.

https://reviewboard.mozilla.org/r/102808/#review104208

Ouch. Let's uplift this!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a70d0266ffe
Remove bookmark star stub drawables in drawable/ that were formerly needed for API 9. r=sebastian

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a70d0266ffe

Comment 10

[Tom Klein]

Testing with more emulators, I find that the bookmark star does appear (even without the patch here) on the mdpi xlarge 10.1" WXGA tablet, so I guess the issue arrises on mdpi or ldpi devices smaller than xlarge.  Our smallest density folders for the bookmark stars are drawable-hdpi-v11 and then drawable-xlarge-hdpi-v11, so apparently the selection algorithm selects drawable-xlarge-hdpi-v11 once we reach xlarge size (even for mdpi devices), but chooses drawable/ over drawable-hdpi-v11/ for sizes smaller than xlarge.

Comment 11

[Tom Klein]

Comment on attachment 8824284 [details]
Bug 1258176 - Remove bookmark star stub drawables in drawable/ that were formerly needed for API 9.

Approval Request Comment
[Feature/Bug causing the regression]: Probably bug 1209967, though I haven't confirmed.
[User impact if declined]: The bookmark star icon, our only means of representing bookmarkability, won't appear on ldpi or mdpi devices smaller than xlarge.  Pressing in the space where the star is supposed to appear still does what it's supposed to, but the icon that's supposed to be there doesn't appear.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: I've manually verified the fix on Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: The screenshot shows the missing bookmark star (the empty space to the left of the readermode icon in the menu (though note that I don't think the reader mode icon appears in that menu anymore)).  A bookmark star should appear in that menu for both bookmarked (filled blue star) and unbookmarked (empty black outline star) pages.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The fix was to remove stub (never intended to appear in UI) transparent versions of the icons from the drawable/ folder which were formerly needed for API 9 (which we no longer support - min version is now 15); now that those versions are gone, Android will instead select actual bookmark star icons which already exist in the drawable-hdpi-v11/ folder.
[String changes made/needed]: None.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8824284 [details]
Bug 1258176 - Remove bookmark star stub drawables in drawable/ that were formerly needed for API 9.

Fix an UI issue related to bookmark star. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0337b9667770

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45df2ac39896

Comment 15

[alex_mayorga]

¡Hola!

The bookmark star now shows in Nightly on this Kindle.

Thanks to everyone involved.

¡Gracias!
Alex