Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dom-repeat inside table element is having unexpected behavior in 2.0 preview #4135

Closed
reinert opened this issue Nov 7, 2016 · 31 comments
Closed
Assignees

Comments

@reinert
Copy link

reinert commented Nov 7, 2016

When we set a dom-repeat inside a table element, it presents the content outside the table.

  <template>
    <table>
      <thead>
        <tr><th>Header</th></tr>
      </thead>
      <tbody>
        <dom-repeat items="{{items}}">
          <template>
            <tr>
              <td>{{item.name}}</td>
            </tr>
          </template>
        </dom-repeat>
      </tbody>
      <tfoot>
        <tr><td>Footer</td></tr>
      </tfoot>
    </table>
  </template>

image

But if we instead use the former syntax template is="dom-repeat", it works as expected.

  <template>
    <table>
      <thead>
        <tr><th>Header</th></tr>
      </thead>
      <tbody>
        
          <template is="dom-repeat" items="{{items}}">
            <tr>
              <td>{{item.name}}</td>
            </tr>
          </template>
        
      </tbody>
      <tfoot>
        <tr><td>Footer</td></tr>
      </tfoot>
    </table>
  </template>

image

Maybe related to #3919.

@sorvell
Copy link
Contributor

sorvell commented Nov 7, 2016

Using dom-repeat inside elements with special parsing rules like <table> and <select> is not currently supported in the 2.x-preview, but we're working on it.

With the loss of is in custom elements v1, <dom-repeat> is now no longer a template extension. For compatibility with Polymer 1.x, we're currently automatically wrapping the previous syntax in a <dom-repeat> element. The <table> and <select> elements have special parsing rules that kick out elements not allowed to be children. The <template> element is accepted but <dom-repeat> is not. This explains the behavior you're seeing.

@reinert
Copy link
Author

reinert commented Nov 7, 2016

Interesting. Thanks.

#damnApple

@kevinpschaaf
Copy link
Member

kevinpschaaf commented Nov 7, 2016

This limitation basically known with the change to HTMLElement-based template wrappers, and we have plans to resolve this in Polymer's templating system by changing the "auto-wrapping" of <template>'s to a scheme where the dom-repeat element is created outside of the DOM with a pointer back to the template which is left inside the dom as the position tracking element. Will keep this issue open to track that work.

@reinert
Copy link
Author

reinert commented Nov 8, 2016

@kevinpschaaf this is horrible. Have you argued this case to get the "is" attribute accepted by everybody?

@richardkazuomiller
Copy link

Maybe if the other browser vendors knew the lengths people have to go to now that extending builtin elements is gone, they might change how they feel about is.

@jogibear9988
Copy link

how will on this bug be worked on? will polymer build it's own "is" mechanism?

@madeleineostoja
Copy link

madeleineostoja commented Nov 10, 2016

Will this mean that Polymer will reccomend two distinct syntaxes for Dom helper elements going forward? A shimmed is extension for cases like this (transformed with a pointer) and the 'regular' custom element wrappers for all other scenarios?

@oleersoy
Copy link

In this case we already have access to the custom element <dom-repeat>, so we have the ability to use custom elements. Why not just create a few more custom elements that allow the <dom-repeat> inside of them ( x-table, x-td, etc?)?

@reinert
Copy link
Author

reinert commented Nov 10, 2016

And lose the default rendering of the table element?

@oleersoy
Copy link

I have not looked in a while, but isn't the default rendering pretty primitive and non-standard across browsers anyways?

@trusktr
Copy link

trusktr commented Nov 11, 2016

It seems like a better choice to leave iteration to a higher-level view layer, and leave HTML/DOM strictly for content.

We could, for example, use a handlebars loop to render the stuff in the tbody rather than using dom-repeat which gets in the way of the DOM content.

@jogibear9988
Copy link

or we add a attribute to dom repeat ("targetId"), if unset we use our own shadow dom, if set we use the target

@oleersoy
Copy link

Or 2 separate elements. Use <dom-repeat> when iterating over content that is to be set in place and <target-repeat> for the other scenario.

@reinert
Copy link
Author

reinert commented Nov 12, 2016

I have not looked in a while, but isn't the default rendering pretty primitive and non-standard across browsers anyways?

@oleersoy I was not reffering to style strictly, but to all <table> related elements and how they are rendered by the browser. Think of a simple <td colspan="2">.

Also take the <select> element. We would lose the rendering of the <option> inner elements; a widely used element the way it is.

@reinert
Copy link
Author

reinert commented Nov 12, 2016

It seems like a better choice to leave iteration to a higher-level view layer, and leave HTML/DOM strictly for content.

@trusktr I couldn't agree more! But would that be out of the spec's scope? It would be charming to the spec address this issue.

@oleersoy
Copy link

@reinert I see what you mean. Lets assume we have it in the specs scope right now agreed to by both Apple and MS.

Now we have to document that <dom-repeat> will do x,y, and z. With the is attribute comes the additional documentation with respect to specific scenarios / contexts.

So if the current solution that @kevinpschaaf noted turns out being equally performant and easy to use, which it sounds like it will, we get the iteration you are looking for and someone gets to skip writing the explanation noting when to use what.

