Closed Bug 1098459 Opened 5 years ago Closed 5 years ago

Remove dead area on the right of "new tab" button

Categories

(Firefox for Android :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file, 1 obsolete file)

Let's make it part of the "new tab" button's hit area instead.
Attachment #8522359 - Flags: review?(michael.l.comella)
Priority: -- → P1
Comment on attachment 8522359 [details] [diff] [review]
Remove dead area on the left of "new tab" button (r=mcomella)

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

I see this removing a dead space to the right of the new tab button, not the left, but to be fair, I'm not familiar enough with this issue in the first place. Can you explain a little how this fixes the issue?

::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
@@ +19,3 @@
>          android:src="@drawable/tab_new_level"
>          android:contentDescription="@string/new_tab"
> +        android:layout_gravity="start"

I never really understood `layout_gravity="start/end"` - is this case equivalent to `layout_gravity="top|left"`?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8522359 [details] [diff] [review]
> Remove dead area on the left of "new tab" button (r=mcomella)
> 
> Review of attachment 8522359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see this removing a dead space to the right of the new tab button, not the
> left, but to be fair, I'm not familiar enough with this issue in the first
> place. Can you explain a little how this fixes the issue?

Long story short: we're using margin to align the 'new tab' button with the menu button in the toolbar. Using margin there means we're adding a dead area on the right of the 'new tab' button. Stepping back a bit, I think a simpler/better way to fix this is to use a TouchDelegate instead. I'll post a new patch.

> 
> ::: mobile/android/base/newtablet/res/layout-large-v11/tab_strip.xml
> @@ +19,3 @@
> >          android:src="@drawable/tab_new_level"
> >          android:contentDescription="@string/new_tab"
> > +        android:layout_gravity="start"
> 
> I never really understood `layout_gravity="start/end"` - is this case
> equivalent to `layout_gravity="top|left"`?

start/end only deals with left/right. It's the way build RTL/LTR-agnostic UIs.
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #3)
> Using margin there means we're adding a dead
> area on the right of the 'new tab' button.
Summary: Remove dead area on the left of "new tab" button → Remove dead area on the right of "new tab" button
Comment on attachment 8522359 [details] [diff] [review]
Remove dead area on the left of "new tab" button (r=mcomella)

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

(In reply to Lucas Rocha (:lucasr) from comment #3)
> I think a simpler/better way to fix this is to use a
> TouchDelegate instead.

I think this sounds more complicated than the current solution.
Attachment #8522359 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8526682 [details] [diff] [review]
Remove dead area on the right of "new tab" button (r=mcomella)

Here's a much simpler/saner approach.
Attachment #8526682 - Flags: review?(michael.l.comella)
Attachment #8522359 - Attachment is obsolete: true
Comment on attachment 8526682 [details] [diff] [review]
Remove dead area on the right of "new tab" button (r=mcomella)

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

Ahh, I see - this is pretty clean.

I'm a little concerned that other developers might change the layout and not know why the right side of the tab strip creates new tabs so it'd be good to add a comment to the addTab button's declaration in the layout xml that mentions there is a touch delegate associated with this view.
Attachment #8526682 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #8)
> Comment on attachment 8526682 [details] [diff] [review]
> Remove dead area on the right of "new tab" button (r=mcomella)
> 
> Review of attachment 8526682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ahh, I see - this is pretty clean.
> 
> I'm a little concerned that other developers might change the layout and not
> know why the right side of the tab strip creates new tabs so it'd be good to
> add a comment to the addTab button's declaration in the layout xml that
> mentions there is a touch delegate associated with this view.

Ok, done.
https://hg.mozilla.org/mozilla-central/rev/245b3f7fcb7a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.