602671 - Create an Android theme for Site Menu / Open Search Menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

See attached url.

Comment 1

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

See http://mozilla.seanmartell.com/fennec/fimo/androidUI/index.html for a CSS impl.

Since we want to control the arrow location, we might need to create a <arrowbox> XBL binding that gives use some interactive control and scripting support.

Comment 2

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

We could also start playing with the new <arrowpanel> support in m-c. I am concerned about the performance of opening panels on devices, but it would be nice to be able to use <panel>-based elements someday, for this and other things.

Comment 3

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

Attachment: wip

This patch adds a <arrowbox> binding and starts using it for the Site Menu.

Needs:
* better "arrow" images
* final styling of the arrowbox itself
* add support for the search providers and the edit bookmark popup and maybe the new tab opened popup

Comment 4

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

Attachment: example of an 'arrow' image

Example of the arrow-up image. Size is 32x24 but the arrow border does not extend over the entire area. We need some space to overlay the main box.

Comment 5

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

We also need the unlocked image and the other two states for the larry image

Comment 6

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

Attachment: patch

This patch adds support for arrowboxes and uses an arrowbox for the identity popup
* images come from martell
* fixes "larry" image bustage in the popup
* offsets the arrow popup as in the mockups

Still needed:
* we are missing the 2 other "larry" states
* we need to restyle the pageaction buttons

Given the current busted state of the identity panel, we should land this partially complete patch and finish the remaining bits ASAP.

Technically, the other two "larry" states are busted right now anyway - waiting for a beta 2 fix as well.

Comment 7

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

Attachment: screentshot

screenshot of current patch

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 483704 [details] [diff] [review]
patch

>+++ b/chrome/content/bindings/arrowbox.xml

I know this isn't your code originally; you can leave the following things unfixed if you want to stay closer to the original for easy merging of future changes.

>+              let anchor = aAnchorNode;

No need for this local variable.

>+              let offset = parseInt(this.getAttribute("offset")) || 0;

Should just use "let offset = this.offset;" instead of duplicating code from the getter.

>+              let hideAnchor = false;

How about renaming this to "hideArrow"?

>+              if (horizPos == 0) {
>                 // ...
>+              }
>+              else if (vertPos == 0) {
>+                container.orient = "";
>+                arrowbox.orient = "vertical";
>+                if (horizPos == 0) {
>+                  hideAnchor = true;
>+                }

The second "if (horizPos == 0)" will never be true; you can remove it.

>+                else {

elses are not cuddled in this toolkit code.  They want to feel loved.

Comment 9

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

made all review changes
pushed:
http://hg.mozilla.org/mobile-browser/rev/cdb24af322a8

I'm holding off calling this fixed until Madhava has a chance to provide any small tweaks for beta 2

Comment 10

[Madhava Enros [:madhava]]

Style development happening for other popups over in bug 602674. That work should be used here (for search menu).

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Search engines popup

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: search engines screenshot

When the pressed state for the endcap button is ready (bug 605628) we should also make sure to use that while the popup is visible.

Comment 13

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

Comment on attachment 484508 [details] [diff] [review]
part 2: Search engines popup

>diff -r f604d63e84b9 chrome/content/browser-ui.js

>+      case "cmd_opensearch": {
>         this.blurFocusedElement();
>-
>-        MenuListHelperUI.show({
>-          title: Elements.browserBundle.getString("opensearch.searchWith"),
>-          menupopup: { children: BrowserSearch.engines },
>-          set selectedIndex(aIndex) {
>-            let name = this.menupopup.children[aIndex].label;
>-            BrowserUI.doOpenSearch(name);
>-          }
>-        });
>+        BrowserSearch.toggle();
>         break;
>+      }

Do you need the {} around the case? Remove them if not needed

>diff -r f604d63e84b9 chrome/content/browser.xul

>+    <arrowbox id="search-engines-popup" hidden="true" align="center" offset="18" flex="1" top="50">
>+      <hbox id="search-engines-list" class="prompt-buttons"/>
>+    </arrowbox>
>+

Is hard coding the "top" a wise thing to do? Can we set that in code in .show() ? Oh, just looked and you do set it in the code. Why default to 50?

r+ with nits

Comment 14

[Matt Brubeck (:mbrubeck)]

Pushed with nits fixed, and some other minor changes discussed on IRC:
http://hg.mozilla.org/mobile-browser/rev/1b0569deb49a

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: fixup

This fixes a problem with the layout in landscape orientation.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: fixup v2

Vivien suggested adding padding between the button icon and label.

Comment 17

[Matt Brubeck (:mbrubeck)]

Pushed fixup: http://hg.mozilla.org/mobile-browser/rev/5b238fbdf2dc

Comment 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Waiting for tomorrow's build to verify since bug 575950 has landed today

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (maemol Linux armv7l; rv:2.0b7pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2

Mozilla/5.0 (android Linux armv7l; rv:2.0b7pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2

Adding AaronMT to the in-litmus? flag. I don't think any testcases are needed here, but it would be good to double check in our Search BFTs.