[css3-page] implement @page margin boxes

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
5 years ago
5 months ago

People

(Reporter: aleksi.salmela64, Unassigned, NeedInfo)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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/
(Reporter)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 1

5 years ago
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
Attachment #731586 - Flags: feedback?(dbaron)
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
Blocks: 286443
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: @page rule's implementation is incomplete → [css3-page] implement @page margin boxes
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
(Reporter)

Comment 5

5 years ago
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?
(Reporter)

Comment 6

5 years ago
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.
Attachment #731586 - Attachment is obsolete: true
Attachment #731629 - Attachment is obsolete: true
Attachment #731586 - Flags: feedback?(dbaron)
(Reporter)

Comment 7

5 years ago
Created attachment 731951 [details]
Margin box specific code
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).
Flags: needinfo?(aleksi.salmela64)

Updated

3 years ago
Blocks: 1117798
Blocks: 1246805
You need to log in before you can comment on or make changes to this bug.