830505 - Work - Re theme the autocomplete screen in snapped view

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect, P2)

Description

[Allison Naaktgeboren :ally]

from the user stories document, raised during metro work week. Have not investigated work needed yet

Comment 1

[Allison Naaktgeboren :ally]

nb: this is probably not so much creating a new one as theming the existing one to use smaller tiles and a different layout when in snapped.

Comment 2

[Asa Dotzler [:asa]]

Yuan, I've been thinking about this for a while and I think that our snapped view for auto-complete should not include search. A single column of auto-complete tiles seems more valuable than trying to split the minimal space between auto-complete and search. What do you think?

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I agree the auto-complete tiles are valuable when the users' input has available results to match, and more valuable than internet searches in this case.  However, Internet searches has its value of helping users to discover. It's particularly useful when the users' search input got no result to match. 

I don't think we need to worry about designing for limited space, I am positive that we can display these two features in one single column with smaller tiles/favicons. The screen size of snap view is similar with a phone screen. FX Android executed pretty well on auto-complete results. If we keep the consistency, we should be fine. 

I will post interaction specs on this feature during Iteration #3.

Comment 4

[Asa Dotzler [:asa]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #3)

> I don't think we need to worry about designing for limited space, I am
> positive that we can display these two features in one single column with
> smaller tiles/favicons.

OK. I'm cool with that too. Thanks.

Comment 5

[Marco Mucci [:MarcoM]]

Hi Yuan, can you add a relationship to the specification that Ally requires.  You can add the work item here:  838497 (if it doesn't already exist).

Comment 6

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to Marco Mucci [:MarcoM] from comment #5)
> Hi Yuan, can you add a relationship to the specification that Ally requires.
> You can add the work item here:  838497 (if it doesn't already exist).

Hi Marco,
I added Bug 846069 as a work item for myself on this feature. Since the new combined app bar also had impact on this feature, the design work is still being iterated, and awaiting for reviews from ux team. I will keep Ally with the progress.

Comment 7

[Allison Naaktgeboren :ally]

Attachment: wip-does not function

notes in comments

Comment 8

[Allison Naaktgeboren :ally]

Attachment: wip: works using forced refresh, notes in comments

this is functionally complete. Im not crazy about the force refresh part. or the descendant selectors.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 736561 [details] [diff] [review]
wip: works using forced refresh, notes in comments

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

::: browser/metro/base/content/bindings/autocomplete.xml
@@ +70,5 @@
> +    <!-- anaaktge
> +      seems to cause slightly odd layout in normal screen in searchs
> +column (ok, maybe I just _hate_ the layout logic on trunk)
> +      flexes are required
> +     -->

If the flexes are what's causing the weird behavior, you could apply them in CSS for the snapped view only (using "-moz-box-flex: 1;").

::: browser/metro/base/content/browser-ui.js
@@ +575,5 @@
> +          // works for orient(remove required), causes css to be applied right layout, test color
> +          // -moz-box, does not layout, but we *do* get the test color
> +          let autocomplete = document.getElementById("start-autocomplete");
> +          autocomplete.removeAttribute("orient");
> +          autocomplete.setAttribute("orient", "vertical");

Wow, nice find.  Perhaps we should file a follow-up bug on this.

::: browser/metro/base/content/browser.xul
@@ +225,5 @@
>                    </hbox>
>                  </box>
> +                <!-- Is this hte binding, the actual parent, what? I
> +                     thought this was a tree, and isnt this putting
> +                     cycles in the tree? -->

I'll give this a shot; hopefully I'm answering the right question.

When we have explicit content in a .xul file like this:

   <box id="start-autocomplete" observes="bcast_windowState"/>

...attached to an XBL binding that contains this:

    <content orient="horizontal">
      <xul:vbox id="results-vbox" class="meta-section viewable-height">
        <xul:label class="meta-section-title" value="&autocompleteResultsHeader.label;"/>
        <richgrid id="results-richgrid" anonid="results" seltype="single" flex="1"/>
      </xul:vbox>

      <xul:vbox id="searches-vbox" class="meta-section viewable-height">
        <xul:label class="meta-section-title" value="&autocompleteSearchesHeader.label;"/>
        <richgrid id="searches-richgrid" anonid="searches" seltype="single" flex="1"/>
      </xul:vbox>
    </content>

...then the stuff *inside* of the XBL <content> element gets added as anonymous children of the bound element, while its attributes are added to the bound element itself, so we end up with, I think, this:

   <box id="start-autocomplete" observes="bcast_windowState" orient="horizontal">
      <xul:vbox id="results-vbox" class="meta-section viewable-height">
        <xul:label class="meta-section-title" value="&autocompleteResultsHeader.label;"/>
        <richgrid id="results-richgrid" anonid="results" seltype="single" flex="1"/>
      </xul:vbox>

      <xul:vbox id="searches-vbox" class="meta-section viewable-height">
        <xul:label class="meta-section-title" value="&autocompleteSearchesHeader.label;"/>
        <richgrid id="searches-richgrid" anonid="searches" seltype="single" flex="1"/>
      </xul:vbox>
   </box>

...where the <box> is an explicit element (because it comes from the original XUL document) and all its children in this case are anonymous (because they come from the <content> of the XBL binding).

Comment 10

[Allison Naaktgeboren :ally]

Attachment: part 1/1, proper

looks like we no can haz magic.

Comment 11

[Allison Naaktgeboren :ally]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> Review of attachment 736561 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/metro/base/content/bindings/autocomplete.xml
> @@ +70,5 @@

> If the flexes are what's causing the weird behavior, you could apply them in
> CSS for the snapped view only (using "-moz-box-flex: 1;").

The flexes aren't causing the (what seems to me) weird layout (where results lays out in two short columns but searches is one long column) mentioned in the comment. Turns out that's just how it is on trunk. The flex makes no difference in landscape(there's more than enough room in the parent) but do make the correct difference in snapped

> ::: browser/metro/base/content/browser-ui.js
> 
> Wow, nice find.  Perhaps we should file a follow-up bug on this.

What on? I did talk to mconnor, who points out that xul has its own box model, which is sometimes different from the css box model, and that may be why we see divergent behavior here. Not sure if we can do anything about that? :/
> 
> ::: browser/metro/base/content/browser.xul
> @@ +225,5 @@
> > +                <!-- Is this hte binding, the actual parent, what? I
> > +                     thought this was a tree, and isnt this putting
> > +                     cycles in the tree? -->
> 
> I'll give this a shot; hopefully I'm answering the right question.

That was very useful, thanks. What I was confused about was the parent(s)/owner(s) of start-autocomplete, and the relationship between the code that claims it as an attribute & the code the claims it as a child(I think?). I realize now that the patch didnt pick up enough context to make that clear. My apologies. 

We have 
<hbox id="start-container ...>
 ...
 <box id="start-autocomplete" observes="bcast_windowState"/>
 ...
</hbox>

versus 
 <textbox id="urlbar-edit"
   ... 
   autocompletepopup="start-autocomplete"
   ... />

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 737049 [details] [diff] [review]
part 1/1, proper

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

The patch looks good. I ran into a couple of questions though which I'll add to this ticket

::: browser/metro/base/content/bindings/autocomplete.xml
@@ +67,4 @@
>    </binding>
>  
>    <binding id="autocomplete-popup">
> +    <content orient="horizontal" class="meta viewable-height viewable-width" flex="1">

I think we can ditch the flex attribute in the content element as its redundant with viewable-* classes

::: browser/metro/theme/browser.css
@@ +870,4 @@
>    visibility: collapse;
>  }
>  
> +#start-autocomplete[viewstate="snapped"] .richgrid-item-content {

I know you don't like this descendant selector. But the equivalent child selector is long and brittle. I would say lets run with it until we can prove its problem that needs solving

Comment 13

[Sam Foster [:sfoster] (he/him)]

Is there a way to get the soft keyboard in snapped view that I'm missing? I put metrofx in a side column. Tap in teh location bar and the cursor is there,  but no soft keyboard to input with. So I can't test/use snapped view autocomplete without a physical keyboard currently

Comment 14

[Asa Dotzler [:asa]]

(In reply to Sam Foster [:sfoster] from comment #13)
> Is there a way to get the soft keyboard in snapped view that I'm missing? I
> put metrofx in a side column. Tap in teh location bar and the cursor is
> there,  but no soft keyboard to input with. So I can't test/use snapped view
> autocomplete without a physical keyboard currently

Snapped view should work with the soft keyboard.  It should come up just fine when you focus a text field in snapped Firefox. It not coming up is a defect.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: Screenshot of snapped autocomplete getting clipped

This is what I get. There's some clipping of the results on the right when in snapped view. Not sure without digging, but maybe viewable-width isn't the right number here?

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Asa Dotzler [:asa] from comment #14)
> (In reply to Sam Foster [:sfoster] from comment #13)
> > Is there a way to get the soft keyboard in snapped view that I'm missing? I
> > put metrofx in a side column. Tap in teh location bar and the cursor is
> > there,  but no soft keyboard to input with. So I can't test/use snapped view
> > autocomplete without a physical keyboard currently
> 
> Snapped view should work with the soft keyboard.  It should come up just
> fine when you focus a text field in snapped Firefox. It not coming up is a
> defect.

...I can't reproduce this, the soft keyboard comes up & works fine for me :(

Comment 17

[Allison Naaktgeboren :ally]

(In reply to Sam Foster [:sfoster] from comment #15)
> Created attachment 738050 [details]
> Screenshot of snapped autocomplete getting clipped
> 
> This is what I get. There's some clipping of the results on the right when
> in snapped view. Not sure without digging, but maybe viewable-width isn't
> the right number here?

I see what you dislike here. I think its a margin/border issue. Let me see what I can do

Comment 18

[Allison Naaktgeboren :ally]

Attachment: carrying over sfoster's r+, w/better borders

Comment 19

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3d1492bc46

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c3d1492bc46