Created attachment 731581 [details] testcase Hi. I'm trying to print my extended essay which I have written in html, but it seems that Firefox doesn't support completely CSS3 paged media module. It has some preliminary support to @page rule, but it doesn't support page selectors nor margin boxes. Steps to Reproduce: 1) open the test case from attachment 2 [review]) go to print preview Actual Results: The only thing that is handled correctly is margin property of the @page rule, which doesn't have any page selectors. (at line 12 in source code) Expected Results: - Every page should have red border (line 13) and page number at lower right corner except the first page. (lines 16 & 26) - The first page shouldn't have margins. (line 29) - Every second page should have different margins. (lines 39 & 42) - The 4th page, which is blank, should have a bold text at the top of the page saying "This page is intentionally left blank". (line 34) Paged Media Module: http://www.w3.org/TR/2013/WD-css3-page-20130314/
Created attachment 731586 [details] [diff] [review] Initial patch for margin boxes and page selectors Here is what I have done so far. This is my first patch to Firefox and I have very little experience with C++, so let me know if there is better way to do something in the code. I wasn't sure should I split this patch to smaller parts, so that it would be easier to read it, but I left it like it's now. The patch has new parser for @page rule. Only thing that it doesn't support yet is multiple pseudo-classes in the page selector, but it shouldn't be too hard to add the support for them. I wrote a new parser for page rules, because the the css3 paged media module introduced new extended rule type, which has properties and margin box rules. The patch also has some modifications made to page frame construction. The changes will create the margin boxes for pages. The next step would probably be modifying the CreateGeneratedContent function to support margin boxes. Oh, and by the way when you close the preview window the firefox will crash, because of some memory leaks. I am currently trying to find out where they are, but I haven't had luck so far.
Comment on attachment 731586 [details] [diff] [review] Initial patch for margin boxes and page selectors Wow. Thanks for the patch. A few very quick comments (I'll try to look more during the week): - nsIDOMCSSPageRule.idl doesn't need an IID rev for the addition of comments (though it's also not clear to me why you're adding stuff commented out) - crashes probably aren't going to be the result of memory leaks (your comment above); attach in gdb (or MSVC debugger if windows), get a backtrace ("bt"), and see what's going on. - shouldn't be printf-ing to stdout - should follow local style better, e.g., space after "if" and "for", no space after "!" - why is a new frame class needed for margin boxes? Could we not reuse an existing frame class? - commenting out big things isn't useful -- if things should be removed, remove them, maybe unless it's temporary, in which case there should be a comment explaining why. If there are things you're not sure about, it's generally best to share those to others as "FIXME:" or "REVIEW:" comments. - likewise, a "DO NOT REMOVE" comment isn't useful; comments should explain why. (Most of the code in the source tree shouldn't be removed.) - use of magic numbers like "16" in a bunch of places isn't great; there should be a constant somewhere for the number of margin boxes
Also, it's best to use one bug per feature; otherwise they'll get too cluttered. This one should probably be about margin boxes; others should be added as other dependencies of bug 286443, which tracks implementing css3-page: https://bugzilla.mozilla.org/showdependencytree.cgi?id=286443&maxdepth=1
I also gave you canconfirm and editbugs privileges in Bugzilla so that you can make appropriate changes to bugs; see: https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla
Created attachment 731629 [details] [diff] [review] Syntax cleanup Hi, thanks for your feedback. This patch addresses some of the syntax issues you pointed out. >> - nsIDOMCSSPageRule.idl doesn't need an IID rev for the addition of comments Main reason for it was that I changed PageRule to group rule, which usually has those addRule and removeRule functions. However I found out later that media rule doesn't have dom api, so adding api to page rule was pointless and I commented out all those dom api functions. >> - crashes probably aren't going to be the result of memory leaks The last message from console was this: Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed) There is some other error messages, so the merory leak may not be the reason for the crash. >> - shouldn't be printf-ing to stdout The patch is far from ready, but I will remove those printf lines eventually. >> - should follow local style better Sorry. It seems that in nsCSSParser.cpp file's syntax has space after "!", but everywhere else there isn't space. >> - why is a new frame class needed for margin boxes? Could we not reuse an existing frame class? I didn't know that is possible. The margin box frame can probably be removed. >> - commenting out big things isn't useful Thanks for pointing out those "FIXME:" comments. In the patch most of the commented parts are temporary. > - likewise, a "DO NOT REMOVE" comment isn't useful; That comment was for me I forgot to change it to something more descriptive. - there should be a constant somewhere for the number of margin boxes Okay. Do you know any good place for such constant? Could it be added to nsPageFrame.h?
Created attachment 731949 [details] [diff] [review] New @page rule parser I have finally divided the patch to two parts. The first one has the new parser for @page rule and second one has all the margin box specific code. The first patch is in quite good shape, but it still has some issues like: - The rule specificities aren't taken into account. + The rules are applied in nsStyleSet::ResolvePageStyle function. The problem seems to be that the page rules are added in after the specificities are correctly calculated in nsStyleSet::FileRules function. - Only margin property seems to works. + Second css property that I got working was border properities, but if I have understood correctly, the pageContentFrame will draw over the pageFrame. So you won't see the borders, except if you comment out the ViewportFrame::BuildDisplayList function, but that isn't really solution. - New properties added in Paged Media doesn't work (size and page properties) - Blank pseudo class doesn't work Those problems could be fixed later in some other bug report. The margin box patch is at early stage and it doesn't have anything interesting. The memory leak, which causes firefox to crash, happens because the margin box frames aren't freed. The frame construction stuff is bit too hard to me. I may still try to get the margin boxes working if anyone else isn't interested to make the rest of the patch.
Also see: https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration about configuring mercurial. In particular, having more context and function names shown in the diff makes patches a lot easier to read.
Sorry, I managed to lose track of this when the feedback? request got removed when you obsoleted the patch (really not a great default of Bugzilla's). Are you still interested in working on this? If so, could you let me know what the state of this work is? Do you think the first patch is ready and needs review, or are there specific questions you need answered in order to fix issues with it? If it needs review, I'd suggest you request review from :heycam (and maybe also :SimonSapin).