Closed Bug 680212 Opened 8 years ago Closed 8 years ago

Make fennecomb awesomebar pretty

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 8
All
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached file 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 :)
Attached patch WIP (obsolete) — Splinter Review
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).
Assignee: nobody → wjohnston
Attached file menu inner glow
Thanks for posting this Wes, my comments are included here: http://cl.ly/133q0m1C383t2U063w3N
Blocks: 655762
OS: Mac OS X → Android
Hardware: x86 → All
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.
"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
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #554587 - Attachment is obsolete: true
Attached patch Patch v1Splinter Review
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...
Attachment #555921 - Attachment is obsolete: true
Attachment #556081 - Flags: review?(mark.finkle)
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
Attachment #556081 - Flags: review?(mark.finkle) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/c2c5d46e9c39
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Samsung Galaxy Tab 10.1
Mozilla/5.0 (Android, Linux armv7l; rv:9.0a1) Gecko/20110906 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
Depends on: 684860
Depends on: 684714
Depends on: 735749
You need to log in before you can comment on or make changes to this bug.