@madeleineostoja
Copy link

madeleineostoja commented Nov 12, 2016

@reinert I think @trusktr is suggesting (correct me if I'm wrong) using a dedicated templating layer (eg: handlebars) on top of Polymer. Or, in other words, just don't use web components for this kind of thing.

This seems pretty heavy handed to me, especially since with the extra js dep we could just carry the polyfill for type extension on webkit and have the old behaviour back across the board. It looks like Mozilla and Edge are implementing is="", so you would only be polyfilling on Safari.

@oleersoy
Copy link

I have to agree that the whole thing does seem heavy handed. We are building a bridge to get around the spec / html schema, so if the spec / schema was relaxed the problem goes away. Shouldn't we be asking browser vendors to do this?

@reinert
Copy link
Author

reinert commented Nov 12, 2016

Yeah, heavy handed. For now, better to stick with the "is" attribute and use the polyfill for safari only.

@madeleineostoja
Copy link

@reinert and others that care to follow, there's a long and intricate ongoing debate on supporting type extension as per the v1 spec at WICG/webcomponents#509. If you'd like to see it supported by vendors that's the best place to argue the case.

@oleersoy
Copy link

We should really:

  1. Have library support for is and <dom-repeat> for the reason that a lot of people like both. In both cases we need polyfills for not just Safari, but for all browsers not yet upgraded with custom element and is support.
  2. Relax the schema removing the need for is within browser to support this specific scenario. That way we can implement things like <dom-repeat> without performing hacks.

@reinert it's heavy handed, but only because we have a html schema that forces a heavy handed technique. So by advocating the usage of is we are doubling up on the heavy handedness. We are just laying another mattress on top of the 9th mattress to soften up the pea at the bottom of the stack.

Assuming the schema restriction is removed and we fast forward ten years we no longer need is (At least not for the hurdle we are trying to clear here). The spec should be things that simplify development for all of us across the spectrum of considerations like simplified maintenance, improved development experience, etc.

@reinert
Copy link
Author

reinert commented Nov 13, 2016

True. By relaxing the schema this problem would be solved in a simpler way (and probably many others).

@madeleineostoja
Copy link

You can argue it both ways. This is just one of many use cases where type extension solves a very real problem.

@hax
Copy link

hax commented Dec 13, 2016

Relaxing schema is NOT a simpler way, it just introduce more problems because it breaks mental model of developers about tables/ol/ul/dl etc.

@kevinpschaaf
Copy link
Member

kevinpschaaf commented Feb 25, 2017

The good news is... that we've decided to continue supporting <template is="dom-repeat"> and <template is="dom-if"> inside Polymer element templates (only). We're doing this not via actual support of custom element type extensions, but rather by re-writing the template to replace the <template is="dom-..."> element with the equivalent <dom-...> wrapper element with the <template> as its child prior to stamping. So you can think of this as a limited template processing feature rather that full support for custom element type extensions.

This has a couple of practical benefits that outweigh the otherwise uncanny usage of is:

  • Since it is parsed as a <template>, the HTML parser will not eject it from parser-constrained elements like <table> and <select>; when we imperatively replace it with the wrapper in the template, the parser constraints are no longer involved and we can get away with the wrapper (e.g. <dom-repeat>) inside the parser-constrained elements (hence the second example in the OP works as expected)
  • It also provides a smoother path for hybrid elements and porting efforts, since the syntax change is not required within Polymer element templates

Note that any main-document use of dom-repeat or dom-if must use the wrapper form, or else contain them inside a dom-bind (which itself must always be in wrapper form).

@kevinpschaaf
Copy link
Member

cc @arthurevans FYI, might be some relevant detail here for docs.

@gechols
Copy link

gechols commented Feb 28, 2017

I'm confused at the 'closed' status of this issue. It appears that a 'decision' was made - which sounds very promising. However, with no mention of a fix being made I'm left wondering what happened? Are we still waiting for a fix or is there a version somewhere with this working that we can test?

@trusktr
Copy link

trusktr commented Mar 1, 2017

@kevinpschaaf's comment mentions is="" will continue to be supported in templates.

I believe he is implying that you are to use the is="" attribute inside a template to solve any problems similar to the initial post, and avoid using the element form of dom-repeat.

In other words, I think he means: "we support the attribute form of dom-repeat for those cases, not the element form, so no fix is needed for the element form and don't use element form for those cases".

@arthurevans
Copy link

@gechols Yes, @trusktr is correct. Just use <template is="dom-repeat"> when you're working inside any Polymer-managed template (i.e., an element template, dom-bind, or another dom-repeat or dom-if). This works now in 2.0-preview, and is noted in the preview docs.

Working on Release Candidate docs now, where we will clarify that this is the recommended path.

@gechols
Copy link

gechols commented Mar 1, 2017

Ah - I see that the latest 2.0-preview has several fixes in it. This is what I was missing. My tables are starting to look much better now!

I can't wait to start seeing the RC versions come out. Thanks for all the hard work on the new version.

@jogibear9988
Copy link

Yes, but the docs say also, that this autowrapping might be removed in the feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests