680077 - Make fennecomb tab menu pretty

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: 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.

Comment 1

[Lucas Rocha (:lucasr)]

Taking.

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Use Util.isTablet() everywhere to check if tablet mode is on

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Define documenttab dimensions on constructor

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Update tabs pane for Honeycomb theme

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.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Screenshot with patches applied

Comment 6

[Paul [pwd]]

Does this bug invalidate bug 664730?

Comment 7

[Lucas Rocha (:lucasr)]

(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.

Comment 8

[Matt Brubeck (:mbrubeck)]

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).

Comment 9

[Matt Brubeck (:mbrubeck)]

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.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: (1/3) Use Util.isTablet() everywhere to check if tablet mode is on (v2)

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.

Comment 11

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Define documenttab dimensions on constructor

Added readonly attribute to new fields as per review. Keeping the review+.

Comment 12

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Update tabs pane for Honeycomb theme

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.

Comment 13

[Matt Brubeck (:mbrubeck)]

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.

Comment 14

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/8d3475475a0b
http://hg.mozilla.org/mozilla-central/rev/84c0fbc1cf1d
http://hg.mozilla.org/mozilla-central/rev/b7d269a291b6

Comment 15

[(no longer active)]

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

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: followup: positioning tweak for Android

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.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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"

Comment 18

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d89c6e0d4096
http://hg.mozilla.org/integration/mozilla-inbound/rev/3644b7ed6cf1
http://hg.mozilla.org/integration/mozilla-inbound/rev/6b2bda929c54
http://hg.mozilla.org/integration/mozilla-inbound/rev/65ce309835c9

Comment 19

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/d89c6e0d4096
http://hg.mozilla.org/mozilla-central/rev/3644b7ed6cf1
http://hg.mozilla.org/mozilla-central/rev/6b2bda929c54
http://hg.mozilla.org/mozilla-central/rev/65ce309835c9

Comment 20

[Aaron Train [:aaronmt]]

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