Open search engines awesome screen row take a lot of space

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Bug Flags:
in-testsuite ?
in-litmus +

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

Created attachment 469010 [details] [diff] [review]
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
Created attachment 469012 [details]
Screenshoot not displayed
Created attachment 469013 [details]
Screenshoot displayed
This bug is part of Fennec 2 work to maximize the awesomescreen, especially when using a virtual keyboard.
added link to design page
Created attachment 469444 [details] [diff] [review]
Patch v0.2

This patch depends on bug 590779 for the urlbar-state observer
Attachment #469010 - Attachment is obsolete: true
Attachment #469444 - Flags: review?(mark.finkle)
Created attachment 469470 [details] [diff] [review]
Patch v0.2 - rebased

Patch rebased on the new patch in bug 590779
Assignee: nobody → 21
Attachment #469444 - Attachment is obsolete: true
Attachment #469470 - Flags: review?(mark.finkle)
Attachment #469444 - Flags: review?(mark.finkle)
Depends on: 590779
Created attachment 469476 [details] [diff] [review]
Patch v0.3

Sorry for the spam the previous patch was a failure.
Attachment #469470 - Attachment is obsolete: true
Attachment #469476 - Flags: review?(mark.finkle)
Attachment #469470 - Flags: review?(mark.finkle)
tracking-fennec: --- → 2.0+
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
Attachment #469476 - Flags: review?(mark.finkle) → review+
(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
http://hg.mozilla.org/mobile-browser/rev/f0f2f91b0ed8
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Created attachment 469690 [details] [diff] [review]
bustage-fix

I've introduce a crazy string! Sorry for that.
Attachment #469690 - Flags: review?(mbrubeck)
Attachment #469690 - Flags: review?(mbrubeck) → review+
(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)
Flags: in-testsuite?
Flags: in-litmus?
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
Status: RESOLVED → VERIFIED
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
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.