1065275 - Implement tab view in the desktop client panel

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

In order to show a contacts list in the desktop client, we'll need to have a tabview component first to switch between the link sharing view to the contacts view.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add a TabView component to the panel

Comment 2

[:Paolo Amadini]

I've applied this patch and it works quite well with the mouse. However, it's not clear to me how the keyboard interaction would work.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8487202 [details] [diff] [review]
Patch v1: add a TabView component to the panel

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

This works, but I think we could come with a bit less hacks to achieve the same thing (I provided an example) :)

::: browser/components/loop/content/js/panel.jsx
@@ +24,5 @@
>  
> +  var TabView = React.createClass({
> +    getInitialState: function() {
> +      return {
> +        selectedTab: "call"

Nit: we'll probably want to initialize state with a prop value, to ease rendering component in some default state in the UI component showcase.

@@ +34,5 @@
> +    },
> +
> +    setTabStates: function(which) {
> +      var selectedTab = which || this.state.selectedTab;
> +      var ownerRefs = this._owner.refs;

This is fragile because it relies on private implementation of React…

@@ +41,5 @@
> +        if (ref.constructor.displayName !== "Tab") {
> +          return;
> +        }
> +        ref.setState({selected: name == selectedTab});
> +      });

Woohoo, this is complicated :)

I tried to come with the simplest possible approach and here's the result http://jsbin.com/pirel/7/edit

WDYT?

@@ +59,5 @@
> +      var cx = React.addons.classSet;
> +      var children = this.props.children;
> +      var tabButtons = [];
> +      var tabRef, isSelected;
> +      for (var i = 0, l = children.length; i < l; ++i) {

How about using React.Children.map instead here?

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to :Paolo Amadini from comment #2)
> I've applied this patch and it works quite well with the mouse. However,
> it's not clear to me how the keyboard interaction would work.

That's a good point, though panel accessibility should be probably addressed in another bug (to file?). It really depends on our Definition of Done here, but I suspect trying to address all aspects at once will slow us down more than anything else, sadly :/

Comment 5

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #2)
> I've applied this patch and it works quite well with the mouse. However,
> it's not clear to me how the keyboard interaction would work.

After reading some more ReactJS docs, my confusion was because the "key" attribute did not refer to keyboard shortcuts :-) I agree we can handle keyboard interaction in a follow-up bug.

Actually, the "key" attribute is probably unneeded in the patch because the order and number of the items do not change.

Comment 6

[:Paolo Amadini]

(In reply to Nicolas Perriault (:NiKo`) from comment #3)
> I tried to come with the simplest possible approach and here's the result
> http://jsbin.com/pirel/7/edit

"React.Children.only" is probably unneeded since tabs can have multiple children, but this looks like the approach I was experimenting with.

One question I have is whether we should render the pages every time and toggle visibility, or render only the current page. Will or can ReactJS keep state in the differencing algorithm for pages we don't render, and just re-insert them when requested?

Comment 7

[:Paolo Amadini]

Attachment: Changes

These changes on top of the existing patch seem to work. This still renders all pages.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to :Paolo Amadini from comment #5)
> Actually, the "key" attribute is probably unneeded in the patch because the
> order and number of the items do not change.

It's actually required :) http://facebook.github.io/react/docs/multiple-components.html#dynamic-children(In reply to :Paolo Amadini from comment #6)

> > http://jsbin.com/pirel/7/edit
> "React.Children.only" is probably unneeded since tabs can have multiple
> children, but this looks like the approach I was experimenting with.

That's a very good point, thanks. Here's an updated version :) http://jsbin.com/pirel/9/edit

> One question I have is whether we should render the pages every time and
> toggle visibility, or render only the current page. 

We shouldn't deal with visibility with CSS, if that's what you mean. Re-rendering a React component is cheap and fast thanks to the VDom diff and batch DOM updates mechanisms.

> Will or can ReactJS keep
> state in the differencing algorithm for pages we don't render, and just
> re-insert them when requested?

Yup, that's its whole purpose (and s/re-insert/re-render) :)

Comment 9

[:Paolo Amadini]

(In reply to Nicolas Perriault (:NiKo`) from comment #8)
> (In reply to :Paolo Amadini from comment #5)
> > Actually, the "key" attribute is probably unneeded in the patch because the
> > order and number of the items do not change.
> 
> It's actually required :)
> http://facebook.github.io/react/docs/multiple-components.html#dynamic-children

From what I read in the document, the attribute is required when the list is dynamic, but not required when it is static, since ReactJS already knows how to do the right thing in that case. So, at least in the patch that does not remove the tabs but hides them with CSS, it seems to me the attribute isn't needed. Or did I misunderstand?

(In reply to :Paolo Amadini from comment #6)
> We shouldn't deal with visibility with CSS, if that's what you mean.
> Re-rendering a React component is cheap and fast thanks to the VDom diff and
> batch DOM updates mechanisms.

Hm, according to the docs, ReactJS computes the "most efficient DOM mutation", but if this computes to completely removing one tab and completely adding back the other, those mutations will still be required each time the tab is changed, right?

If the removed DOM elements are kept in a memory cache or in some hidden place by ReactJS already, this might not be so bad, so this is why I'm asking.

This ties in with the 1000s contacts list investigation. If it takes, let's say, 100ms to add all the nodes, but it is only done the first time and maybe in the background while the tab is hidden, then we don't need to implement a virtual view at all. If it takes 100ms to add them every time the tab is changed, the situation can be different.

If we can solve this problem in a simpler way by using CSS hiding instead of a complex virtual view, it would be great.

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to :Paolo Amadini from comment #9)
> From what I read in the document, the attribute is required when the list is
> dynamic, but not required when it is static

Let me put this another way: if you don't set a key prop, you get a bad warning from react in the console telling you to do so. 

Also, here the list is dynamic, because we're mapping, which is a transformation operation from one list to another.

> Hm, according to the docs, ReactJS computes the "most efficient DOM
> mutation", but if this computes to completely removing one tab and
> completely adding back the other, those mutations will still be required
> each time the tab is changed, right?
> 
> If the removed DOM elements are kept in a memory cache or in some hidden
> place by ReactJS already, this might not be so bad, so this is why I'm
> asking.
> This ties in with the 1000s contacts list investigation

But you'd hold a whole lot of stuff in memory, even if hidden. Also, we're talking about a tab component, we won't ever have 1000 tabs (well, hopefully :)).

> This ties in with the 1000s contacts list investigation. If it takes, let's
> say, 100ms to add all the nodes, but it is only done the first time and
> maybe in the background while the tab is hidden, then we don't need to
> implement a virtual view at all.

What when you need to update a tab content? You'll propagate state change deep into the concerned sub components, which is basically triggering a whole rerender — which is still cheap, because React makes rerendering a whole bunch of deeply nested components cheap.

> If it takes 100ms to add them every time
> the tab is changed, the situation can be different.

We could spend some time microbenchmarking this kind of scenarios, but in y own experience rerendering is really cheap. That's also the way things have been mostly done in Loop for a while now, with no noticeable performance impact. (The contact list is a totally different story, as it's not Loop code yet).
 
> If we can solve this problem in a simpler way by using CSS hiding instead of
> a complex virtual view, it would be great.

I disagree :) To me that would be the jQuery way of doing it. As we're using React, we should embrace its paradigm which is compute a VDom representation of the UI matching a given state at any point in time — not rendering everything at first start then updating applied classes to show/hide things.

That may be something we'll want to discuss more deeply with other people though, especially those involved with Web performance, DOM and React at Mozilla.

Comment 11

[:Paolo Amadini]

(In reply to Nicolas Perriault (:NiKo`) from comment #10)
> Let me put this another way: if you don't set a key prop, you get a bad
> warning from react in the console telling you to do so. 
> 
> Also, here the list is dynamic, because we're mapping, which is a
> transformation operation from one list to another.

Ah, I see, thanks for the clarification!

> > If it takes 100ms to add them every time
> > the tab is changed, the situation can be different.
> 
> We could spend some time microbenchmarking this kind of scenarios, but in y
> own experience rerendering is really cheap.

If rendering anew is cheap, that definitely alleviates my concerns :-)

> > If we can solve this problem in a simpler way by using CSS hiding instead of
> > a complex virtual view, it would be great.
> 
> I disagree :) To me that would be the jQuery way of doing it. As we're using
> React, we should embrace its paradigm which is compute a VDom representation
> of the UI matching a given state at any point in time — not rendering
> everything at first start then updating applied classes to show/hide things.
> 
> That may be something we'll want to discuss more deeply with other people
> though, especially those involved with Web performance, DOM and React at
> Mozilla.

I agree that, all other things being equal, we should definitely use the ReactJS paradigm. Especially if we end up only ever rendering 5 contacts, as it was suggested, or if rendering performance turns out not to be a problem at all.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: add a TabView component to the panel

Paolo, your changes looked great! I incorporated them in this patch and incorporated a few minor changes.

What do you think?

Comment 13

[:Paolo Amadini]

Comment on attachment 8487865 [details] [diff] [review]
Patch v2: add a TabView component to the panel

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

r+ for landing in the "browser" folder with a few changes, though you probably want a separate CSS review first, from someone who knows the area more than me, and any relevant additional feedback.

A bug for adding keyword interaction to switch tabs needs to be filed.

::: browser/components/loop/content/js/panel.jsx
@@ +44,5 @@
> +      var tabs = [];
> +      React.Children.forEach(this.props.children, function(tab) {
> +        var tabName = tab.props.name;
> +        var isSelected = (this.state.selectedTab == tabName);
> +        // TODO: add localized tooltip captions for each tab button.

Please file a bug for this, and omit the source code comment.

@@ +49,5 @@
> +        tabButtons.push(
> +          <li className={cx({selected: isSelected})}
> +              data-tab-name={tabName}
> +              onClick={this.handleSelectTab}>
> +          </li>

So, it turns out these require "key" attributes (I believe the tabName can be used for the purpose). You should see a related console warning otherwise.

@@ +54,5 @@
> +        );
> +        tabs.push(
> +          <div className={cx({tab: true, selected: isSelected})}>
> +            {tab.props.children}
> +          </div>

Based on the current discussion, we can go with rendering only the active tab. In the unlikely case it turns out to be problematic, we can easily revise this later.

@@ +470,5 @@
>      },
>  
> +    selectTab: function(which) {
> +      //XXX To be used for initializing tab contents' lazy loading from a data-source.
> +    },

This can just be omitted in the final patch.

Comment 14

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/9157d0662102

Comment 15

[Mike de Boer [:mikedeboer]]

Paolo is working on a unit test; I'll leave it up to him if he wants to file a separate bug for that.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9157d0662102

Comment 17

[u279076]

Verified fixed in today's Firefox Nightly 35.0a1.

Comment 18

[u279076]

Untracking for verification on the Fig branch prior to uplift.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8487865 [details] [diff] [review]
Patch v2: add a TabView component to the panel

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09e683df2115

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8487865 [details] [diff] [review]
Patch v2: add a TabView component to the panel

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 22

[u279076]

This is working as expected in today's Firefox Aurora 34.0a2.