590476 - Open search engines awesome screen row take a lot of space

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Patch

The patch just move the open search engines to a popup that you can see by clicking on the favicon button on the top left side (when the awesome screen is dislayed).

This is just a list that search the current terms entered into the urlbar for now (it does not switch the "All Pages" view)

For an idea of the look: see screenshots

Comment 1

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

Attachment: Screenshoot not displayed

Comment 2

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

Attachment: Screenshoot displayed

Comment 3

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

This bug is part of Fennec 2 work to maximize the awesomescreen, especially when using a virtual keyboard.

Comment 4

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

added link to design page

Comment 5

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

Attachment: Patch v0.2

This patch depends on bug 590779 for the urlbar-state observer

Comment 6

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

Attachment: Patch v0.2 - rebased

Patch rebased on the new patch in bug 590779

Comment 7

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

Attachment: Patch v0.3

Sorry for the spam the previous patch was a failure.

Comment 8

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

Comment on attachment 469476 [details] [diff] [review]
Patch v0.3


>   handleIdentityButtonEvent: function(aEvent) {
>     aEvent.stopPropagation();
> 
>+    if (Elements.urlbarState.getAttribute("mode") == "edit") {
>+      CommandUpdater.doCommand("cmd_opensearch");
>+      return;
>+    }
>+

Add a comment as to why "edit" matters

>+                      <box id="urlbar-image-box" mousethrough="always" observes="bcast_urlbarState">
>                         <image id="urlbar-throbber"/>
>+                        <image id="urlbar-magnifier"/>
>                         <image id="urlbar-favicon" hidden="true"/>

Should urlbar-magnifier be hidden="true" too?

>       <vbox id="menulist-popup" class="dialog-dark">
>+        <label id="menulist-title" crop="center"/>

I think you need a flex="1" here to make the crop="center" work

>+#urlbar-magnifier {
>+  list-style-image: url("chrome://browser/skin/images/navigation-magnifier-30.png");
>+  display: none;

Ah, I see you have it here. I wonder if having hidden=true" in the XUL has any Ts impact?

r+ with the nits answered

Comment 9

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

(In reply to comment #8)
> >+                      <box id="urlbar-image-box" mousethrough="always" observes="bcast_urlbarState">
> >                         <image id="urlbar-throbber"/>
> >+                        <image id="urlbar-magnifier"/>
> >                         <image id="urlbar-favicon" hidden="true"/>
> 
> Should urlbar-magnifier be hidden="true" too?

> 
> >+#urlbar-magnifier {
> >+  list-style-image: url("chrome://browser/skin/images/navigation-magnifier-30.png");
> >+  display: none;
> 
> Ah, I see you have it here. I wonder if having hidden=true" in the XUL has any
> Ts impact?
> 
> r+ with the nits answered

I guess hidden=true vs display: none has the same impact and I don't think this has a big impact for this specific case

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#36

Comment 10

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

http://hg.mozilla.org/mobile-browser/rev/f0f2f91b0ed8

Comment 11

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

Attachment: bustage-fix

I've introduce a crazy string! Sorry for that.

Comment 12

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

(In reply to comment #11)
> Created attachment 469690 [details] [diff] [review]
> bustage-fix
> 
> I've introduce a crazy string! Sorry for that.

http://hg.mozilla.org/mobile-browser/rev/24af494a34e4

(Thanks for the quick review Matt)

Comment 13

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100830 Namoroka/4.0b5pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100830 Namoroka/4.0b5pre Fennec/2.0a1pre

Comment 14

[Aakash Desai [:aakashd]]

litmus testcases updated/added to regression test this bug:

https://litmus.mozilla.org/show_test.cgi?id=12141
https://litmus.mozilla.org/show_test.cgi?id=12169
https://litmus.mozilla.org/show_test.cgi?id=12248
https://litmus.mozilla.org/show_test.cgi?id=12245
https://litmus.mozilla.org/show_test.cgi?id=12185
https://litmus.mozilla.org/show_test.cgi?id=12122
https://litmus.mozilla.org/show_test.cgi?id=12695