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

Resolves ClassCastException for grid layouts #19

Closed
wants to merge 1 commit into from

Conversation

philcali
Copy link

@philcali philcali commented Aug 5, 2012

The Problem

When running ITextRenderer.layout() on a grid formatted html file, you would get:

java.lang.ClassCastException: org.xhtmlrenderer.render.BlockBox cannot be cast to org.xhtmlrenderer.newtable.TableBox

A great example would be to tagsoup http://twitter.github.com/bootstrap/

The Solution

An extra check to block the exception from bubbling to the surface. This simple change resolves:

Thank you for a such a great library!

@zyro23
Copy link

zyro23 commented Jan 14, 2013

whats the status of this? any reason why it has not yet been merged?

@tazmaniax
Copy link

Also interested to know if and when this will be merged. thx

@pbrant
Copy link
Member

pbrant commented Feb 1, 2013

It's not an appropriate fix. There isn't any guarantee that the render tree is in a good state after a runtime exception is thrown. If somebody can boil down the problem to say 10-15 lines of XML, I'll take a look at a proper fix.

@dalelotts
Copy link

The following will reproduce this defect. It seems that just including the twitter bootstrap css is one way to reproduce the problem.

I'm not sure that I agree with this fix (it seems to mask the problem rather than fix it), but I am also confused @pbrant 's comment...there is not exception thrown because this fix prevents the code from ever attempting the cast. Can you provide more detail?

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html>
<head>
    <style type="text/css">
    @page {
        size: 8.5in 11in;
        margin: 0.25in;
    }
    </style>    

    <link href="http://twitter.github.com/bootstrap/assets/css/bootstrap.css" rel="stylesheet">

</head>

<body>

<div class="container">

</div>

</body>
</html>

@haberbyte
Copy link

I boiled it down to this:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html>
<head>
    <style>
      .container:before {
        display: table;
        content: "";
      }
    </style>
</head>

<body>

<div class="container">
</div>

</body>
</html>

It seems to only happen in a combination of display "table" and a content: "" specification inside a :before or :after pseudo class.

@pbrant Would you take a look at this?

@pbrant
Copy link
Member

pbrant commented Feb 21, 2013

Awesome test case reduction!

The problem is actually in BoxBuilder, around line 949 and the subsequent
call to createBlockBox(). The issue is that we always create block-level
generated content as a BlockBox instead of a TableBox. Unfortunately, the
original commit (07d9505) doesn't reference a specific test case so I'm
not sure what this change was fixing, but the I don't think the fix went
far enough. We should be copying the style and setting display: block (see
the uses of createLayoutStyle() in the same file for examples).

@dalelotts I think it's fine actually. I think there was another fix
floating around that caught the exception and carried on.

Jan, if you want to look at this, please don't hesitate! Otherwise I'll
look at this in the next few days (or just commit the workaround hack).
There are a couple of outstanding pull requests so I'll make a pass
through those at the same time and release this as 9.0.2.

Pete

On Thu, Feb 21, 2013 at 4:01 AM, Jan notifications@github.com wrote:

I boiled it down to this:

<style> .container:before { display: table; content: ""; } </style>

It seems to only happen in a combination of display "table" and a content:
"" specification inside a :before or :after pseudo class.


Reply to this email directly or view it on GitHubhttps://github.com//pull/19#issuecomment-13870334.

@haberbyte
Copy link

I believe my latest commit on my fork (See #22 ) fixes this properly.

https://github.com/habermann24/flyingsaucer/commit/1640fcf2d9766a238c45dbb9b7be04ef043455e4

@megamattron
Copy link

Hi everyone - great to see a fix here, any sense of whether a new release incorporating it will be coming out soon? Or should we build a custom version for now? If the latter do you recommend we just cherry pick that commit @habermann24 or use your fork in it's entirety? Thanks!

@haberbyte
Copy link

@megamattron I am using my fork in two of my "production apps", so feel free to use it in its entirety. It's not that many changes after all and i recently rebased all the improvements from upstream. I desperately wanted opacity to work and had issues with some font loading, that's why i did all this.

Not sure if this will get merged. I would appreciate that of course :)

@haberbyte
Copy link

If there is no way to get this stuff merged, then i will probably do another pull request for the bug fixes made, so we can at least get those merged!

@megamattron
Copy link

Great, thanks @habermann24 I'll probably use your fork for now then. Are you loading the bootstrap.css file successfully now with your fork?

@haberbyte
Copy link

Yes it's loading it without the Exception.

@flyingsaucerproject
Copy link
Collaborator

Hi Jan,

It's better to use a topic branch per change. It helps keeps review
process (and eventually merge) more transparent. Your change looks good,
but...

  • We should use four space indents. No tabs anywhere.
  • We should be checking for isTableRow() and isTableSection() too (see
    implementation of createBlockBox())

On Tue, Mar 5, 2013 at 5:36 PM, Jan notifications@github.com wrote:

Yes it's loading it without the Exception.


Reply to this email directly or view it on GitHubhttps://github.com//pull/19#issuecomment-14449821
.

@pbrant
Copy link
Member

pbrant commented Mar 5, 2013

In taking a closer look, I'm not sure this is quite the right approach. We should be modifying the CascadedStyle and deriving a new CalculatedStyle from that (vs. deriving a new CalculatedStyle using the previous one as a parent). I'm closing this pull request. See pull request #30 for my changes here.

@haberbyte
Copy link

@pbrant Thank you very much for your support on this!

@mrserzhan
Copy link

I solve with changing
Was
.row::after {
display: table;
content: " ";
}

Become
.row::after {
display: block;
content: " ";
}

@pelodelfuego
Copy link

@Serzhan

Hello, could you detail your workaround please ?
It would be really helpfull.

@mrserzhan
Copy link

@pelodelfuego You need to find 'display: table;' in each of ::after and replace them to 'display: block;'
It's work for me.

@pelodelfuego
Copy link

@Serzhan

Thanks for your reactivity,

I added:

*::after {
    display: block  !important;
    content: ""     !important;
}

to my print stylesheet.

But it doesn't seems to work... I'll keep digging but maybe you have another idea ?

@pelodelfuego
Copy link

solved by adding:

.col-print-1 {width:8%;  float:left;}
.col-print-2 {width:16%; float:left;}
.col-print-3 {width:25%; float:left;}
.col-print-4 {width:33%; float:left;}
.col-print-5 {width:42%; float:left;}
.col-print-6 {width:50%; float:left;}
.col-print-7 {width:58%; float:left;}
.col-print-8 {width:66%; float:left;}
.col-print-9 {width:75%; float:left;}
.col-print-10{width:83%; float:left;}
.col-print-11{width:92%; float:left;}
.col-print-12{width:100%; float:left;}

Thanks for your help !

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

Successfully merging this pull request may close these issues.

None yet

10 participants