1101122 - Update about:home search styling

Status: VERIFIED FIXED

(Firefox :: General, defect)

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

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.

Comment 2

[Dave Townsend [:mossop]]

Attachment: patch

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.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8524911 [details] [diff] [review]
patch

(fixing mid-air)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 6

[Dave Townsend [:mossop]]

(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.

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch

Updated with the new styling tweaks

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch

Fixes the test

Comment 10

[Dave Townsend [:mossop]]

Attachment: patch

This updates the patch to just move the icons to the shared directory as they are all the same across platforms.

Comment 11

[Dave Townsend [:mossop]]

The search settings part of this patch depends on bug 1088660

Comment 12

[:Felipe Gomes (needinfo for replies!)]

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/[e-mail]                      (../shared/[e-mail])

just remembered that windows' jar.mn has two sections where files have to be listed

Comment 13

[Dave Townsend [:mossop]]

Attachment: beta patch

Right. Fixed.

Comment 14

[Justin Dolske [:Dolske]]

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 15

[Justin Dolske [:Dolske]]

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.

Comment 16

[Dave Townsend [:mossop]]

(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.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f76f2fa5be8

Comment 18

[Dave Townsend [:mossop]]

Attachment: naming fix

Because I'm a dummy

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8526480 [details] [diff] [review]
naming fix

This has no functional impact, right? Can we move to a followup (post-34)?

Comment 20

[Dave Townsend [:mossop]]

(In reply to :Gavin Sharp [email: [e-mail]] 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

Comment 21

[Dave Townsend [:mossop]]

Attachment: trunk patch

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.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

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

Comment 23

[Dave Townsend [:mossop]]

(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.

Comment 24

[Dave Townsend [:mossop]]

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

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: New patch for beta, flare UI conditioned to the oneOffButtons pref

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

Comment 26

[Dave Townsend [:mossop]]

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/[e-mail]                      (../shared/[e-mail])

Want to correct the icon name while you're here?

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch for beta, flare UI conditioned to the oneOffButtons pref

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.

Comment 28

[:Felipe Gomes (needinfo for replies!)]

waiting on try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fac78a138c01

Comment 29

[Jefferson]

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 30

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

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

Comment 32

[Petruta Horea [:phorea], Desktop Test Engineering]

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.

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Includes bug 1103194
https://hg.mozilla.org/integration/fx-team/rev/f9bf11b69e62

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9bf11b69e62

Comment 35

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/492f2a661cab

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix aboutaccounts.js

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

Comment 37

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4fb9368f8de1

Comment 38

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified 35 and 36 versions - all platforms.