680212 - Make fennecomb awesomebar pretty

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Awesomebar assets

Let's make the tablet version of our awesomebar look like this: http://www.flickr.com/photos/61892693@N03/6057032350/in/photostream

Notes:
* We've taken the tabbed menu approach from phones and moved it over to the left, to give us an extra row for results when the keyboard is up
* The awesomebar list scrolls if it's longer than 5 items
* Tapping the search icon brings up an options menu for search results like this http://www.flickr.com/photos/61892693@N03/6056484409/in/photostream/
* As in the phone, once you start typing the tabs go away and you just see awesomebar results, similarly to our phone UI

Some more detailed specs are available here http://cl.ly/2O163B233x2y3i103Y22

Assets are attached :)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP

Moving over my work from bug 677666 and closing it. This also starts some themeing. Build is at:

http://dl.dropbox.com/u/72157/fennec-honeycomb1.apk

(same as my other build).

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: menu inner glow

Thanks for posting this Wes, my comments are included here: http://cl.ly/133q0m1C383t2U063w3N

Comment 3

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

I would encourage Wes and Ian to get somehting "mostly right" landed ASAP and tweak the pixels in followup bugs. Some of these tweaks are not as simple as the seem, have cascading repercussions, and should not hold up landing the major feature.

Also note that changes to colors (tab backgrounds, selections, etc.) should be uniform across the UI. We try not to special case single elements.

Comment 4

[Ian Barlow (:ibarlow)]

"Mostly right" should probably include

* Fix the alignment of the awesomebar in relation to the url bar
* Fix the width of the awesomebar
* Fix the font sizes in the awesomebar
* Fix selection colours

Comment 5

[Wesley Johnston (:wesj)]

Attachment: WIP

Saving my work again. This themes the search providers as well.

Still some issues to work out with sizing and events. I'm not sure what problem you were seeing ian. Need to see if I can reproduce that somehow. Despite my best efforts to force it not to be, the search popup initially also wants to be window width wide. The second time it is shown things are fine.

I'm also trying to figure out the best way to handle the awesomescreen. We currently push it as a "dialog" which means that it is removed when you hit Back/Esc/etc. but not when you click outside it (before, you couldn't click outside it). Pushing as a dialog fixes this, but causes other problems with focus in the urlbar and with other popups/dialogs like the search box or the sync dialog. I think fixing the first option (push as a dialog) sounds a bit easier, so I'm just going to add some tap listeners and hide it if the tap is not inside the awesome screen, and if another dialog/popup isn't being shown.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Working pretty well now. I had originally moved the AwesomeScreen code out of BrowserUI so that I could use pushPopup instead of pushDialog. That turned out to cause enough test failures and headaches that I threw it away, and instead have opted for a TapDown listener, and a special list of items you can click on while this is open and not dismiss the awesomescreen. That also helps fix some bugs with hitting the back button, menu button, etc. while the awesome screen is showing.

I've hid a lot of this behind a media query. From what I've heard, we're phasing that out so I'm open to attaching some more listeners to the tablet broadcaster if we want. But I really like using media queries for this stuff, and think in my ideal world we'd use css media queries + matchMedia for all tablet theming.

Finally, I'm positioning these elements using popup.left most of the time. I've had to do some finagling to make sure things work in ltr and rtl modes. Its a bit ugly. I'd be happy to refactor that if there are some ideas, or maybe move some of it into our arrowbox bindings... of course then we have to make the awesomescreen an arrowbox... grr...

Comment 7

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

Comment on attachment 556081 [details] [diff] [review]
Patch v1

>diff --git a/mobile/chrome/content/AwesomePanel.js b/mobile/chrome/content/AwesomePanel.js

