1219393 - search icon spacing depends on the device orientation

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P3)

Description

[microrffr]

Attachment: tmp_10807-Screenshot_2015-10-28-11-46-35975084416.png

User Agent: Mozilla/5.0 (Android 5.1.1; Mobile; rv:44.0) Gecko/44.0 Firefox/44.0
Build ID: 20151027030239

Steps to reproduce:

Have at least two search engines.
Be on a web page (not sure if this happens in about:home).
Go to the address bar and type something.
The non-default search engine icon shows up on the bottom, centered.
Close out of the address bar, going back to the web page.
Rotate the device to the other orientation.
Go to the address bar and type something.


Actual results:

The non-default search engine icon is not centered.


Expected results:

that it be centered

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to confirm. Since it has a complicated STR, I'd say it's probably a good mentored, polish bug.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-11-06-16-41-35.png

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0a1 (2015-11-05)

Using the steps provided and rotate the device several times from portrait to landscape and viceversa, the search engine icon is not centered.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Confirm this behavior on about:home simpler str

* tap in the address bar type a search but don't complete the search
* exit the search via the x or the back button
* change the orientation of the phone
* tap into the address bar and type a search

the search engine will not be in the center

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Portrait centered search engine

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Portrait uncentered search engine

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Landscape centered search engine

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Landscape uncentered search engine

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks for the confirmation – let's set this up as a mentored bug.

The code you'll want to look at is in BrowserSearch, which is the Fragment that houses the SearchEngineBar. The SearchEngineBar is the UI widget that displays the icons of the search engines.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think for this case, it makes more sense to be left aligned, right next to the Search icon there.

Mike, does this complicate/change things? if not let's go down that route :)

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #9)
> Mike, does this complicate/change things? if not let's go down that route :)

I think we can special case it fairly simply so let's go for it.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Carrying forward the +

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Ni-ing Ricardo to think about this

Comment 14

[Kevin Lam]

Hi I have a working patch for this bug completed if its available for a portrait-centered solution. Should I make the change so that the icons are left aligned instead?

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Awesome Kevin!

Could we see some screenshots of your patch?

Comment 16

[Kevin Lam]

Attachment: KevinLam Landscape centered favicon

Before fix: 

Icon off center slightly to the left.


After fix:

Icon centered.


Steps to reproduce:
1. Open up a new browser.
2. Type something in the search bar. Icon is centered.
3. Close the search bar by deleting all the characters or pressing the x icon.
4. Rotate the device.
5. Type something in the search bar.

Comment 17

[Kevin Lam]

Attachment: KevinLam Portrait Centered favicon

Before fix:

Icon hugs the right side of the device's screen.


After fix:

Icon is centered.


Steps to reproduce:
1. Open up a new browser.
2. Type something in the search bar. Icon is centered.
3. Rotate the device.
4. Close the search bar by deleting all the characters or pressing the x icon.
5. Rotate the device again.
5. Type something in the search bar.

Comment 18

[Kevin Lam]

Attachment: KevinLam bug1219393.patch

Here is my patch for the screenshot results shown above.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Anthony, what do you think?

By the way, Kevin, you can use the "Need more information from" field to give a user a persistent notification so users will be more likely to see your comments or questions.

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Kevin Lam from comment #18)
> Created attachment 8782678 [details] [diff] [review]
> KevinLam bug1219393.patch
> 
> Here is my patch for the screenshot results shown above.

Hey Kavin!

Sorry I didn't see this go by.

Would it be possible to left-align the remaining icons instead? That way we wouldn't have to worry about new padding.

Comment 21

[Kevin Lam]

Attachment: KevinLam Potrait Highlighted Icon

Not a problem Anthony.

I don't quite understand what you mean by remaining icons. The icons are currently centered in each holder-container shown by the gray highlight. Just to clarify, are you looking to left-align the icons inside each container?

The problem before was that the resizing measurements (for the container when switching between landscape and portrait) was stored but the visuals were not updated upon device orientation change. I only included the refreshing part without changing padding or layout.

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Kevin Lam from comment #21)
> Created attachment 8786553 [details]
> KevinLam Potrait Highlighted Icon
> 
> Not a problem Anthony.
> 
> I don't quite understand what you mean by remaining icons. The icons are
> currently centered in each holder-container shown by the gray highlight.
> Just to clarify, are you looking to left-align the icons inside each
> container?

Nope. I'm looking to left-align the holder-containers themselves. So that when a user removes a couple default search engines, the remaining ones are left-aligned rather than centered (as in comment 17)

Comment 23

[Kevin Lam]

Attachment: KevinLam Left-Aligned Two Icons

