Closed Bug 1101122 Opened 10 years ago Closed 9 years ago

Update about:home search styling

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 + verified

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(5 files, 6 obsolete files)

      No description provided.
Attached patch patch (obsolete) — Splinter Review
Going to need some tinkering once we have the icon but this is functional. Also adds the popup by adding a panel to browser.xul that a message from content makes us open. Using a CPOW is the simplest, can probably clean up with co-ordinate passing later.
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Attachment #8524911 - Flags: review?(felipc)
Attached patch patch (obsolete) — Splinter Review
This includes the new icon and is functional. Apparently we want to move the magnifying glass inside the search box though so waiting on mockups for that before I can do anymore. The main code should be reviewable though.
Attachment #8524911 - Attachment is obsolete: true
Attachment #8524911 - Flags: review?(felipc)
Attachment #8524950 - Flags: review?(felipc)
Comment on attachment 8524911 [details] [diff] [review]
patch

Review of attachment 8524911 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.xul
@@ +237,5 @@
>             mousethrough="always">
>        <box id="UITourHighlight"></box>
>      </panel>
>  
> +    <panel id="abouthome-search-panel" orient="vertical" type="arrow">

cool, I guess it would be nice for me to use this same panel in the about:newtab work and remove the duplicated panel defined there. I'll see about doing this after I finish the other tasks

::: browser/modules/AboutHome.jsm
@@ +207,5 @@
> +
> +      case "AboutHome:OpenSearchPanel":
> +        let panel = window.document.getElementById("abouthome-search-panel");
> +        let anchor = aMessage.objects.anchor;
> +        panel.openPopup(anchor);

I think the CPOW won't work in real e10s, because openPopup is not JS-implemented. This should be enough for non-e10s users but I believe not for Nightly.
Attachment #8524911 - Attachment is obsolete: false
Comment on attachment 8524911 [details] [diff] [review]
patch

(fixing mid-air)
Attachment #8524911 - Attachment is obsolete: true
Comment on attachment 8524950 [details] [diff] [review]
patch

Review of attachment 8524950 [details] [diff] [review]:
-----------------------------------------------------------------

Only question is about the CPOW mentioned in comment 3. Not sure if it's important to handle that now or not.
Attachment #8524950 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes from comment #3)
> Comment on attachment 8524911 [details] [diff] [review]
> patch
> 
> Review of attachment 8524911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.xul
> @@ +237,5 @@
> >             mousethrough="always">
> >        <box id="UITourHighlight"></box>
> >      </panel>
> >  
> > +    <panel id="abouthome-search-panel" orient="vertical" type="arrow">
> 
> cool, I guess it would be nice for me to use this same panel in the
> about:newtab work and remove the duplicated panel defined there. I'll see
> about doing this after I finish the other tasks

Maybe? I don't think it's strictly necessary. Though if we do we should probably rename it.

> ::: browser/modules/AboutHome.jsm
> @@ +207,5 @@
> > +
> > +      case "AboutHome:OpenSearchPanel":
> > +        let panel = window.document.getElementById("abouthome-search-panel");
> > +        let anchor = aMessage.objects.anchor;
> > +        panel.openPopup(anchor);
> 
> I think the CPOW won't work in real e10s, because openPopup is not
> JS-implemented. This should be enough for non-e10s users but I believe not
> for Nightly.

That is a good point. I'll try to figure out what our other options are.
I've filed bug 1101269 to track being able to anchor a panel to a child element. I don't think we'll get it in time for this though so we may just disable the panel when in e10s.
Attached patch patch (obsolete) — Splinter Review
Updated with the new styling tweaks
Attachment #8524950 - Attachment is obsolete: true
Attachment #8525545 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
Fixes the test
Attachment #8525545 - Attachment is obsolete: true
Attachment #8525547 - Flags: review+
Group: mozilla-employee-confidential
Attached patch patch (obsolete) — Splinter Review
This updates the patch to just move the icons to the shared directory as they are all the same across platforms.
Attachment #8525547 - Attachment is obsolete: true
Attachment #8525674 - Flags: review+
The search settings part of this patch depends on bug 1088660
Depends on: fx34-searchui
Comment on attachment 8525674 [details] [diff] [review]
patch

Review of attachment 8525674 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/jar.mn
@@ +40,5 @@
>          skin/classic/browser/keyhole-forward-mask.svg
>          skin/classic/browser/KUI-background.png
>          skin/classic/browser/livemark-folder.png
> +        skin/classic/browser/magnifier.png                          (../shared/magnifier.png)
> +        skin/classic/browser/magnifier@2px.png                      (../shared/magnifier@2px.png)

