search icon spacing depends on the device orientation

VERIFIED FIXED in Firefox 52

Status

()

P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: microrffr, Assigned: kevin.lam.cs, Mentored, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 52
Points:
---

Firefox Tracking Flags

(fennec+, firefox52 verified)

Details

(Whiteboard: [lang=java])

Attachments

(13 attachments, 5 obsolete attachments)

88.87 KB, image/png
Details
88.02 KB, image/png
Details
72.10 KB, image/png
Details
85.49 KB, image/png
Details
113.96 KB, image/png
Details
143.61 KB, image/png
Details
170.57 KB, image/png
Details
122.51 KB, image/png
Details
144.11 KB, image/png
Details
226.74 KB, image/png
Details
124.95 KB, image/png
Details
212.18 KB, image/png
Details
2.96 KB, patch
sebastian
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8680149 [details]
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
NI self to confirm. Since it has a complicated STR, I'd say it's probably a good mentored, polish bug.
Flags: needinfo?(michael.l.comella)
Created attachment 8684247 [details]
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(michael.l.comella)
Created attachment 8686318 [details]
Portrait centered search engine
Created attachment 8686319 [details]
Portrait uncentered search engine
Attachment #8680149 - Attachment is obsolete: true
Attachment #8684247 - Attachment is obsolete: true
Created attachment 8686320 [details]
Landscape centered search engine
Created attachment 8686322 [details]
Landscape uncentered search engine
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.
Blocks: 1157964
Mentor: michael.l.comella, s.kaspari
Whiteboard: [lang=java]

Updated

3 years ago
Summary: search engine icon not centered → search engine icon not left aligned
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 :)
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
Summary: search engine icon not left aligned → search icon spacing depends on the device orientation
Duplicate of this bug: 1266431
Carrying forward the +
tracking-fennec: --- → +
Ni-ing Ricardo to think about this
Flags: needinfo?(rvazquez)
Priority: -- → P3
(Assignee)

Comment 14

2 years ago
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?
Flags: needinfo?(alam)
Awesome Kevin!

Could we see some screenshots of your patch?
Flags: needinfo?(alam) → needinfo?(kevin.lam.cs)
(Assignee)

Comment 16

2 years ago
Created attachment 8782673 [details]
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.
(Assignee)

Comment 17

2 years ago
Created attachment 8782676 [details]
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.
(Assignee)

Comment 18

2 years ago
Created attachment 8782678 [details] [diff] [review]
KevinLam bug1219393.patch

Here is my patch for the screenshot results shown above.
(Assignee)

Updated

2 years ago
Flags: needinfo?(kevin.lam.cs)
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.
Flags: needinfo?(alam)
Assignee: nobody → kevin.lam.cs
(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.
Flags: needinfo?(alam)

Updated

2 years ago
Flags: needinfo?(kevin.lam.cs)
(Assignee)

Comment 21

2 years ago
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?

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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(kevin.lam.cs)
(Assignee)

Updated

2 years ago
Flags: needinfo?(alam)
(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)
Flags: needinfo?(alam) → needinfo?(kevin.lam.cs)
(Assignee)

Comment 23

2 years ago
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?
Attachment #8782678 - Attachment is obsolete: true
Flags: needinfo?(kevin.lam.cs) → needinfo?(alam)
(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. :)
Flags: needinfo?(alam)
(Assignee)

Comment 25

2 years ago
Created attachment 8788004 [details] [diff] [review]
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.
Attachment #8788004 - Flags: feedback?(alam)
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.
Flags: needinfo?(michael.l.comella)
Attachment #8788004 - Flags: feedback?(alam)
(Assignee)

Comment 27

2 years ago
Created attachment 8788312 [details]
KevinLam Left-Aligned Circumstance 1 (Filled)

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

Comment 28

2 years ago
Created attachment 8788313 [details]
KevinLam Left-Aligned Circumstance 2 (Filled)

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

Comment 29

2 years ago
Created attachment 8788314 [details]
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.
(Assignee)

Comment 30

2 years ago
Created attachment 8788315 [details]
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 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.
Attachment #8788004 - Flags: feedback+
Flags: needinfo?(michael.l.comella)
(Assignee)

Comment 32

2 years ago
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?
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 34

2 years ago
Created attachment 8790127 [details] [diff] [review]
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.
Attachment #8788004 - Attachment is obsolete: true
Attachment #8790127 - Flags: feedback?(s.kaspari)
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.
Attachment #8790127 - Flags: feedback?(s.kaspari) → feedback+
(Assignee)

Comment 36

2 years ago
Created attachment 8794024 [details] [diff] [review]
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.
Attachment #8790127 - Attachment is obsolete: true
Attachment #8794024 - Flags: review?(s.kaspari)
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. :)
Attachment #8794024 - Flags: review?(s.kaspari) → review+
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4a79b4164b66
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Verified as fixed in build 52.0a1 2016-10-12;
Device: LG G4 (Android 6.0) and Nexus 9 (Android 6.0.1).
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.