>+  get _targets() {
>+    delete this._targets;
>+    return this._targets = [this.container,
>+#ifdef MOZ_SERVICES_SYNC
>+                            document.getElementById("syncsetup-container"),
>+#endif
>+                            document.getElementById("urlbar-container"),
>+                            document.getElementById("search-engines-popup")]

This was hard to grok at first. Maybe like this:

    return this._targets = [
      this.container,
#ifdef MOZ_SERVICES_SYNC
      document.getElementById("syncsetup-container"),
#endif
      document.getElementById("urlbar-container"),
      document.getElementById("search-engines-popup")
    ];

>+
>+  handleEvent: function(aEvent) {

>+        while(target && this._targets.indexOf(target) == -1) {
>+          target = target.parentNode;
>+        }
>+        Util.dumpLn("T: " + target);

* while (
* no {}
* remove the dumpLn

>+  set activePanel(aPanel) {

>+    if (willShowPanel) {
<snip>
>+    };

* no semi-colon

>diff --git a/mobile/chrome/content/Util.js b/mobile/chrome/content/Util.js
>+  LOCALE_DIR_RTL: -1,
>+  LOCALE_DIR_LTR: 1,
>+  get localeDir() {
>+    // determine browser dir first to know which direction to snap to
>+    let chromeReg = Cc["@mozilla.org/chrome/chrome-registry;1"].
>+                      getService(Ci.nsIXULChromeRegistry);

No wrap needed

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>         // Check if the sidebars are inverted (rtl)
>-        let direction = (tabsSidebar.left > controlsSidebar.left) ? 1 : -1;
>+        let direction = -1*Util.localeDir;

-1 * Util.localeDir

>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

>+      if (aIndex == aArray.length-2 && aArray.length % 2 == 0)
>+        button.classList.add("odd-last-child");

aArray.length - 2

* no way to do this via fancy CSS selector?

>+      if (Util.localeDir > 0) {
>+        if (left + width > window.innerWidth)
>+          left = window.innerWidth - width;
>+      } else {
>+        left = searchButtonRect.right - width;
>+      }
>+      popup.left = left;
>+    } else {
>+      popup.left = 0;
>+    }

not easier in CSS I assume?

>diff --git a/mobile/themes/core/honeycomb/browser.css b/mobile/themes/core/honeycomb/browser.css

>+  #search-engines-list {
>+    padding: @padding_normal@ !important;
>+    max-width: -moz-calc(4 * @touch_button_xlarge@ + 2 * @padding_normal@);

* using the calc is good? or does it make sense to just make a new constant?

>+  #search-engines-list > .action-button {
>+    border-radius: 0px;
>+    min-width: -moz-calc(2 * @touch_button_xlarge@);
>+    max-width: -moz-calc(2 * @touch_button_xlarge@);
>+    width: -moz-calc(2 * @touch_button_xlarge@);

* using the calc is good? or does it make sense to just make a new constant?

>diff --git a/mobile/chrome/content/browser.xul b/mobile/chrome/content/browser.xul

>-    <arrowbox id="search-engines-popup" hidden="true" offset="18" flex="1">
>-      <hbox id="search-engines-list" class="action-buttons" flex="1"/>
>+    <arrowbox id="search-engines-popup" hidden="true" offset="18">
>+      <hbox id="search-engines-list" class="action-buttons"/>

Is the flex not needed for gingerbread/froyo?


r+, but fix nits and answer the questions

Comment 8

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> * no way to do this via fancy CSS selector?
There is! Fixed.

> >+      if (Util.localeDir > 0) {
> >+        if (left + width > window.innerWidth)
> >+          left = window.innerWidth - width;
> >+      } else {
> >+        left = searchButtonRect.right - width;
> >+      }
> >+      popup.left = left;
> >+    } else {
> >+      popup.left = 0;
> >+    }
> 
> not easier in CSS I assume?
I'm not really sure how to do this in CSS. I've tried using popup.start to avoid the rtl locales problems, but it doesn't seem to work (yet?).

> * using the calc is good? or does it make sense to just make a new constant?
Its hard to argue there won't be a small performance hit, but I would guess its very negligible. I think this looks nice too and would make it easy to adjust if we want to make this spread to n elements wide someday.

> Is the flex not needed for gingerbread/froyo?
It was! And removing it wasn't needed here! Thanks. I added a few other tweaks to make things work better on gingerbread/froyo themes.

Comment 9

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c2c5d46e9c39

Comment 10

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c2c5d46e9c39

Comment 11

[Aaron Train [:aaronmt]]

Samsung Galaxy Tab 10.1
Mozilla/5.0 (Android, Linux armv7l; rv:9.0a1) Gecko/20110906 Firefox/9.0a1 Fennec/9.0a1