Oh, is it something similar to this where each of the containers are of fixed size and they do not fully expand to fill the remainder of search-engine bar? If this is the case, would it be preferable for the containers to be smaller in portrait and larger in landscape or the same size in both scenarios?

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Kevin Lam from comment #23)
> Created attachment 8787442 [details]
> KevinLam Left-Aligned Two Icons
> 
> Oh, is it something similar to this where each of the containers are of
> fixed size and they do not fully expand to fill the remainder of
> search-engine bar? If this is the case, would it be preferable for the
> containers to be smaller in portrait and larger in landscape or the same
> size in both scenarios?

Yes, like this.

But I don't think we should resize the containers to be smaller in portrait and larger in landscape. They should be the same size in both scenarios because its about tap-ability. :)

Comment 25

[Kevin Lam]

Attachment: KevinLam bug1219393.patch

Just removed the sections that affect resizing of the holder-containers upon device orientation change. This enables left alignment of the containers. The containers will be of fixed size regardless of screen aspect ratios or orientation.

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8788004 [details] [diff] [review]
KevinLam bug1219393.patch

Hey Kevin! got some screenshots I could look at? I just want to check all the "circumstances" are covered :)

I'll throw the patch review over to mcomella.

Comment 27

[Kevin Lam]

Attachment: KevinLam Left-Aligned Circumstance 1 (Filled)

After repeating the steps in comment #16 with a filled search-engine bar, the result is as expected.

Comment 28

[Kevin Lam]

Attachment: KevinLam Left-Aligned Circumstance 2 (Filled)

After repeating the steps in comment #17 (Portrait->Landscape->Portrait), the result is as expected.

Comment 29

[Kevin Lam]

Attachment: KevinLam Left-Aligned Circumstance 1 (Single)

Following the steps in comment #16 with a single icon in the search-engine bar, the result is left-aligned.

Comment 30

[Kevin Lam]

Attachment: KevinLam Left-Aligned Circumstance 2 (Single)

Following the steps in comment #17 with a single icon in the search-engine bar (portrait -> landscape -> portrait), the result is left aligned.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8788004 [details] [diff] [review]
KevinLam bug1219393.patch

Review of attachment 8788004 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't seem to be right. It just removes all the dynamic spacing.

I guess what we want is:
* If the icons do not fit into the bar then keep the current algorithm where the last visible icon is only shown half (to indicate that the bar can be scrolled)
* If there are less icons and no scrolling is needed then (this is new) left align the icons and do not use the algorithm.

Comment 32

[Kevin Lam]

From my understanding, the search engine bar's default functionality, without the dynamic spacing, left aligns the icons. Is it sufficient to use this without specifying any further code i.e. if icons do not fit the bar -> use current algorithm (leave the other condition to be handled by the default). Or is it necessary to explicitly left align the icons?

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Kevin Lam from comment #32)
> From my understanding, the search engine bar's default functionality,
> without the dynamic spacing, left aligns the icons. Is it sufficient to use
> this without specifying any further code i.e. if icons do not fit the bar ->
> use current algorithm (leave the other condition to be handled by the
> default). Or is it necessary to explicitly left align the icons?

Yes, using the default behavior (maybe resetting the size of the container?) in this case should be good enough.

Comment 34

[Kevin Lam]

Attachment: KevinLam bug1219393.patch Retain algorithm

These changes kept the dynamic spacing algorithm the same. The only change is resetting the container back to its default size when the total size required is less than the maximum size available.

I also checked the special circumstances with these changes. The result is the same as the previous images I posted.

Comment 35

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8790127 [details] [diff] [review]
KevinLam bug1219393.patch Retain algorithm

Review of attachment 8790127 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good.

::: mobile/android/base/java/org/mozilla/gecko/home/SearchEngineBar.java
@@ +96,5 @@
>                  // All search engines fit int: So let's just display all.
> +                final float defaultIconContainerWidth = TypedValue.applyDimension(
> +                        TypedValue.COMPLEX_UNIT_DIP, ICON_CONTAINER_MIN_WIDTH_DP,
> +                                getResources().getDisplayMetrics());
> +                mIconContainerWidth = Math.round(defaultIconContainerWidth);

You should already have mMinIconContainerWidth available here and do not need to calculate it again.

Comment 36

[Kevin Lam]

Attachment: KevinLam bug1219393.patch Retain Algorithm.02

Awesome. I made the change to use mMinIconContainerWidth. I also retested the special cases on my Android device. Hopefully, it's good to go.

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8794024 [details] [diff] [review]
KevinLam bug1219393.patch Retain Algorithm.02

Review of attachment 8794024 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kevin. Thanks for the patch and sorry for the delay!

The patch is looking good. I'll land it. :)

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a79b4164b66464e07347cb22b1648f39a8ed89b
Bug 1219393 - Left aligned search icons only when there are less icons than required to fill the search icon bar. r=sebastian

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a79b4164b66

Comment 40

[Sorina Florean [:sflorean]]

Verified as fixed in build 52.0a1 2016-10-12;
Device: LG G4 (Android 6.0) and Nexus 9 (Android 6.0.1).