Open Bug 856371 Opened 11 years ago Updated 11 months ago

[css3-page] implement @page margin boxes

Categories

(Core :: CSS Parsing and Computation, defect)

defect

Tracking

()

REOPENED

People

(Reporter: aleksi.salmela64, Unassigned)

References

(Depends on 2 open bugs, Blocks 2 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

Attached file 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/
OS: Linux → All
Hardware: x86_64 → All
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
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
Attached patch Syntax cleanup (obsolete) — Splinter Review
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?
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)
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)
Blocks: 1117798
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 14 votes.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

@page margins work now. There are more specific bugs for more specific features (like @page :first, etc).

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(emilio)
Flags: needinfo?(aleksi.salmela64)
Resolution: --- → WORKSFORME

Please note that Page-Margin Boxes are very different from page margins, and a specific feature within the CSS3 Paged Media module.

@page { @right-middle { content: "test"; } } margin boxes are not yet implemented (what this issue is about); but @page { margin: 1px; } is (what you mentioned as implemented and used to close the issue).

See https://www.w3.org/TR/css-page-3/#margin-boxes for the reference specification of this feature.

Flags: needinfo?(emilio)

Ah, thanks!

Status: RESOLVED → REOPENED
Flags: needinfo?(emilio)
Resolution: WORKSFORME → ---
Depends on: 1831450
Depends on: 1833466
Duplicate of this bug: 189597
No longer blocks: 1246805
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: