lorenzo/pinky

First <container> will be ignored when it's the first element in <center>

kanow opened this issue ยท 14 comments

kanow commented

If I use the container in a center element, the container will be not rendered as a table. In src/inky.xsl in line 104 my container will be matched and a class float-center is added to the element. It will be not changed to a table element.

The expected behaviour for me were an table with the added class="float-center". It seems that the matching from line 11 ist not working. I don't know why is it so ( I'm not familiar with this xsl stuff).

To fix this problem for the moment I changed the code in line 104 in this way:
<xsl:template match="center/*[position() = 1 and not(contains(@class, 'float-center')) and not('container')]">

This solves the problem for me but I believe that's not the right way. Nevertheless I want to understand why the first matching, to change the container (line 11), is not working.

If you change that line, do tests still pass?

kanow commented

ComponentTest failed.

There were 3 failures:

1) ComponentsTest::testCenterAppliesClassToFirstChild
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0"?>
 <html>
   <body>
     <center>
-      <div align="center" class="float-center"/>
+      <div/>
     </center>
   </body>
 </html>

/home/.../pinky/tests/ComponentsTest.php:433
/home/.../pinky/tests/ComponentsTest.php:21

2) ComponentsTest::testCenterDoesRecurse
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0"?>
 <html>
   <body>
     <center>
-      <center align="center" class="float-center">
-        <p align="center" class="float-center">Hello</p>
+      <center>
+        <p>Hello</p>
       </center>
     </center>
   </body>
 </html>

/home/.../pinky/tests/ComponentsTest.php:433
/home/.../pinky/tests/ComponentsTest.php:40

3) ComponentsTest::testCenterAppliesClassToMenuItems
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0"?>
 <html>
   <body>
     <center>
-      <table align="center" class="float-center menu">
+      <table class="menu">
         <tr>
           <td>
             <table>
               <tr>
-                <th class="float-center menu-item">
+                <th class="menu-item">
                   <a href="#"/>
                 </th>
               </tr>
             </table>
           </td>
         </tr>
       </table>
     </center>
   </body>
 </html>

/home/.../pinky/tests/ComponentsTest.php:433
/home/.../pinky/tests/ComponentsTest.php:69

FAILURES!
Tests: 21, Assertions: 21, Failures: 3.

GridTest is ok.

What happens if you add a div around the <container> in your case? Would it work correctly in that case without your changes?

kanow commented

Yes that works. I removed my patch from the inky.xsl, ComponentTest were ok and the frontend view were also correct. Now it will be a <div align="center" class="float-center"> around the table created. That's what I expect. The question is, why does the first matching of a container ist not working when the second matching on a first element in an center element is true? Ist the second match stronger similar to the behaviour of css selectors?

that's not link to container only because the following would also fails

    public function testCenterAppliesClassToButton()
    {
        $doc =<<<doc
        <center>
            <button href="http://zurb.com">Button</button>
        </center>
doc;
        $expected =<<<doc
        <center>
            <table class="button">
                <tr>
                  <td>
                    <table>
                      <tr>
                        <td><a href="http://zurb.com">Button</a></td>
                      </tr>
                    </table>
                  </td>
                </tr>
            </table>
        </center>
doc;
        $this->assertSameDocuments($doc, $expected);
    }

adding a div fixes it but does fill hacky.
never worked with xsl before, using <xsl:template match="center/*/*[position() = 1 and not(contains(@class, 'float-center'))]"> would make the test pass but would break the other tests in the exact same way than previously reported. any idea @lorenzo ?

Hey @antograssiot unfortunately I'm currently out on holidays, so can't look at this. I suggest using the workaround until a solution can be found. If you want to play with it and add/change the xsl rules to make tests pass, please let me know and I can try to guide you.

I sure can give a try to make test pass I have everything set up localy to run the tests.
if you have a clue on where to start/what to try I will be happy to help, anyway I'm not in a hurry, at all, enjoy your holidays, and myabe see you soon again ๐Ÿ‘‹

@lorenzo any updates on this? I ran into the same issue.

@dpfaffenbauer No updates yet, use the workaround here #2 (comment)

I will try to get down to the bottom of the problem this weekend

@lorenzo ๐Ÿ‘ workaround works fine, but not only for center > container, not for center itself. workaround for center alone is div class=center align=center

alright, thanks for the workaround and your patience. Also, if you have any ideas on how to fix this I'm open to hear. I've had very limited time lately

unfortunately I am very busy with other projects as well and don't have a lot of experience with XSL neither.... Would take me forever to find out what is actually going wrong...

I am happy to test if you find something though.

@dpfaffenbauer can you please verify the pull request fixes your issue?

@lorenzo yep, both cases I had are working now. the one with the container and the other one where I only use center with an img