856371 - [css3-page] implement @page margin boxes

Status: REOPENED

(Core :: CSS Parsing and Computation, defect)

Description

[aleksi.salmela64]

Attachment: 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/

Comment 1

[aleksi.salmela64]

Attachment: 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 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 5

[aleksi.salmela64]

Attachment: 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?

Comment 6

[aleksi.salmela64]

Attachment: 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.

Comment 7

[aleksi.salmela64]

Attachment: Margin box specific code

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

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

Comment 12

[Matthias]

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.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Ah, thanks!

Comment 15

[Andreas Breitschopp]

Chrome has apparently just shipped this crucial feature with version 131:
https://chromestatus.com/feature/5195769732923392

I'm a long-time loyal Firefox user and developer, but this feature is really important to me.
So I'm afraid if that doesn't get added also to Firefox soon, I'll have to do the switch to Chrome as well...