Closed Bug 602671 Opened 9 years ago Closed 9 years ago

Create an Android theme for Site Menu / Open Search Menu

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: vingtetun, Assigned: mbrubeck)

References

()

Details

Attachments

(4 files, 4 obsolete files)

Assignee: nobody → wjohnston
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.
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.
tracking-fennec: --- → 2.0b2+
Assignee: wjohnston → mark.finkle
Attached patch wip (obsolete) — Splinter Review
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
Attached image example of an 'arrow' image (obsolete) —
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.
We also need the unlocked image and the other two states for the larry image
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #483467 - Attachment is obsolete: true
Attachment #483704 - Flags: review?(mbrubeck)
Attached image screentshot
screenshot of current patch
Attachment #483470 - Attachment is obsolete: true
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.
Attachment #483704 - Flags: review?(mbrubeck) → review+
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
Style development happening for other popups over in bug 602674. That work should be used here (for search menu).
Assignee: mark.finkle → mbrubeck
Status: NEW → ASSIGNED
Attachment #484508 - Flags: review?(mark.finkle)
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 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
Attachment #484508 - Flags: review?(mark.finkle) → review+
Pushed with nits fixed, and some other minor changes discussed on IRC:
http://hg.mozilla.org/mobile-browser/rev/1b0569deb49a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 575403
Attached patch fixup (obsolete) — Splinter Review
This fixes a problem with the layout in landscape orientation.
Attachment #483704 - Attachment is obsolete: true
Attachment #484555 - Flags: review?(mark.finkle)
Attached patch fixup v2Splinter Review
Vivien suggested adding padding between the button icon and label.
Attachment #484555 - Attachment is obsolete: true
Attachment #484557 - Flags: review?(mark.finkle)
Attachment #484555 - Flags: review?(mark.finkle)
Attachment #484557 - Flags: review?(mark.finkle) → review+
Waiting for tomorrow's build to verify since bug 575950 has landed today
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?(aaron.train)
Flags: in-litmus?(aaron.train) → in-litmus+
You need to log in before you can comment on or make changes to this bug.