Closed Bug 680077 Opened 8 years ago Closed 8 years ago

Make fennecomb tab menu pretty

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: ibarlow, Assigned: lucasr)

References

Details

Attachments

(6 files, 3 obsolete files)

Attached file Tab menu assets
Let's make tabs look like these mocks: http://www.flickr.com/photos/61892693@N03/6050531024/in/photostream and http://www.flickr.com/photos/61892693@N03/6052859709/in/photostream

Notes:
* We have unique tab menus for landscape and portrait mode
* In landscape mode, the menu is persistent and displays page thumbnails and titles
* In portrait mode, the menu is hidden and can be instantiated by tapping a "tabs" icon in the action bar. The menu appears as a favicon+title list
* Menus are fixed width and height, so tabs should be scrollable. 

More detailed specs: cl.ly/47102P131N0L1G2y0W1M

Visual assets (icons) are attached.
Taking.
Assignee: nobody → lucasr.at.mozilla
Blocks: 655762
OS: Mac OS X → Android
Hardware: x86 → All
Depends on: 677669
Version: Firefox 8 → Trunk
Attachment #558484 - Flags: review?(mbrubeck)
This is just an initial round of updates to make the tabs pane look more like the new honeycomb tablet design. I'll file separate bugs for the remaining bits. This patch relies on the patch for bug 677669 and should not land before it. This patch still doesn't do the proper styling for the tabs list in portrait mode.
Attachment #558488 - Flags: review?(mbrubeck)
Does this bug invalidate bug 664730?
(In reply to Paul [sabret00the] from comment #6)
> Does this bug invalidate bug 664730?

In bug  664730, there's still a point about favicons on the page screenshots which is not covered here. It might be worth discussing that separately there.
Attachment #558482 - Flags: review?(mbrubeck) → review+
Comment on attachment 558484 [details] [diff] [review]
(2/3) Define documenttab dimensions on constructor

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

r=mbrubeck; I can fix the nits on checkin.

::: mobile/chrome/content/tabs.xml
@@ +21,5 @@
>      <implementation>
>        <field name="ignoreUndo">false</field>
>        <field name="thumbnail">document.getAnonymousElementByAttribute(this, "anonid", "thumbnail");</field>
> +      <field name="_reload">document.getAnonymousElementByAttribute(this, "anonid", "reload");</field>
> +      <field name="_closeContainer">document.getAnonymousElementByAttribute(this, "anonid", "close-container");</field>

Let's add readonly="true" to these fields (including the existing "thumbnail" one above).
Attachment #558484 - Flags: review?(mbrubeck) → review+
Comment on attachment 558488 [details] [diff] [review]
(3/3) Update tabs pane for Honeycomb theme

The file "honeycomb/images/close-background-hdpi.png" is missing from this patch.

Some general problems, though not all of these need to be solved now:

1. This doesn't handle switching tablet mode on or off while Fennec is running.  This doesn't have to be a supported use case, but currently it could happen on some devices if they exceed tablet_panel_minwidth in landscape but not portrait. We should either support mode-switching, or ensure that the mode won't change on its own.

2. Related to #1, when I tested this on a desktop build, even when I made it start up with a tablet-sized window, the first thumbnail was created with the smaller non-tablet size.  It looks like the first thumbnail can get created before the window has been sized correctly.

3. This doesn't work for tablet mode in the Froyo or Gingerbread themes.  Not a super high priority, but should be fixed before shipping.  Could be a follow-up bug.

4. It also doesn't work well for non-tablet mode in the Honeycomb theme.  Not a blocker; could be a follow-up bug.

I'd like to address #1 and #2 (or at least have a plan for them) before landing this.
Attachment #558488 - Flags: review?(mbrubeck) → review-
Just a rebased version of Lucas's patch.  I applied this on top of the latest patch in bug 677669 to test it, and resolved some merge conflicts.
Attachment #558482 - Attachment is obsolete: true
Attachment #558613 - Flags: review+
Added readonly attribute to new fields as per review. Keeping the review+.
Attachment #558484 - Attachment is obsolete: true
Attachment #558826 - Flags: review+
Here's a more comprehensive patch to cover the patch review comments. It now handles live tablet mode switch. I've added the tablet-specific bits to tablet.css instead of changing honeycomb directly. I also added separate tablet-specific images for the close and new tab buttons to allow having both tablet and phone mode in each theme. This is not perfect yet, here are some of the known major issues:

- I see tabs with messed up layouts on startup sometimes. Maybe it's related to Matt's issue number 2.
- The tablet switch is not happening nicely. The tab pane is not being resized properly for some reason.

I suspect that both are related to timing issues with layout reflows or something. Given that I changed the patch quite a bit, I thought it would be nice to get it out for review while I investigate those issues.
Attachment #558488 - Attachment is obsolete: true
Attachment #558840 - Flags: review?(mbrubeck)
Blocks: 685224
Comment on attachment 558840 [details] [diff] [review]
(3/3) Update tabs pane for Honeycomb theme

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

This looks great! r=mbrubeck with one nit (below) which I will fix on checkin.

This is broken in RTL locales.  Let's file a followup bug for that, along with any other issues we find.

::: mobile/chrome/content/tabs.xml
@@ +108,5 @@
>              thumbnail.removeAttribute("empty");
>  
> +            // Ensure the thumbnail will have the correct
> +            // dimensions for tablet and phone modes
> +            this.updateTabletLayout(thumbnail);

This line should be moved above the "about:blank" check just above, otherwise blank tabs are not resized correctly.
Attachment #558840 - Flags: review?(mbrubeck) → review+
Depends on: 685308
This was part of the backout that I did because of possible zooming (bug 685541) and panning regressions (bug 685540):

http://hg.mozilla.org/mozilla-central/rev/4be2da039559
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 685539
Blocks: 685568
The positioning of the tab image was off by one pixel on my Android tablet.  (I could also reproduce this on desktop by changing "ifdef ANDROID" to "ifndef ANDROID" in themes/core/honeycomb/defines.inc.)

I couldn't figure out how to set this correctly using mozmm values, so I just switched to hard-coded pixels since the other elements in the sidebar are all px-sized.
Attachment #559234 - Flags: review?(mark.finkle)
Comment on attachment 559234 [details] [diff] [review]
followup: positioning tweak for Android

Can you add a comment in the CSS as to why we are using "px"

Something like:
"The thumbnails are sized based on pixels, so we use px to get accurate positioning"
Attachment #559234 - Flags: review?(mark.finkle) → review+
Depends on: 685549
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Samsung Galaxy Tab 10.1 (Android v3.1)
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110909 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
Blocks: 685868
You need to log in before you can comment on or make changes to this bug.