Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug]Improperly displayed Tabs Tray, Top Sites, and search engine "More options" overlay #17092

Closed
AndiAJ opened this issue Dec 17, 2020 · 18 comments
Assignees
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified Feature:Shortcuts Top Sites/Topsites on the Firefox home page Feature:Tabs needs:ac Needs Android Component Work S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist
Milestone

Comments

@AndiAJ
Copy link
Collaborator

AndiAJ commented Dec 17, 2020

Prerequisites

Have fresh install or a clean profile

Steps to reproduce

  1. Open the Tabs Tray
  2. Tap the "More options" overlay
  3. Check the position of the overlay on the screen

Expected behavior

The overlay should emerge from the ⋮ "More options" button

Actual behavior

The overlay is misplaced, displayed on the upper right corner of the screen

Device information

  • Android device:
    • OnePlus A3 (Android 6.0.1)
    • Xiaomi Mi Pad 2 (Android 5.1)

  • Fenix version:
    • Nightly 201217
    • 85.0.0-beta.1

Notes

❗ Reproducible after long tapping any Top Site
❗ Reproducible with both List and Grid view

► Video
20201217-102507

┆Issue is synchronized with this Jira Task

@AndiAJ AndiAJ added 🐞 bug Crashes, Something isn't working, .. Feature:Tabs S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist labels Dec 17, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Dec 17, 2020
@AndiAJ AndiAJ removed the needs:triage Issue needs triage label Dec 17, 2020
@AndiAJ AndiAJ changed the title [Bug]Improperly displayed empty Tabs Tray "More options" overlay [Bug]Improperly displayed Tabs Tray and Top Sites "More options" overlay Dec 17, 2020
@AndiAJ AndiAJ added the Feature:Shortcuts Top Sites/Topsites on the Firefox home page label Dec 17, 2020
@ebalazs-sv
Copy link

This issue is reproducible on Beta 85.0.0-beta.6 from 1/8 with Nexus 5 (Android 6.0.1).

@rocketsroger rocketsroger self-assigned this Jan 18, 2021
@rocketsroger rocketsroger added the needs:ac Needs Android Component Work label Jan 18, 2021
@rocketsroger
Copy link
Contributor

@codrut-topliceanu
Copy link
Contributor

@rocketsroger
Hey, I've spent some time looking into this issue.
The fix I've found is in AC in BrowserMenu.kt

private fun PopupWindow.showAtAnchorLocation(anchor: View, isCloserToTop: Boolean) {
    val locationOnScreen = IntArray(2)
    anchor.getLocationOnScreen(locationOnScreen)

    if (isCloserToTop) {
        animationStyle = R.style.Mozac_Browser_Menu_Animation_OverflowMenuTop
        showAtLocation(anchor, Gravity.NO_GRAVITY,
        locationOnScreen[0], locationOnScreen[1])
    } else {
        animationStyle = R.style.Mozac_Browser_Menu_Animation_OverflowMenuBottom
        showAtLocation(anchor, Gravity.NO_GRAVITY,
            locationOnScreen[0], locationOnScreen[1])
    }
}

@rocketsroger
Copy link
Contributor

@rocketsroger
Hey, I've spent some time looking into this issue.
The fix I've found is in AC in BrowserMenu.kt

@codrut-topliceanu I agree that showAtAnchorLocation probably need some more fixes. However, I think since the original issue was reproducible on Huawei devices only we should limit that fix so most of our use cases goes through the same path. I think it would be a great idea if you put up your fix as a separate change so the workaround is better as well. Let me know when you have a PR for it I can review it quickly. Thanks

@mcarare
Copy link
Contributor

mcarare commented Jan 19, 2021

@rocketsroger I think the original report was on a Xiaomi tablet: #15344.
This was fixed by using the code referenced by @codrut-topliceanu.

@rocketsroger
Copy link
Contributor

@rocketsroger I think the original report was on a Xiaomi tablet: #15344.
This was fixed by using the code referenced by @codrut-topliceanu.

