If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Create an Android theme for Site Menu / Open Search Menu

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: vingtetun, Assigned: mbrubeck)

Tracking

Bug Flags:
in-litmus +

Details

(URL)

Attachments

(4 attachments, 4 obsolete attachments)

See attached url.
No longer depends on: 575403
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
Created attachment 483467 [details] [diff] [review]
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
Created attachment 483470 [details]
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.
We also need the unlocked image and the other two states for the larry image
Created attachment 483704 [details] [diff] [review]
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.
Attachment #483467 - Attachment is obsolete: true
Attachment #483704 - Flags: review?(mbrubeck)
Created attachment 483705 [details]
screentshot

screenshot of current patch
Attachment #483470 - Attachment is obsolete: true
(Assignee)

Comment 8

7 years ago
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)

Comment 11

7 years ago
Created attachment 484508 [details] [diff] [review]
part 2: Search engines popup
Assignee: mark.finkle → mbrubeck
Status: NEW → ASSIGNED
Attachment #484508 - Flags: review?(mark.finkle)
(Assignee)

Comment 12

7 years ago
Created attachment 484509 [details]
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 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+
(Assignee)

Comment 14

7 years ago
Pushed with nits fixed, and some other minor changes discussed on IRC:
http://hg.mozilla.org/mobile-browser/rev/1b0569deb49a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 575403
(Assignee)

Comment 15

7 years ago
Created attachment 484555 [details] [diff] [review]
fixup

This fixes a problem with the layout in landscape orientation.
Attachment #483704 - Attachment is obsolete: true
Attachment #484555 - Flags: review?(mark.finkle)
(Assignee)

Comment 16

7 years ago
Created attachment 484557 [details] [diff] [review]
fixup v2

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+
(Assignee)

Comment 17

7 years ago
Pushed fixup: http://hg.mozilla.org/mobile-browser/rev/5b238fbdf2dc
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)

Updated

7 years ago
Flags: in-litmus?(aaron.train) → in-litmus+
You need to log in before you can comment on or make changes to this bug.