Closed Bug 1101122 Opened 7 years ago Closed 7 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.
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: 7 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.