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

EZP-30732: Allows table cells to contain inlines and blocks #56

Closed

Conversation

@SerheyDolgushev
Copy link
Contributor

commented Aug 7, 2019

Question Answer
JIRA issue EZP-30732
Bug/Improvement yes
New feature no
Target version 1.1
BC breaks no
Tests pass yes
Doc needed no

Seems like there is a bug in doocbook.rng:

    <define name="db.html.td">
      <element name="td">
        <a:documentation>A table entry in an HTML table</a:documentation>
        <ref name="db.html.td.attlist"/>
        <choice>
          <zeroOrMore>
            <ref name="db.all.inlines"/>
          </zeroOrMore>
          <zeroOrMore>
            <ref name="db.all.blocks"/>
          </zeroOrMore>
        </choice>
      </element>
    </define>

The fist condition is always true:

          <zeroOrMore>
            <ref name="db.all.inlines"/>
          </zeroOrMore>

And thats if table cell cotnains any db.all.blocks - triggers a validation error:
Validation of XML content failed

The most correct fix would be to make this change in doocbook.rng.

This PR is related to ezsystems/ezplatform-admin-ui#1034.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@SerheyDolgushev SerheyDolgushev changed the base branch from master to 1.1 Aug 7, 2019

@SerheyDolgushev SerheyDolgushev force-pushed the SerheyDolgushev:fix_blocks_in_table_cells branch from a26f93b to 23ac9fb Aug 7, 2019

@dew326 dew326 requested a review from alongosz Aug 8, 2019

@vidarl

vidarl approved these changes Aug 12, 2019

Copy link
Member

left a comment

@SerheyDolgushev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

What is the best way to contact docbook and let them know about this bug?

@ndw

This comment has been minimized.

Copy link

commented Aug 12, 2019

Can you provide a sample document that you believe should validate but doesn't? This document validates for me:

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td><para>This is a paragraph.</para>
  </td>
</tr>
</tbody>
</informaltable>

</article>
@SerheyDolgushev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@ndw can you try to test any block (not inline) element inside the table cell? F.e.

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td>
   <blockquote />
  </td>
</tr>
</tbody>
</informaltable>

</article>

a011b56 is faling without this PR.

@ndw

This comment has been minimized.

Copy link

commented Aug 12, 2019

The problem with that example isn't in the table, it's in the blockquote. DocBook does not allow an empty block quotation. This validates just fine for me:

<article xmlns="http://docbook.org/ns/docbook">
<title>Test</title>

<informaltable>
<tbody>
<tr>
  <th>Heading</th>
</tr>
<tr>
  <td><blockquote><para/></blockquote>
  </td>
</tr>
</tbody>
</informaltable>

</article>
@SerheyDolgushev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@ndw you are correct. After spending more time on this issue, seems like the true reason was found: there was zero width space (U+200B) after the child element in the table cell. Child element was a block type. So our case was:

<article xmlns="http://docbook.org/ns/docbook">
  <title>Test</title>
  <informaltable>
    <tbody>
      <tr>
        <th>Heading</th>
      </tr>
      <tr>
        <td>
          <blockquote>
            <para/>
          </blockquote>
          [zero width space]
        </td>
      </tr>
    </tbody>
  </informaltable>
</article>

The current PR makes this content valid, and no validation erros are thrown. But, I assume, the more correct solution would be to fix the code which adds zero width space (instead of changing the rng schema).

@ndw can you think about any other cases where zero width space might lead to validation errors?

@SerheyDolgushev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@vidarl seems like this one might be closed in the flavor of ezsystems/ezplatform-admin-ui@9e8739d

@ndw

This comment has been minimized.

Copy link

commented Aug 13, 2019

@ndw

This comment has been minimized.

Copy link

commented Aug 13, 2019

@SerheyDolgushev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

It took me a few minutes to get my head around this. The XML Recommendation defines whitespace as explicitly (and exclusively) SPACE ( ), TAB ( ), and NEWLINE ( )[]. Any other character that occurs between block elements (including zero-width space, non-breaking space, etc. etc. etc.) is effectively text where it isn't allowed. A zero-width space where only elements are allowed is going to cause validation errors every time. [] Technically, CARRIAGE RETURN (#xD;) is also part of the space production, but the parser removes them or converts them to NEWLINE in all cases except where they occur as explicit numeric character references.

On Tue, Aug 13, 2019 at 6:25 AM Serhey Dolgushev @.> wrote: @vidarl https://github.com/vidarl seems like this one might be closed in the flavor of @. <ezsystems/ezplatform-admin-ui@9e8739d> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56?email_source=notifications&email_token=AAAI7OKSJQVVKBZRPGFBJD3QEKKY5A5CNFSM4IKAGMB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4FLOFI#issuecomment-520795925>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAI7OMSLFTKIYW5VAB2GMTQEKKY5ANCNFSM4IKAGMBQ .

Yep, this one was fun :)
It was very difficult to discover that empty width space between the child elements of td. And as you said it counts like a text, which explains why validation error was triggered. So no changes in the schema are required. It needs to be fixed in the app logic which is responsible for generating XML.

@ndw Thank you a lot for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.