RFC: Layout and ItemLayout render a big amount of nested DOM nodes

[bmdalex]

Layout and ItemLayout render a big amount of nested DOM nodes

Steps

I was trying to reuse and ItemLayout (and Layout) Stardust components to re-write the Chat.Message component since it looked that I can leverage the logic provided by ItemLayout props (media, endMedia, header, headerMedia, etc) to add author and date props to Chat.Message.

However, I realized these layout components render a big amount of redundant DOM nodes; here is an example for a very simple Chat.Message implemented with ItemLayout:

Example code:

const messages = [
  {
    key: 1,
    author: 'John Doe',
    date: 'Mon Sep 17 2018 13:54:05',
    content: 'Hello',
    mine: true,
    avatar: { src: 'public/images/avatar/small/matt.jpg', status: availableStatus },
  },
]

const ChatExampleAvatar = () => <Chat messages={messages} />

export default ChatExampleAvatar

Screenshot:

Rendered DOM:

<li date="Mon Sep 17 2018 13:54:05" class="ui-layout ui-itemlayout ui-chat__message">
  <div class="ui-layout__main">
    <div class="ui-item-layout__main" style="grid-template-rows:1fr 1fr">
      <div class="ui-layout ui-item-layout__header">
        <div class="ui-layout__main">John Doe</div>
        <span class="ui-layout__gap"></span>
        <div class="ui-layout__end">
          <span class="ui-item-layout__headerMedia">Mon Sep 17 2018 13:54:05</span>
        </div>
      </div>
      <div class="ui-layout ui-item-layout__content">
        <div class="ui-layout__main">Hello</div>
      </div>
    </div>
  </div>
  <span class="ui-layout__gap"></span>
  <div class="ui-layout__end">
    <span class="ui-item-layout__endMedia">
      <div class="ui-avatar">
        <img aria-hidden="true" src="public/images/avatar/small/matt.jpg" class="ui-image"/>
        <span title="Available" class="ui-status">
          <span class="ui-icon" aria-hidden="true"></span>
        </span>
      </div>
    </span>
  </div>
</li>

Implementation:

<ItemLayout
  as={as}
  {...rest}
  className={cx(classes.root, classes.content)}
  media={!mine && this.renderAvatar(avatar, styles, variables)}
  endMedia={mine && this.renderAvatar(avatar, styles, variables)}
  header={author}
  headerMedia={date}
  content={content}
  truncateHeader={true}
/>

Issue:

Expected Result

The DOM tree has a reasonable depths (let's say a max of 4 nodes).

Actual Result

The DOM tree has a depth of 6 just for this basic example!

Proposed solution

Add better conditions to Layout and ItemLayout components in order to prevent additional DOM elements in the tree when a certain rendered area (start, main, end, header, media, etc) is represented by:

• another ItemLayout / Layout component
• single DOM node
  What we do now is to add a wrapper DOM element (div usually) to wrap content for a layout slot. We can use the single node itself for that and apply the layouting styles directly.

Version

0.5.2

[kuzhelov]

I would rather propose to focus on correctness aspects while being at the stage we are now, rather than trying to optimize. But even speaking of whether it is a problem for now - to me it seems that it is not, as DOM tree that is currently rendered seems to be appealing in terms of semantics each div element provides.

So, just to reiterate - lets consider to close it for now, and if we would have a serious problem related to the amount of DOM nodes rendered, we will return to it - at the same time, probably, we would be able to introduce simplier techniques to handle this problem (like relying on CSS layout tools), rather than tweak the component's logic for it.

[bmdalex]

I am on the same page when it comes to the fact that a correct approach is vital, but I am a strong believer that the approach should be simple and optimal, especially in these early stages.

I don't agree with closing this, but rather add some proposal to it first and see if someone comes up with something feasible in terms of how correct, optimal simple and time consuming (from dev implementation POV) the proposals are.

It's also a lot better to tackle this now rather than later when it could be spread on a lot of components. The side effects would be so much bigger making a change like this later.

@levithomason showed us a nice POC on how the DOM tree size can be drastically reduced using CSS grid for very simple elements (I think for Label), and it was a percentage of >50% of nodes being removed.

Imagine having that element tens or hundred of times in your app (the case of buttons, chat messages, labels, etc), we'd have hundreds of unnecessary nodes being rendered and a potential source of:

• bugs (from complexity);
• perf impact;
• debugging complexity and confusion.

[levithomason]

Work in progress here, #401. The idea is to see if CSS Grid layouts can be used to replace most of our Layout needs.

[jurokapsiar]

(pasting the discussion we had some time ago)

I did a small prototype to find out if grids with templates will work from accessibility point of view if they wrap focusable elements.

It works correctly only if the template order is the same as the order in DOM. If you use the template to switch the order, both FocusZone as well as virtual cursors of screen readers break. You can see it in proto/css-grid-focus branch (CSS Grid prototype in the menu)

So we can use templates, but our API should not allow to reposition areas - they always need to be in the same order as they are in DOM.

Usual ways of changing the tab order (tabIndex >= 1 or responding to TAB keypress events) work only partially as they do not influence the order in which screen readers order the tabbable elements.

[bmdalex]

We have successfully removed the Layout usage from Chat.Message implementation in #518 with pretty small costs and lots of benefits and will carry on for rest of components. That work is being tracked here: #525