Closed
Bug 1219393
Opened 9 years ago
Closed 8 years ago
search icon spacing depends on the device orientation
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect, P3)
Firefox for Android Graveyard
Awesomescreen
Tracking
(fennec+, firefox52 verified)
VERIFIED
FIXED
Firefox 52
People
(Reporter: microrffr, Assigned: kevin.lam.cs, Mentored, NeedInfo)
References
Details
(Whiteboard: [lang=java])
Attachments
(13 files, 5 obsolete files)
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 |
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)
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
Attachment #8680149 -
Attachment is obsolete: true
Attachment #8684247 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
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.
Updated•9 years ago
|
Summary: search engine icon not centered → search engine icon not left aligned
Comment 9•9 years ago
|
||
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)
Updated•9 years ago
|
Summary: search engine icon not left aligned → search icon spacing depends on the device orientation
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 14•8 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?
Updated•8 years ago
|
Flags: needinfo?(alam)
Comment 15•8 years ago
|
||
Awesome Kevin!
Could we see some screenshots of your patch?
Flags: needinfo?(alam) → needinfo?(kevin.lam.cs)
Assignee | ||
Comment 16•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Here is my patch for the screenshot results shown above.
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
Comment 20•8 years ago
|
||
(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•8 years ago
|
Flags: needinfo?(kevin.lam.cs)
Assignee | ||
Comment 21•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
(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•8 years ago
|
||
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 26•8 years ago
|
||
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•8 years ago
|
||
After repeating the steps in comment #16 with a filled search-engine bar, the result is as expected.
Assignee | ||
Comment 28•8 years ago
|
||
After repeating the steps in comment #17 (Portrait->Landscape->Portrait), the result is as expected.
Assignee | ||
Comment 29•8 years ago
|
||
Following the steps in comment #16 with a single icon in the search-engine bar, the result is left-aligned.
Assignee | ||
Comment 30•8 years ago
|
||
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•8 years ago
|
||
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•8 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)
Comment 33•8 years ago
|
||
(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•8 years ago
|
||
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 35•8 years ago
|
||
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•8 years ago
|
||
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 37•8 years ago
|
||
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+
Comment 38•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 40•8 years ago
|
||
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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•