Setting containerTagName to null won't clear the container

[zepumph]

This happens when there are other siblings than just the primary sibling, basically because the default kicks in too strong. I want to discuss the desired functionality before diving in.

[zepumph]

What should the functionality be? Other option remove pDOM content when set back to null, but I see the container potentially as a different beast. Potential options on behavior when containerTagName: null is set:

1. No op, but we document it well.
2. Assert if there are siblings, because that is broken code.
3. Something clever and vague like automatically removing siblings, but documenting it well.
4. Option 4: wildcard, any suggestions?

Marking for dev meeting.

[jessegreenberg]

I like option 2. We have this assertion in insertContentElement:

assert && assert( accessiblePeer.containerParent, 'Cannot add sibling if there is no container element' );

Which will error if you do this:

    b.tagName = 'button';
    b.labelTagName = 'p';
    b.containerTagName = null;

But not if you do this:

    b.containerTagName = null;
    b.tagName = 'button';
    b.labelTagName = 'p';

Also, this won't error:

    var b = new Node( {
      tagName: 'button',
      labelTagName: 'p',
      containerTagName: null
    } );

because labelTagName happens to come after containerTagName in ACCESSIBILITY_OPTION_KEYS.

[jessegreenberg]

Hmm, from #761 (comment),

    b.containerTagName = null;
    b.tagName = 'button';
    b.labelTagName = 'p';

is perfectly valid if default value for containerTagName is null. So I am not sure yet what the best option is.

[zepumph]

I don't know what the word "valid" means here. Yes the above works, but the containerTagName is still added. This is because the labelTagName call adds in a parent automatically if conatiner is null.

Both of the following code snippets yield the same HTML. Please confirm and play in the playground.

    var b = new Node();
    b.containerTagName = null;
    b.tagName = 'button';
    b.labelTagName = 'p';

    var b = new Node( {
      tagName: 'button',
      labelTagName: 'p',
      containerTagName: null
    } );

<div class="accessibility">
  <div tabindex="-1" id="container-2-11">
    <p tabindex="-1" id="label-2-11"></p>
    <button id="2-11" tabindex="0"></button>
  </div>
</div>

[jessegreenberg]

Sorry, I was trying to say that they are effectively no-ops because the default value for containerTagName is null. It is only an error if you change the order so labelTagName is set before containerTagName.

[zepumph]

I worry about it seeming clever and confusing. Here, an explicit call to nullify a tagName doesn't change the pDOM at all.

Maybe it will be good to describe this behavior in the containerTagName doc, "unlike other tagName setters, explicitly setting containerTagName to null will be overwritten if there are siblings in the peer."

[jessegreenberg]

I agree it is a problem and think documentation may be the best way to go. What if we assert that containerTagName cannot be set to null, only allowing null as a default value on construction?

[zepumph]

What about:

    var b = new Node( {
      tagName: 'button',
      labelTagName: 'p',
    } );

  b.labelTagName = null;
  console.log( b.containerTagName); // ---> 'DIV'
  b.containerTagName = null;
  console.log( b.containerTagName); // ---> null

Confirmed in playground. It seems like right now, it would be important to be able to clear that container after clearing the label. It is a bummer that you have to "cleanup" the container after you didn't even set it.

[jessegreenberg]

Totally agree.

It is a bummer that you have to "cleanup" the container after you didn't even set it.

Do you think it is worth throwing an error rather than creating a a default parent DOMElement when adding label/description siblings, so client always has to add the container?

EDIT: for clarity

[zepumph]

No I don't think this is worth it. I think that makes things much more annoying to use, and that it isn't worth it to have to add this to every place where a sibling is declared (sorta like trading one gotcha for another).

Let's go down the documentation route, as devs will be looking at the doc for sibling/parent setters a lot as they learn how it works. We can get feedback from them as we go.

[jessegreenberg]

OK, that sounds great to me.

[zepumph]

Alright, @jessegreenberg please review and close if that's all (commit coming when my computer wakes up).

[jessegreenberg]

This documentation was helpful, but I don't think it is accurate anymore after #715, since siblings no longer require a container tag name. I actually removed it. @zepumph can you please verify this is correct?

[zepumph]

Good find! THanks.