[Bug]: The search engine selector should have a 4px padding top and left

[DreVla]

Steps to reproduce

1. Open search selector menu from home fragment or search dialog
2. Check menu padding

Expected behaviour

The menu should have 4dp padding left and bottom/top (depending on toolbar position)

Actual behaviour

The menu is not placed correctly In SearchDialogFragment

In HomeFragment

Device name

No response

Android version

11

Firefox release type

Firefox Nightly

Firefox version

109.0a1

Device logs

No response

Additional information

No response

┆Issue is synchronized with this Jira Task

[DreVla]

I have worked on this task for a while. During this time I realized there are multiple issues that make fixing this more difficult than it seems. There are four cases here:

When opened from HomeFragment

When toolbar at bottom

When toolbar at top

When opened from SearchDialogFragment

When toolbar at bottom

When toolbar at top

After testing each of these, I noticed none of them had the right alignment, but case 2a was the closest. 

I supposed there was a problem witht the offsets. The menu that pops up is an element from Android Components, PopupWindow. It is displayed depending on orientation and screen space using two methods from android showAsDropDown and showAtLocation that can receive x and y offsets as parameters. It seemed nothing was wrong here.

After asking for help from Petru, he suggested we check the anchor for the menu. There are two views that act as anchors, identified by "@+id/search_selector”". We tried changing the anchor to the parent, which led to an improvement but not the expected result, therefore we figured that creating a new anchor and positioning it where we want would solve the issue. 

It did fix the problem, but only in case 1a:

But when the toolbar is at the top, because the constraint was bottom to bottom of parent it appeared as follows:

A solution is to programmatically change the constraint to top to top of parent

The current solution presented in the draft is only for case 1a,b. Arguably, this is a “hacky” implementation, but after many hours of trial and error, it is the only one we have reached for now. For now this will remain as a draft and if anyone can suggest a better implementation please do. Link to PR: https://github.com/mozilla-mobile/fenix/pull/27999

[mavduevskiy]

It appears we worked on that in parallel! I worked on adding the unified search icon to the HomeFragment, but missed the spacing requirement. working with @t-p-white on the cutting the last item in half made me realize that's an issue.

After digging into it, I realized that actually the anchoring works correctly, but we are passing the wrong anchor and also the menu itself has paddings from elevation that we don't account for.

If you look at this picture closely, you will see that the bottom edge of the popup aligns perfectly with the corner of the view we are passing – search_selector. It has margins, so the x coordinate is shifted to teach the start of the margin of the view. Bottom aligns with the anchor bottom.

My solution was to calculate the spacing between the menu and it's container, and offset the container accordingly.

The behavior of the old menu is the same, by the way. It is aligned over the anchor completely, but spacing between the container and menu "shifts" the menu a bit, creating spacing.

Btw, achieving right position is not possible without reducing elevation by at least 1dp – 8dp is too much to have enough space at the TOP orientation. But luckily the design has updated elevation.

[Mugurell]

Thank you for the update. It seems like we have to treat this popup as an edgecase. I was also thinking about a new showAtLocation method with event support for X and Y offsets. Is just that there are many scenarios in which the same popup for unified search is positioned a little bit differently - depending on the toolbar position (and the popup orientation), depending on where it is opened from - the homescreen or from the browser and also depending on LTR/RTL and so any solution would have to account for all of these.

[Mugurell]

Synced with Michael Verdi about the status of unified search and this issue also. As Mike saw there are issues with RTL also and generally many edgecases that would be hard to tackle separate from the current implementation we use as default for showing the menu popups. The patch from https://github.com/mozilla-mobile/firefox-android/pull/386 would help alleviate some issues and then Michael Verdi agreed with the proposal to wait until moving the menus to Compose to place this popup with an exact 4px padding start/bottom.

[mavduevskiy]

changed the order to keep DataSync calm and tranquil on the Jira side

[gabrielluong]

Moved to bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1815548

Change performed by the Move to Bugzilla add-on.