Closed Bug 1201623 Opened 4 years ago Closed 4 years ago

On tablets URL bar, about:home magnifying glass and globe icon take up different widths

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: antlam, Assigned: sergej, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java][lang=xml])

Attachments

(1 file, 2 obsolete files)

STR:
 - Firefox on Tablet
 - Open a new tab
 - Click the URL bar
 - Repeat
 - Notice the icon shift the input cursor to the right when it changes from the globe icon to the search icon

Let's clean this up and make the padding the same.
Perhaps mentor-able?
bug 1173652 was open to make these the same layouts, thus fixing this issue, but that would add a lot of code complexity and is maybe not worth it.

It's unclear how difficult this might be.
Mentor: michael.l.comella, mhaigh
See Also: → 1173652
Whiteboard: [lang=java][lang=xml]
Attached patch padding.diff (obsolete) — Splinter Review
The padding was too big.
 
Display and edit layout merge will require way more work and must be done in corresponding bug.
Attachment #8663284 - Flags: review?(michael.l.comella)
Assignee: nobody → sergej
Comment on attachment 8663284 [details] [diff] [review]
padding.diff

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

This is better, but on my N9, the editing mode text still shifts to the right when the url bar is tapped.

Did you try rewriting the layout to use a more absolute positioning? e.g. the "favicon" container is always the same size, or the alignment happens from the right, or...
Attachment #8663284 - Flags: review?(michael.l.comella) → feedback+
Icon in edit layout is inside EditText as left drawable. To change this, icon must be moved to linear layout from edit text and corresponding code from ToolbarEditText to ToolbarEditLayout
(In reply to Sergej Kravcenko from comment #5)
> Icon in edit layout is inside EditText as left drawable. To change this,
> icon must be moved to linear layout from edit text and corresponding code
> from ToolbarEditText to ToolbarEditLayout

Works for me.
Attached patch toolbar.diff (obsolete) — Splinter Review
Should also fix bug 1173652.

This is not full merge, I've just made edit layout icon exact copy of display layout icon. The only thing that left is some misleading browser_toolbar_site_security_padding_* names inside toolbar_edit_layout.xml
Not sure how to generalise these names to look right.
Attachment #8664584 - Flags: review?(michael.l.comella)
I'll get to this tomorrow – don't forget to obsolete your old patch, like in the other bug!
Attachment #8663284 - Attachment is obsolete: true
Comment on attachment 8664584 [details] [diff] [review]
toolbar.diff

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

Nice!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=795cdd4674ed

Fix the comment, upload your patch, and feel free to add "checkin-needed" without re-review, provided the push is green.

Thanks Sergej!

::: mobile/android/base/resources/layout/toolbar_edit_layout.xml
@@ +6,5 @@
>  <merge xmlns:android="http://schemas.android.com/apk/res/android"
>         xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
> +    <!-- Search icon must have exact position and size as site security in
> +         display layout -->

Add an equivalent comment to the display layout.
Attachment #8664584 - Flags: review?(michael.l.comella) → review+
Attachment #8664584 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, had to back this out in https://hg.mozilla.org/integration/fx-team/rev/39a4d02f17db seems one of this changes caused a bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4833952&repo=fx-team
Flags: needinfo?(sergej)
Looks like the failures were caused by bug 1144707 – the try in comment 9 is clean.
Flags: needinfo?(sergej)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/24644fbe805f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.