just remembered that windows' jar.mn has two sections where files have to be listed
Attached patch beta patchSplinter Review
Right. Fixed.
Attachment #8525674 - Attachment is obsolete: true
Attachment #8525693 - Flags: review+
Gavin asked to to take a peek at this and other bugs.

Generally looks fine, although similar question as bug 1101147 comment 12 -- any plan to eventually use the new bug 1088660 search UI here? You can't currently change the search engine directly in about:home, but it would seem nice to have consistent UX for performing a search but with a one-off different engine.
Comment on attachment 8525693 [details] [diff] [review]
beta patch

Review of attachment 8525693 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.css
@@ +1222,5 @@
> +  -moz-padding-start: 0;
> +  -moz-padding-end: 0;
> +  padding-top: 0;
> +  padding-bottom: 0;
> +  background: rgb(248, 250, 251);

Nano nit: the background color here differs slightly from the color of the panel's arrow, at least on OS X.
(In reply to Justin Dolske [:Dolske] from comment #14)
> Gavin asked to to take a peek at this and other bugs.
> 
> Generally looks fine, although similar question as bug 1101147 comment 12 --
> any plan to eventually use the new bug 1088660 search UI here? You can't
> currently change the search engine directly in about:home, but it would seem
> nice to have consistent UX for performing a search but with a one-off
> different engine.

This patch does add a way to get to the search poreferences to change the default search engine. I agree though it would be nice to have a way to do one-off searches here.
Depends on: 873923
Attached patch naming fix (obsolete) — Splinter Review
Because I'm a dummy
Attachment #8526480 - Flags: review?(dolske)
Comment on attachment 8526480 [details] [diff] [review]
naming fix

This has no functional impact, right? Can we move to a followup (post-34)?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #19)
> Comment on attachment 8526480 [details] [diff] [review]
> naming fix
> 
> This has no functional impact, right? Can we move to a followup (post-34)?

It doesn't have to land on 34 no
Attached patch trunk patchSplinter Review
This patch applies to trunk and everything works in non-e10s windows. The only change was to support in-content prefs, we have to manually hide the popup when it is clicked and the test has to detect a new tab rather than a new window.

This patch also includes the file renames.
Attachment #8526480 - Attachment is obsolete: true
Attachment #8526480 - Flags: review?(dolske)
Attachment #8526909 - Flags: review?(felipc)
Attachment #8525693 - Attachment description: patch → beta patch
Comment on attachment 8526909 [details] [diff] [review]
trunk patch

Review of attachment 8526909 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_aboutHome.js
@@ +628,5 @@
> +  let tab = event.target;
> +  yield promiseTabLoadEvent(tab);
> +  is(tab.linkedBrowser.currentURI.spec, "about:preferences#search", "Should have seen the prefs tab");
> +  gBrowser.removeTab(tab);
> +});

Nice, this will be useful for my test too!

::: browser/modules/AboutHome.jsm
@@ +207,5 @@
> +
> +      case "AboutHome:OpenSearchPanel":
> +        let panel = window.document.getElementById("abouthome-search-panel");
> +        let anchor = aMessage.objects.anchor;
> +        panel.openPopup(anchor);

just check if this will simply fail, and not cause a crash in e10s
Attachment #8526909 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (behind on reviews until the end of the week) from comment #22)
> ::: browser/modules/AboutHome.jsm
> @@ +207,5 @@
> > +
> > +      case "AboutHome:OpenSearchPanel":
> > +        let panel = window.document.getElementById("abouthome-search-panel");
> > +        let anchor = aMessage.objects.anchor;
> > +        panel.openPopup(anchor);
> 
> just check if this will simply fail, and not cause a crash in e10s

Yeah it just does nothing. Not even an error in the console.
Depends on: 1103127
Note that the trunk patch does not include the patch from bug 1103163 (might be needed on 35) or the potential patch in bug 1103127
Notes:
- this is basically your patch from before, with main changes in aboutHome.css and aboutHome.js, plus the attribute setting in browser/base/content/content.js

- Since the css between #searchLogoContainer and #searchIcon is significantly different, instead of using the same element, I kept both in the xhtml file, and hide one and unhide the other depending on the pref.

- I didn't notice any flickering with this approach. I tried both directions and it worked fine. I settled with going with the new UI by default, and replacing it with the old, because _if_ any flickering happens it's slightly better to first see the magnifying glass and then see the engine logo appear, than the opposite.

