Closed Bug 1039738 Opened 5 years ago Closed 5 years ago

Private tabs list is cut off for portait orientation

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox32 --- unaffected
firefox33 --- verified
fennec 33+ ---

People

(Reporter: bnicholson, Assigned: mcomella)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Open two private tabs, then open the tabs tray: only one private tab is visible. It looks like the ListView height isn't filling the TabsTray as expected.
tracking-fennec: --- → ?
Blocks: 1021356
This is caused by bug 1021356 part 1, likely due to the change to a ScrollView.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Comment on attachment 8457516 [details] [diff] [review]
Allowing scrolling in private tabs list

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

::: mobile/android/base/resources/layout/private_tabs_panel.xml
@@ +5,5 @@
>  
>  <merge xmlns:android="http://schemas.android.com/apk/res/android"
>         xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
> +    <ScrollView android:layout_width="match_parent"

Why do we need a ScrollView at all? TwoWayView is already scrollable.

@@ +45,5 @@
>  
> +    </ScrollView>
> +
> +    <!-- Note: scrolling in the TabsTray does not work
> +         unless it is laid out after the empty view. -->

Why?

Also, note that even with this bug, I *can* still scroll the tabs tray -- it's just not the right height. Since the regressing bug changed PrivateTabsPanel to extend ScrollView instead of FrameLayout, I would expect the fix here to be to add fillViewport="true" to the ScrollView. Does that not work? Still curious why a ScrollView is needed in the first place, though.
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Why do we need a ScrollView at all? TwoWayView is already scrollable.

Just to be clear, I'm talking about the TabsTray (which extends TwoWayView) in this layout file.
Comment on attachment 8457516 [details] [diff] [review]
Allowing scrolling in private tabs list

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

I was more focused on bug 1021356 than this one -- the ScrollView comment above isn't relevant to your fix here since you're moving the TabsTray outside of it. I'm still curious about your note. Why does scrolling in the TabsTray not work unless it is laid out after the empty view?
Attachment #8457516 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #5)
> I was more focused on bug 1021356 than this one -- the ScrollView comment
> above isn't relevant to your fix here since you're moving the TabsTray
> outside of it. I'm still curious about your note. Why does scrolling in the
> TabsTray not work unless it is laid out after the empty view?

I'm not sure. I imagine it has something to do with how the .setEmptyView and related code handle which view (i.e. the empty view and not-so-empty view) is displayed and thus how the click events are swallowed by each view.

I'll add, "For an unknown reason..." to the comment.
Updated patch (and updated description, given comment 3's clarification.
Attachment #8457516 - Attachment is obsolete: true
Comment on attachment 8457663 [details] [diff] [review]
Correct private tabs list appearance

Move r+.
Attachment #8457663 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a794759a5272
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
tracking-fennec: ? → 33+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.