Closed Bug 1029792 Opened 6 years ago Closed 6 years ago

[Search experiment] Implement notification UI

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
8

Tracking

()

RESOLVED FIXED
Iteration:
33.2

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(4 files)

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.
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).
Flags: firefox-backlog+
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 :)
Flags: needinfo?(mano)
Assignee: nobody → mano
Status: NEW → ASSIGNED
Flags: needinfo?(mano)
Hi Mano, can you provide a point estimate and if the bug should be marked as [qa+] or [qa-] for verification.
Iteration: --- → 33.2
QA Whiteboard: [qa?]
Flags: needinfo?(mano)
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.
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.
Flags: needinfo?(mano)
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?
Flags: needinfo?(mano)
No problem at all...
Assignee: mano → bwinton
Flags: needinfo?(mano)
Hi Blake, can you provide a point estimate and if the bug should be marked as [qa+] or [qa-] for verification.
Flags: needinfo?(bwinton)
Attached file 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.
Attached image 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.
CC'ing Dao and Neil for help here, specially on comment 10
Points: --- → 8
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(bwinton)
(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.
Attached file 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.
There seems to be something wrong with the selector of PopupAutoComplete, because the notification also shows up in web forms.
(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
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.
Assignee: bwinton → felipc
Summary: [Search experiment] Implement notification UI and display UI for the correct groups → [Search experiment] Implement notification UI
Blocks: 1031320
(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.
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";
> }
Flags: needinfo?(enndeakin)
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.
Flags: needinfo?(enndeakin)
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Notification UI was QA'ed in bug 1029818
QA Whiteboard: [qa+] → [qa-]
You need to log in before you can comment on or make changes to this bug.