-
Notifications
You must be signed in to change notification settings - Fork 37
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
Repeat <thead> and <tfoot> when splitting table across pages #10
base: phantomjs
Are you sure you want to change the base?
Conversation
This fix for ariya/phantomjs#13324 is an adaptation of ariya/phantomjs#13331 modified for the new submodule linking of QtWebKit used starting 2.1.0. I am not the author of the original patch (@trigveaa is) nor am I competent enough with the QtWebKit codebase to fully endorse it. All I know is that this patch seems to fix the issue for my use-case (aside from the page header overlap I reported in ariya/phantomjs#13331).
Sorry for never responding to my original PR. The reason I didn't resubmit it here is that the overlap issue should probably be investigated before merging this. (I'm not sure if my statement "It seems like the problem with the header overlapping the table was because of some css styles in the page I was testing." is correct) You should probably have kept the individual commits though. In my opinion, they contain useful information. |
Yup, that's right. To be honest, I just used the diff of your original PR (hence the commits being squashed) with the updated path |
if (toRenderBox(section)->y() > max_dy) { | ||
continue; | ||
} | ||
int i = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless int?
Is there any update on this? What is still needed to accept this PR? This is a critical issue for rendering HTML tables in PhantomJS that span multiple pages. I'm not familiar with QtWebKit but am willing to learn and jump in where needed to help resolve this bug. |
Thanks @vitallium, PhantomJS 2.5 work wasn't even on my radar. I can see if the issue is still present in the 2.5 codebase. Where is the dev work happening? Couldn't find a 2.5 branch under PhantomJS. |
I'd love to bring this upstream and see this issue finally fixed, but:
If now is the time to escalate upstream without any further input, almost 4 minor version bumps, and ariya/phantomjs#13324 being mostly dormant; please feel free to do it yourself but I won't be the one bringing it forward. If you do, be kind and post an update here so we can all try to remain sane and continue tracking the issue. Cheers |
Which ubuntu versions are used to compile phantomjs 2.1.0 and 2.1.1 so can apply this patch? |
@assendk Once upon a time I had a docker image that built phantomjs with the patches from a |
This fix for ariya/phantomjs#13324 is an adaptation of ariya/phantomjs#13331 modified for the new submodule linking of QtWebKit used starting 2.1.0.
I am not the author of the original patch (@trygveaa is) nor am I competent enough with the QtWebKit codebase to fully endorse it. All I know is that this patch seems to fix the issue for my use-case (aside from the page header overlap I reported in ariya/phantomjs#13331).
Cheers