1029792 - [Search experiment] Implement notification UI

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

This bug is about implementing the mockup from bug 1029189 and showing it for users of cells 3 and 5 from the experiment. If the mockup gets too complicated this could be broken down further, but it's hard to know at the moment if that's necessary.

Comment 1

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

Mano/Marco, do one of you want to pick this up? There's a preliminary mockup in bug 1029189 that could use some implementation feedback (it looks a bit annoying to implement to me).

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Mano, can you please pick this up? I talked to Marco and he is not able to work on it this week unfortunately. This is quite high priority and we would need something until the end of the week so please let me know if you think you can't do it for whatever reason :)

Comment 3

[Marco Mucci [:MarcoM]]

Hi Mano, can you provide a point estimate and if the bug should be marked as [qa+] or [qa-] for verification.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Mano, I think hooking the notification up to the groups will need to wait for a patch in bug 1029776 but getting the notification itself working and built should be good for a start.

Comment 5

[Asaf Romano (gone)]

Hrm, it looks like the code for this would have to live in the new addon, potentially making the implementation more complex than I imagined.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Mano: Since the code needs to live in the addon, and since I've been working on the addon already, would you like me to pick up this bug instead?

Comment 7

[Asaf Romano (gone)]

No problem at all...

Comment 8

[Marco Mucci [:MarcoM]]

Hi Blake, can you provide a point estimate and if the bug should be marked as [qa+] or [qa-] for verification.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: dropdownnote.js

I've got an initial implementation of the UI here. It's not hooked up to the add-on, just the code necessary to display the notification on the bottom of the search popup.

To test it:
 - set devtools.chrome.enabled = true
 - open Scratchpad and load this file
 - Set Environment > Browser from the menu.
 - Run it

You can make modifications to this code and just run it again to replace with the latest changes.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Wrong popup height

One thing that worries me is that adding this box to the footer of the search popup is sometimes throwing the calculation of the popup height to the wrong direction. I don't have yet a good understanding of why it happens or how to fix it.

The screenshot shows an example of it. It only occurs sporadically. And typing the same thing again doesn't reproduce the problem.. So it looks like a timing issue with the height calculation.


Also, another detail: the box added to the bottom doesn't simply add to the height of the popup. But it doesn't remain the same size either. It seems it grows by a certain amount until it reaches a max-height.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

CC'ing Dao and Neil for help here, specially on comment 10

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Felipe Gomes from comment #10)
> Created attachment 8446905 [details]
> Wrong popup height
> 
> One thing that worries me is that adding this box to the footer of the
> search popup is sometimes throwing the calculation of the popup height to
> the wrong direction. I don't have yet a good understanding of why it happens
> or how to fix it.

I can't explain this offhand, but the fact that something unexpected happens doesn't surprise me, since I don't think autocomplete popups are designed to deal with arbitrary content getting injected.

Overloading the autocomplete popup doesn't seem ideal to me from a user's perspective. E.g. if you type something distinctly enough, the popup may not show up. I for one tend to ignore these suggestions; often I would just paste something and submit it immediately.

I would suggest displaying an info bubble (arrow panel) on startup or when the search bar is focused. This also has the benefit of pointing to the dropdown where the engine can be changed. It seems like this was initially mocked up in attachment 8444758 [details]. Not sure why a different route was taken later.

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: dropdownnote.js styled

Here's a version with some minor styling changes.
Would it be possible to show a little more of the suggestions (moving the notification down by about 100px and making the whole popup longer – at least as long as it fits on the screen)? I tried but couldn't figure it out.

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Notification showing up in web forms

There seems to be something wrong with the selector of PopupAutoComplete, because the notification also shows up in web forms.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to Philipp Sackl [:phlsa] from comment #14)
> Created attachment 8447079 [details]
> Notification showing up in web forms
> 
> There seems to be something wrong with the selector of PopupAutoComplete,
> because the notification also shows up in web forms.

Yeah this happens because the same popup element is used for webforms and content. It will be hidden by default and the add-on will unhide it only when used from the searchbar

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Given the evidence that this will be less-than-trivial, we're gonna split this bug further and Blake and I will each work in one part.

Comment 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Dão Gottwald [:dao] from comment #12)
> Overloading the autocomplete popup doesn't seem ideal to me from a user's
> perspective. E.g. if you type something distinctly enough, the popup may not
> show up. I for one tend to ignore these suggestions; often I would just
> paste something and submit it immediately.
> 
> I would suggest displaying an info bubble (arrow panel) on startup or when
> the search bar is focused. This also has the benefit of pointing to the
> dropdown where the engine can be changed. It seems like this was initially
> mocked up in attachment 8444758 [details]. Not sure why a different route
> was taken later.

The reason we decided against the bubble in the end is that it would either be shown at a moment where it is not relevant (on startup) or it would force you to act on it when focusing the search field because it would interfere with the suggestion dropdown.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Neil, can you help here? I think I discovered the reason of the problem mentioned in comment 10.

What I'm doing here is basically document.getElementById("PopupAutoComplete").appendChild(<hbox>)   (where this hbox is the footer notification that we want to display here).

The problem is that this added hbox is making the tree height attribute (calculated  by adjustHeight) not be respected. For example, if the height is calculated as 180, and my hbox has itself a height of 60, the whole popup is not ending up with a height of 240. Rather, the tree shrinks a bit and becomes smaller than 180.

Do you know why does that happen, and more importantly, how can I solve that? Note that this is intended for add-on code so it needs to be something live-fixable.


If I monkey-patch the adjustHeight() function, and make it set min-height on the tree (rather than just height), everything works correctly.  I'm using this following snippet:

> p = document.getElementById("PopupAutoComplete");
> p._oldAdjustHeight = p.adjustHeight;
> p.adjustHeight = function() {
>   p._oldAdjustHeight();
>   let height = p.tree.getAttribute("height");
>   p.tree.style.minHeight = height + "px";
> }

Comment 19

[Neil Deakin]

The issue is that the height of block elements don't get recomputed properly in certain cases. This affects the long label you have added (The label with "This version of...")

You can either set an explicit height/minheight on the label, or you could adjust the size of the content after you've inserted the content:

  var search = document.getElementById("search-experiment-notification");
  search.minHeight = search.getBoundingClientRect().height;

You'll want to do this after inserting to handle when the popup is closed and during popupshowing to handle the case if the popup is already open, since you need to adjust it when something would trigger the popup's size to be recomputed. Or you might only need to handle just one of those, since you didn't specify when you're calling the code you attached.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Thanks for the explanation. It sounds like the previous solution was pretty close to what would be necessary (setting a minHeight on every size change), so I kept the snippet I had before as I had already tested it a lot and I didn't notice any problems.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Notification UI was QA'ed in bug 1029818