@mcarare I see, it was my mistake. I didn't see Xiaomi was part of the issue as well. I will add Xiaomi as part of the check. The reason I wanted to limit the fix to only affected devices is that so we can use one algorithm for most of the devices out there. I will also add @codrut-topliceanu 's fix as well?

@mcarare
Copy link
Contributor

mcarare commented Jan 19, 2021

Yes, I think that should fix the issue. I am not sure the device filtering is needed, though.

@rocketsroger
Copy link
Contributor

@mcarare looks like @codrut-topliceanu changes is more risky than I thought. It breaks some of the unit tests. There are two other places that calls the function. I'll leave it for @codrut-topliceanu to apply the patch. I do think we risk breaking other things if we change showAtAnchorLocation though.

I think the safest way to go is to limit the workaround since we know the normal code path works except for Huawei and Xiaomi devices.

@codrut-topliceanu
Copy link
Contributor

@rocketsroger Thank's for the issue ❤️ . Sometimes I start working on things and forget to assign myself, apologies.

To work on this issue I've reproduced the bug on an LG K4 (Android 6), additionally @AndiAJ reproduced the issue on an OnePlus A3 (Android 6.0.1) and @ebalazs-sv reproduced it on Nexus 5 (Android 6.0.1). There may be other devices out there there besides these and Huawei/Xiaomi that could encounter this bug. So I'm not sure making the workaround only for these devices is advised.

Additionally I've tested, and found the fix to work, for the following pop-up windows in both portrait and landscape modes:

  • Tabs Tray overflow menu
  • Top Sites overflow menu
  • 3 dot menu
  • Collections item overflow menu
  • Search Engine radio buttons menu

But I do agree that this is a risky change, so great care should be taken when we review+test this.

@rocketsroger
Copy link
Contributor

@codrut-topliceanu can you please also take a look at my change? I still think even with your fix we should still limit the workaround to Huawei and Xiaomi devices unless other devices starts to see the same problem. Thanks,

@codrut-topliceanu
Copy link
Contributor

@rocketsroger could you link your version please? I can't seem to find it.

@rocketsroger
Copy link
Contributor

@rocketsroger could you link your version please? I can't seem to find it.

Sure, here's the pull request mozilla-mobile/android-components#9440. Thanks,

@lobontiumira
Copy link

I was able to reproduce this when accessing the three-dot menu from the default search engines, on Beta 86.0.0-beta.2, with Samsung Galaxy Tab A6 (Android 5.1.1):

AnyName

@lobontiumira lobontiumira changed the title [Bug]Improperly displayed Tabs Tray and Top Sites "More options" overlay [Bug]Improperly displayed Tabs Tray, Top Sites, and search engine "More options" overlay Feb 2, 2021
@rocketsroger
Copy link
Contributor

rocketsroger commented Feb 2, 2021

I was able to reproduce this when accessing the three-dot menu from the default search engines, on Beta 86.0.0-beta.2, with Samsung Galaxy Tab A6 (Android 5.1.1):

Sorry should have been more clear. This issue still need mozilla-mobile/android-components#9560 to be merged to fix. This problem is broken by the original workaround.

@rocketsroger
Copy link
Contributor

I have removed the workaround that causes all these issues. Tomorrow's Nightly build should have the fix. Please verify. Thanks!

@rocketsroger rocketsroger added the eng:qa:needed QA Needed label Feb 2, 2021
@ebalazs-sv
Copy link

Verified as fixed on Nightly 2/3 with Nexus 5 (Android 6.0.1).
I will close this issue and remove the qa:needed label.

@ebalazs-sv ebalazs-sv added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Feb 3, 2021
@AndiAJ
Copy link
Collaborator Author

AndiAJ commented Feb 9, 2021

Still reproducible on 85.1.3 using a OnePlus A3 (Android 6.0.1)

@rocketsroger
Copy link
Contributor

Still reproducible on 85.1.3 using a OnePlus A3 (Android 6.0.1)

Yes, I've confirmed that this fix will only show up in 86.0.0-beta.3 or above. Thanks,

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified Feature:Shortcuts Top Sites/Topsites on the Firefox home page Feature:Tabs needs:ac Needs Android Component Work S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist
Projects
None yet
Development

No branches or pull requests

6 participants