Last Comment Bug 875937 - Defect - App bar pin tile icon refreshes after it is displayed instead of before
: Defect - App bar pin tile icon refreshes after it is displayed instead of before
feature=defect c=Awesome_screen u=met...
Product: Firefox for Metro
Classification: Client Software
Component: Browser (show other bugs)
: unspecified
: All Windows 8.1
: P3 normal (vote)
: Firefox 25
Assigned To: Kamil Jozwiak [:kjozwiak]
: juan becerra [:juanb]
Depends on:
Blocks: 831918 metrov1it11
  Show dependency treegraph
Reported: 2013-05-24 13:41 PDT by Brian R. Bondy [:bbondy]
Modified: 2014-07-24 11:06 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

fixes the pin issue (1.72 KB, patch)
2013-07-16 19:45 PDT, Kamil Jozwiak [:kjozwiak]
mbrubeck: review+
Details | Diff | Splinter Review
fixes the pin issue v2 (1.82 KB, patch)
2013-07-17 18:35 PDT, Kamil Jozwiak [:kjozwiak]
kjozwiak: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2013-05-24 13:41:50 PDT
1. Select a tile and pin it.
2. Select a different tile that is unpinned, notice that pin icon shows up.
3. Unselect that tile
4. Select the tile that was previously pinned

Actual results: You'll see the appbar come up with the pin icon, and then after it is up it changes to the unpin icon
Expected result: It should just show the unpin icon right away and not change after it has come up.
Comment 1 Brian R. Bondy [:bbondy] 2013-05-28 14:05:25 PDT
Comment 2 Kamil Jozwiak [:kjozwiak] 2013-07-16 19:45:26 PDT
Created attachment 776898 [details] [diff] [review]
fixes the pin issue

other than some automated tests, this is my first patch that seems to fix the problem described by Brian in comment 0
Comment 3 Matt Brubeck (:mbrubeck) 2013-07-17 09:43:12 PDT
Comment on attachment 776898 [details] [diff] [review]
fixes the pin issue

Review of attachment 776898 [details] [diff] [review]:

Looks great.  Thanks!

::: browser/metro/base/content/appbar.js
@@ +159,5 @@
>    },
>    showContextualActions: function(aVerbs) {
> +    // When the appbar is not visible, we want the icons to refresh right away
> +    var immediate = !Elements.contextappbar.isShowing;

nit: Please use "let" instead of "var".
Comment 4 Kamil Jozwiak [:kjozwiak] 2013-07-17 18:35:29 PDT
Created attachment 777522 [details] [diff] [review]
fixes the pin issue v2

fixed the small nit mentioned in comment 3
Comment 5 Brian R. Bondy [:bbondy] 2013-07-17 18:44:32 PDT
\o/ :D Nice job!

I pushed to try here:

This will run through the metro tests to make sure nothing got broken in the tests or the app. 

I'll check if that passes tomorrow and if so I'll push it over to mozilla-central.
Comment 6 Kamil Jozwiak [:kjozwiak] 2013-07-17 18:58:27 PDT
Thanks Brian! Appreciate it
Comment 7 Brian R. Bondy [:bbondy] 2013-07-18 07:05:52 PDT
All green, nice job!
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-07-18 17:59:40 PDT
Comment 9 Manuela Muntean [Away] 2013-07-29 07:55:52 PDT
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130728 Firefox/25.0

Verified as fixed on the latest Nightly build using the steps from the description.

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