Closed Bug 1341275 Opened 7 years ago Closed 7 years ago

Activity stream - Website icon from Context menu is inconsistent

Categories

(Firefox for Android Graveyard :: General, defect, P1)

54 Branch
ARM
Android
defect

Tracking

(firefox54 wontfix, firefox55 wontfix, firefox56 fixed, firefox57 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: cfat, Assigned: mcomella)

References

Details

(Whiteboard: [MobileAS] 1.26)

Attachments

(7 files)

Environment: 
Devices:  
- HTC 10 (Android 6.0.1);
- Nexus 5 (Android 6.0.1)
Build: Nightly 54.0a1 (2017-02-21);

Prerequisites: Activity stream on Android is enabled.

Steps to reproduce:
1. Open Fennec;
2. Go to Activity stream Panel;
3. Tap on the 3 dots context menu icon of a website several times (either from Top sites or Highlights areas);
4. Repeat step 3 for various websites.

Expected result:
Context menu is triggered, icons are displayed properly, there are no UI issues.

Actual result:
The icon from the context menu is inconsistent: sometimes it appears as big as in the tiles, sometimes only the favicon is displayed.

Notes:
Please see the Screencast attached.
This is not reproducible on Oneplus Two (Android 6.0.1) and LG G4 (Android 5.1).
Whiteboard: [MobileAS]
Priority: -- → P2
Assignee: nobody → michael.l.comella
I am able to reproduce on my N5.
When opening the context menus, I noticed that `FaviconView.formatImage` is called twice:
- FaviconView.onSizeChanged -> formatImage
- onIconResponse in BottomSheetContextMenu -> ... -> formatImage

When onSizeChanged is called first, everything looks great. When onIconResponse is called first, we see the unexpected behavior.

Now to figure out why it's called twice and whether or not it should be called twice...
When onIconResponse is called first, it calls showNoImage because the size of the ImageView isn't set yet and it's early returning. `showNoImage` sets `mDominantColor = 0`, overriding the value we wanted to assign. When onSizeChanged is called second, mDominantColor is still 0 so we don't fill with the proper color.
I have the patches completed for review but I'm blocked on pushing to reviewboard because my SSH access expired (bug 1381164). This patch isn't urgent so I'll wait – NI self to remember to follow-up on this bug.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Comment on attachment 8887105 [details]
Bug 1341275: Remove unused constants in FaviconView.

https://reviewboard.mozilla.org/r/157840/#review162950
Attachment #8887105 - Flags: review?(s.kaspari) → review+
Comment on attachment 8887106 [details]
Bug 1341275: Rm redundant mActualHeight/Width variables.

https://reviewboard.mozilla.org/r/157842/#review162952

::: mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java:137
(Diff revision 1)
>       * normal sized ones, replaces null bitmaps with the default Favicon, and fills all remaining space
>       * in this view with the coloured background.
>       */
>      private void formatImage() {
>          // We're waiting for both onSizeChanged and updateImage to be called before scaling.
> -        if (mIconBitmap == null || mActualWidth == 0 || mActualHeight == 0) {
> +        if (mIconBitmap == null || getWidth() == 0 || getHeight() == 0) {

I assume you checked (or know) that getWidth() and getHeight() return the right values this point already? I don't remember the complete flow but it takes some time until those methods return the measured size (There's also getMeasuredWidth()/getMeasuredHeight()).
Attachment #8887106 - Flags: review?(s.kaspari) → review+
Comment on attachment 8887107 [details]
Bug 1341275: Rm redundant check for size change in onSizeChanged.

https://reviewboard.mozilla.org/r/157844/#review162954
Attachment #8887107 - Flags: review?(s.kaspari) → review+
Comment on attachment 8887108 [details]
Bug 1341275: Replace color 0 with Color.TRANSPARENT.

https://reviewboard.mozilla.org/r/157846/#review162956
Attachment #8887108 - Flags: review?(s.kaspari) → review+
Comment on attachment 8887109 [details]
Bug 1341275: Add mShouldShowImage flag to FaviconView.

https://reviewboard.mozilla.org/r/157848/#review162962
Attachment #8887109 - Flags: review?(s.kaspari) → review+
Comment on attachment 8887106 [details]
Bug 1341275: Rm redundant mActualHeight/Width variables.

https://reviewboard.mozilla.org/r/157842/#review162952

> I assume you checked (or know) that getWidth() and getHeight() return the right values this point already? I don't remember the complete flow but it takes some time until those methods return the measured size (There's also getMeasuredWidth()/getMeasuredHeight()).

I didn't think about this – thanks for catching it. That being said, I looked this up:

1) `getMeasuredWidth/Height` returns the dimension the view measures for itself so that the parent can decide to give it its full size whereas `getWidth/Height` returns the dimension the parent actually assigned the child. `mActualWidth/Height` correspond to the actual size so `getMeasured*` is not useful to us here.

2) `getWidth/Height` is a computation from `mBottom` & friends, which get assigned from `setBottom` or `setFrame`.

Both `setFrame` and `setBottom` (& friends) first assign the new values then call `sizeChange`, which calls `onSizeChanged`. All of our code waits for `onSizeChanged` before using width/height values so they should be valid. In fact, this is where `mActualWidth/Height` was getting assigned in the first place.

I'll add a comment to make sure this is clear to developers coming across the code.

---

Bonus info: `setFrame` will be called from `layout` so the `getWidth/Height` values should be set after the first layout.
Comment on attachment 8887171 [details]
Bug 1341275 - review: Add comment explaining FaviconView.getWidth non-zero values.

https://reviewboard.mozilla.org/r/157912/#review163042

r=trivial.
Attachment #8887171 - Flags: review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d565d004c4af
Remove unused constants in FaviconView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/8f2c2231ebfd
Rm redundant mActualHeight/Width variables. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/fd4091dc602f
Rm redundant check for size change in onSizeChanged. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c3b8710c0d73
Replace color 0 with Color.TRANSPARENT. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/f961f2145216
Add mShouldShowImage flag to FaviconView. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/d7b2b1eb46a0
review: Add comment explaining FaviconView.getWidth non-zero values. r=mcomella
Whiteboard: [MobileAS] → [MobileAS] 1.26
Iteration: --- → 1.26
Priority: P2 → P1
We are about to go to 55 RC. Mark 55 won't fix.
Devices:
 - HTC 10 (Android 7.0);
 - Nexus 5 (Android 6.0.1);
 - Note 4 (Android 5.0.1).

Verified this in the latest Nightly using the steps in Comment 0. Long tapped instead of tapping on 3 dot menu since the design has changed. The issue was no longer reproducible.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.