- I haven't tried the tests yet. I suppose they will work out of the box because we had just removed testcases that were not relevant. Ideally I would want to do the same as I did with newtab: copy the old test file and toggle the pref, so that we can continue testing the old UI too. But I'm not gonna block on that at the moment
Attachment #8527110 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8527110 [details] [diff] [review]
New patch for beta, flare UI conditioned to the oneOffButtons pref

Review of attachment 8527110 [details] [diff] [review]:
-----------------------------------------------------------------

A couple of comments here, I can take another look when the search bits are done.

::: browser/base/content/abouthome/aboutHome.css
@@ +88,5 @@
> +}
> +
> +#searchIcon[hidden] {
> +  display: none;
> +}

So rather than controlling the hidden state programmatically how about something like:

html[searchUIConfiguration="oldsearchui"] #searchIcon {
  display: none;
}

html:not([searchUIConfiguration="oldsearchui"]) #searchLogoContainer {
  display: none;
}

I think you can get away without the mutation listener for this part then.

::: browser/base/content/abouthome/aboutHome.js
@@ +166,5 @@
>          gInitialized = true;
>        }
>        return;
> +    } else if (mutation.attributeName == "searchUIConfiguration") {
> +      console.log("mutated!");

Debugging?

@@ +168,5 @@
>        return;
> +    } else if (mutation.attributeName == "searchUIConfiguration") {
> +      console.log("mutated!");
> +      let useOldSearchUI =
> +        "oldsearchui" == document.documentElement.getAttribute("searchUIConfiguration"); 

Nit, whitespace at end of line

::: browser/themes/windows/jar.mn
@@ +42,5 @@
>          skin/classic/browser/keyhole-forward-mask.svg
>          skin/classic/browser/KUI-background.png
>          skin/classic/browser/livemark-folder.png
> +        skin/classic/browser/magnifier.png                          (../shared/magnifier.png)
> +        skin/classic/browser/magnifier@2px.png                      (../shared/magnifier@2px.png)

Want to correct the icon name while you're here?
Attachment #8527110 - Flags: review?(dtownsend+bugmail)
New patch, using the CSS instead of mutation observer.
The only issue is that to hide the placeholder text, I had to use ::-moz-placeholder, and display:none doesn't seem to work with it, so I had to use color: transparent.

Fixed the other nits and ran the test, it passed.
Attachment #8527119 - Flags: review?(dtownsend+bugmail)
Attachment #8527119 - Flags: review?(dtownsend+bugmail) → review+
Since the search engine logo will be hidden, it would be nice to have a place where the user can see which search engine currently is the default. Perhaps one or more of the following:

(1) #abouthome-search-panel - create a second button with a label such as "Search using <current engine>" which dismisses the panel and focuses the input

(2) tooltip text such as "Search using <current engine>" (Search Bar will use a tooltip)

(3) placeholder text in the input such as "Search using <current engine>"
Comment on attachment 8527119 [details] [diff] [review]
Patch for beta, flare UI conditioned to the oneOffButtons pref

I wanted to land this patch soon because it contains the fix from bug 1103127. I said I was going to include the fix here in this new patch so we didn't land 1103127 by itself to avoid conflicts.
Attachment #8527119 - Attachment description: New patch for beta, flare UI conditioned to the oneOffButtons pref → Patch for beta, flare UI conditioned to the oneOffButtons pref
Attachment #8527119 - Flags: approval-mozilla-beta?
Attachment #8527119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed the new version with the switchable UI based on browser.search.showOneOffButtons

It also includes the patch from bug 1103127.

https://hg.mozilla.org/releases/mozilla-beta/rev/ca40bcdc68ae
https://hg.mozilla.org/releases/mozilla-beta/rev/27505b6035ef
QA Contact: petruta.rasa
The search field in about:home page only contains the magnifier glass with the "Manage Search Engines.." when clicked, the field is empty. Verified using Firefox 34.0 RC build (20141124205320) under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OSX 10.9.5.
https://hg.mozilla.org/mozilla-central/rev/f9bf11b69e62
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 36
browser_aboutHome.js exercises this aboutaccounts.js function, and closes the window too quickly before getSignedInUser() resolves, causing window.location to be null and the test failing due to the rejected promise
Attachment #8530473 - Flags: review?(bmcbride)
Attachment #8530473 - Flags: review?(bmcbride) → review+
Depends on: 1106238
Depends on: 1106239
Depends on: 1107471
Depends on: 1107494
No longer depends on: 1107494
Depends on: 1108254
Verified 35 and 36 versions - all platforms.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.