Last Comment Bug 115199 - @page in CSS2 not implemented
: @page in CSS2 not implemented
Status: RESOLVED FIXED
parity-Opera, parity-IE8, CSS2.1
: css2, dev-doc-complete, helpwanted, testcase
Product: Core
Classification: Components
Component: Printing: Output (show other bugs)
: Trunk
: All All
: -- enhancement with 72 votes (vote)
: mozilla19
Assigned To: Brendan Dahl [:bdahl]
:
Mentors:
: 72321 136316 174954 248573 275782 314902 331451 349759 541914 (view as bug list)
Depends on: 811827 812344 827591 829817
Blocks: 35615 css2.1-tests 752787 811391 1241378
  Show dependency treegraph
 
Reported: 2001-12-13 22:14 PST by Joseph T. Duncan
Modified: 2016-02-10 09:38 PST (History)
96 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial pass at getting the rudamentary parts working. (22.14 KB, patch)
2002-05-08 15:09 PDT, rods (gone)
no flags Details | Diff | Splinter Review
patch (16.44 KB, patch)
2002-07-02 09:33 PDT, rods (gone)
no flags Details | Diff | Splinter Review
a sketch of the interface changes for implementing @page-rule support in the style system (6.20 KB, patch)
2002-08-19 17:07 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
a sketch of the interface changes for implementing @page-rule support in the style system (6.19 KB, patch)
2002-08-19 17:15 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch for Page rule parsing, DOM support and nsPageStyleContext computation (73.98 KB, patch)
2002-10-07 09:28 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
This is the part of the patch we all always forget about when cvs diff'ing :-/ sorry... (1.91 KB, patch)
2002-10-07 19:18 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
full patch (216.84 KB, patch)
2002-11-21 15:39 PST, rods (gone)
no flags Details | Diff | Splinter Review
full final patch for review (232.34 KB, patch)
2002-12-02 08:41 PST, rods (gone)
no flags Details | Diff | Splinter Review
testcase (1.01 KB, text/html)
2003-04-08 01:05 PDT, Daniel Wang
no flags Details
WIP-1 (27.38 KB, patch)
2011-10-10 23:58 PDT, Michael Ventnor
dbaron: feedback+
Details | Diff | Splinter Review
WIP-1 rendering (20.20 KB, patch)
2011-10-18 02:31 PDT, Michael Ventnor
roc: feedback+
Details | Diff | Splinter Review
WIP-1-rebase (28.22 KB, patch)
2012-06-04 12:32 PDT, Julian Viereck
no flags Details | Diff | Splinter Review
WIP-1 rendering-rebase (14.40 KB, patch)
2012-06-04 12:34 PDT, Julian Viereck
no flags Details | Diff | Splinter Review
WIP-2-Rendering (19.97 KB, patch)
2012-06-15 16:12 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
WIP-2-Style (40.54 KB, patch)
2012-06-15 16:24 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
WIP-2-Rendering (20.64 KB, patch)
2012-07-09 17:41 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
WIP-2-Rendering (29.66 KB, patch)
2012-07-18 14:45 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
WIP-2-Rendering (22.25 KB, patch)
2012-07-24 09:03 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
Part 1 - rendering v1 (16.21 KB, patch)
2012-08-31 14:28 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
Part 2 - style v1 (38.95 KB, patch)
2012-08-31 14:35 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
Part 1 - rendering v3 (18.55 KB, patch)
2012-09-18 15:48 PDT, Brendan Dahl [:bdahl]
roc: review+
Details | Diff | Splinter Review
Part 2 - style v3 (39.02 KB, patch)
2012-09-18 15:50 PDT, Brendan Dahl [:bdahl]
dbaron: review+
Details | Diff | Splinter Review
Part 1 - rendering v4 (18.54 KB, patch)
2012-10-18 13:54 PDT, Brendan Dahl [:bdahl]
bdahl: review+
Details | Diff | Splinter Review
Part 2 - style v4 (40.99 KB, patch)
2012-10-18 13:59 PDT, Brendan Dahl [:bdahl]
dbaron: review+
Details | Diff | Splinter Review
Part 2 - style v5 (41.74 KB, patch)
2012-10-29 17:32 PDT, Brendan Dahl [:bdahl]
no flags Details | Diff | Splinter Review
Part 2 - style v6 (41.91 KB, patch)
2012-10-30 09:25 PDT, Brendan Dahl [:bdahl]
dbaron: review+
Details | Diff | Splinter Review

Description Joseph T. Duncan 2001-12-13 22:14:11 PST
I'm trying to use the @page selector from CSS2 to default the printing of my
webpage to landscape. I've used w3.orgs CSS validator to make sure that the
style sheet and implementation of the @page selector are correct, but NS 6.2.1
and MZ 0.9.6 won't default to landscape. It works correctly on IE 5.5 :(

Here is my minimilized test case:
http://www.engr.orst.edu/~duncan/bugtestcase.html
http://www.engr.orst.edu/~duncan/css.css

Also here are links to everything that I found to help me get this to work:
http://www.course.com/downloads/newperspectives/crweb2/dhtml/T7.html#Printing
http://www.w3.org/TR/CSS2/page.html#page-box
Comment 1 Joseph T. Duncan 2001-12-14 10:31:52 PST
Unfortunatly my M$ cohort had a setting wrong and it wont work in any version of
IE either. 
Comment 2 Joseph T. Duncan 2001-12-14 10:33:35 PST
Opera 6.0 seems to work
Comment 3 Andrew Hagen 2002-03-19 16:56:00 PST
Could you describe the difference between the actual results and expected results?
Comment 4 Joseph T. Duncan 2002-03-19 23:56:53 PST
If you print out my test case first normaly and then print it out "manualy"
selecting landscape layout. You will find that the second line in my test case
will be cut off and not print on the page correctly. When manualy selecting
landscape it will print out correctly. The use of @page is suppost to
automagicly switch the printing output to landscape when it is deffind as it is
in my css.css
Comment 5 Joseph T. Duncan 2002-03-20 00:04:35 PST
also when you go to print preview you will see that the second line of text runs
off the print preview page where the css should have told the browser to switch
to landscape layout
Comment 6 Andrew Hagen 2002-03-20 05:15:11 PST
Confirming on Build ID: 2002031903 (0.9.9+) Windows 98.

Expected results: CSS @page information affects the page setup options,
including whether the document should be printed in landscape or portrait format.

Actual results: It does not.
Comment 7 rods (gone) 2002-03-26 09:00:45 PST
We have not implemented much more than the page break, we aren't even paring the
@page rule yet.
Comment 8 Andrew Hagen 2002-04-09 15:01:20 PDT
*** Bug 136316 has been marked as a duplicate of this bug. ***
Comment 9 rods (gone) 2002-05-08 15:09:16 PDT
Created attachment 82852 [details] [diff] [review]
Initial pass at getting the rudamentary parts working.

This implements the @page and supports "size" and "margin", but doesn't support
and Pseudo names (:first, :right, :left)
Comment 10 Marc Attinasi 2002-05-09 13:54:43 PDT
Rod, care to explain your design for this?
Comment 11 rods (gone) 2002-05-09 16:03:36 PDT
The design is fairly simple. I parse the rule(s) for all @page rules whether
they have pseudo names or not.

Then in the Document viewer when doing Printing or PrintPreview, I make a clone
of the current PrintSettings (PS) and then serach the @page rules for any any
rules that do not have a pseudo name. Then I check to see if the size attr is
setting orientation or has a size. The I either set the orientation or set the
size of the page, then I get the margins and apply them to the PS. This way the
percentage margin works.

Then I let printing continue and the print dialog will appear and contain all
the info that is intended for how that page prints out.

After if prints, the cloned PS gets destroyed so that any special setting from
the page for printing will not be remembered.
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-05-20 20:05:21 PDT
Comment on attachment 82852 [details] [diff] [review]
Initial pass at getting the rudamentary parts working.

>? content/html/style/src/nsICSSPageRule.h

Seeing the contents of this file would be extremely helpful to reviewing the
patch.	If you do a cvs add in your tree, then cvs diff -N will show the
modified files as well.

>+void DocumentViewerImpl::SetupCSSPageRules(PrintData* aPrt, nsIDocument* aDoc)

This doesn't do cascading anywhere near correctly.  At first glance, I'd
think we should really add functions to resolve page style contexts based
on the normal cascading rules (adding PageRulesMatching to
nsIStyleRuleProcessor,
and using WalkRuleTree, style contexts, and rule nodes as normal, although
perhaps with some modifications).  I'd have to think about this a bit more,
though.  The current implementation is definitely wrong, though.

>Index: content/html/style/src/makefile.win

You'll need to change Makefile.in and MANIFEST too.

>Index: content/html/style/src/nsCSSParser.cpp
> PRBool CSSParserImpl::ParsePageRule(PRInt32& aErrorCode, RuleAppendFunc aAppendFunc, void* aData)

It would be nice if this parsed page identifiers as well as pseudo-classes.
At the very least, it should probably do an early return

> {
>-  // XXX not yet implemented
>+  if (!GetToken(aErrorCode, PR_TRUE)) {
>+    REPORT_UNEXPECTED_EOF();
>+  }
>+  nsAutoString pseudoClass;
>+
>+  if (eCSSToken_Symbol == mToken.mType) {
>+    PRUnichar symbol = mToken.mSymbol;
>+    if ('{' == symbol) {
>+      UngetToken();
>+    } else if (':' != symbol) {
>+      REPORT_UNEXPECTED_TOKEN(
>+        NS_LITERAL_STRING("Expected ',' in media list but found"));

How about "Expected page selector or declaration block"?

>+      UngetToken();

return PR_FALSE;

(You need to return false to avoid accepting rules like
"@page ; { margin: 1in; }".)

>+    } else  {
>+      if (!GetToken(aErrorCode, PR_TRUE)) {
>+        REPORT_UNEXPECTED_EOF();

return PR_FALSE;

(Why not?)

>+      }
>+      if (eCSSToken_Ident == mToken.mType) {
>+        pseudoClass  = mToken.mIdent;
>+      }

else {
  REPORT_UNEXPECTED_TOKEN(NS_LITERAL_STRING("Expected page pseudo-class but
found"));
  return PR_FALSE;
}

(You need to return false to avoid accepting rules like
"@page :; { margin: 1em; }".)

>+
>+    }
>+  }

else {
  REPORT_UNEXPECTED("Mozilla does not support page selectors");
  return PR_FALSE;
}

(You need to return false to avoid accepting rules like
"@page foo { margin: 1em; }" and treating them as if they
didn't have page selectors.)

>+
>+  nsCSSDeclaration* declaration = ParseDeclarationBlock(aErrorCode, PR_TRUE);
>+  if (!declaration) return PR_FALSE;
>+
>+  nsCOMPtr<nsICSSPageRule>  rule;
>+  NS_NewCSSPageRule(getter_AddRefs(rule), ToNewUnicode(pseudoClass), declaration);
>+  if (rule) {
>+    (*aAppendFunc)(rule, aData);
>+    return PR_TRUE;
>+  }
>+  else {  // failed to create rule, backup and skip block
>+    UngetToken();
>+  }
>+
>   return PR_FALSE;

Why the UngetToken on failure here?  That seems wrong, since Parse
DeclarationBlock should do the appropriate skipping on failure, and this
will cause you to back up and find an unmatched brace or loose semicolon,
which could then cause too much to be ignored according to the CSS parser
error handling rules.  This could also be simplified (and the main control
flow un-indented) to:

if (!rule) {
  /* UngetToken() */
  return PR_FALSE;
}
(*aAppendFunc)(rule, aData);
return PR_TRUE;

> }
> 


>Index: content/html/style/src/nsCSSRules.cpp
>+class CSSPageRuleImpl : public nsCSSRule,
>+                        public nsICSSPageRule,
>+                        public nsIDOMCSSRule

This would need significant changes to support cascading correctly (see
above).  Based on the current implementation (but not on what I suggest)
I don't like the implications of the "is-a nsIStyleRule", but the other
rules do that too.  I've stuck some appropriate NS_NOTREACHED in the
nsCSSRule base class in my own tree to deal with that...
Comment 13 rods (gone) 2002-07-02 09:33:26 PDT
Created attachment 89939 [details] [diff] [review]
patch

This patch doesn't include the DocumentViewer changes, same as previous patch.
But it cleans up the parsing and includes the nsICSSPageRule.h
Comment 14 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-07-02 16:25:16 PDT
It didn't include the DOM changes either.  Did you mean to include them?
Comment 15 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-08-19 17:07:46 PDT
Created attachment 95927 [details] [diff] [review]
a sketch of the interface changes for implementing @page-rule support in the style system

This patch could be the beginning of a design discussion about how to implement
this.  It's basically a rough sketch of the changes to interfaces that I would
want to make in order to implement @page-rule support in the style system.  It
would probably be a good idea if hyatt and/or bzbarsky had a look at this
before someone goes off and implements it.
Comment 16 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-08-19 17:15:44 PDT
Created attachment 95930 [details] [diff] [review]
a sketch of the interface changes for implementing @page-rule support in the style system

Oops, that didn't have some of the changes that I thought I made.  Here's the
real one.
Comment 17 rods (gone) 2002-08-20 07:27:25 PDT
I am studying your patch, what is "nsPageRuleData" what function does it perform?
Comment 18 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-08-20 07:30:20 PDT
nsPageRuleData is something that I forgot to remove in the first patch -- it's
the main thing I corrected in the second one.
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-08-20 07:31:38 PDT
Comment on attachment 95930 [details] [diff] [review]
a sketch of the interface changes for implementing @page-rule support in the style system

And I still missed one line:

>+  PageRuleProcessorData(nsIPresContext* aPresContext,
>+                        Side aSide,
>+                        PRUint32 aPageNumber,
>+                        nsIAtom* aPageName,
>+                        nsPageRuleData* aDataSink)

that last line should be |nsPageRuleWalker* aRuleWalker| (with the
corresponding change to the constructor).
Comment 20 Daniel Glazman (:glazou) 2002-10-07 09:28:24 PDT
Created attachment 102005 [details] [diff] [review]
patch for Page rule parsing, DOM support and nsPageStyleContext computation

To get a page style context, call

    ResolvePageStyleContextFor(nsPageStyleContext** aResult)

and use the utility methods on the nsPageStyleContext

    nsresult GetSizeAsKeyword(PRInt32 * aCSSKeyword);
    nsresult GetIsSizeSet(PRBool * aIsSet);
    nsresult GetIsMarginSet(PRBool * aIsSet);
    nsresult GetSize(nsSize * aSize);
    nsresult GetMargin(nsMargin * aMargin, const nsSize & aPageSizeInTwips);
    nsresult GetMarginAsKeyword(PRInt32 * aCSSKeyword);

I chose the simplest way of computing the page style context even if it's not
the most efficient. Given the number of page rules we'll have to deal with,
the impact is not big.
Comment 21 Boris Zbarsky [:bz] 2002-10-07 10:56:37 PDT
A few comments based on a quick skim (I'll take a closer look later):

1)  Why not put @page in nsCSSRules.cpp?  Not that I really mind having a separate
    file -- it's clearer, imo.  But was there a reason we had a single file
    initially?
2)  Use MPL for new files, not NPL?
3)  nsPageStyleContext is not refcounted and is not an nsISupports?  Why?  That
    seems like it'll be a pain in actual use, since users will have to deallocate
    manually...
Comment 22 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-10-07 17:07:08 PDT
I'd like to deCOMify nsStyleContext -- why not start nsPageStyleContext there? 
How is |delete| (or a virtual |Destroy| if needed) harder than |Release|?
Comment 23 Boris Zbarsky [:bz] 2002-10-07 17:59:36 PDT
Release() is easy to trigger automatically via nsCOMPtr.  If nsAutoPtr works
with nsPageStyleContext, then I withdraw item #3.
Comment 24 Daniel Glazman (:glazou) 2002-10-07 19:16:19 PDT
Boris: 

1) I hate nsCSSRules.cpp. I find it hard to read and have always wanted to split
   it into one file per type of rule.

2) oooops

3) given the fact nsPageStyleContext will initially be used only by the Print
   Engine, in a very specialized area and almost only by Rod Spears, I thought we
   can make the economy of the COMification here.
Comment 25 Daniel Glazman (:glazou) 2002-10-07 19:18:16 PDT
Created attachment 102100 [details] [diff] [review]
This is the part of the patch we all always forget about when cvs diff'ing :-/ sorry...
Comment 26 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-10-17 04:46:23 PDT
*** Bug 174954 has been marked as a duplicate of this bug. ***
Comment 27 Michal Kubecek 2002-10-25 05:59:00 PDT
The target milestone should be probably changed to mozilla1.2final since 1.2b is
out.
Comment 28 Sören 'Chucker' Kuklau (gone) 2002-10-25 06:10:27 PDT
Re: Comment #27 From Michal Kubecek 2002-10-25 05:59
> The target milestone should be probably changed to mozilla1.2final since 1.2b is
> out.

The Target Milestone is an estimated target date from the assignee. The Target
Milestone is *not* the field for the milestone where the bug will definitly be
fixed.
Comment 29 Malcolm Rowe 2002-11-15 04:06:15 PST
It's probably worth mentioning that the @page rule has been removed from CSS 
2.1 (due, I believe, to lack of implementations).
Comment 30 Daniel Glazman (:glazou) 2002-11-15 04:31:47 PST
> It's probably worth mentioning that the @page rule has been removed from CSS 
> 2.1 (due, I believe, to lack of implementations).

(a) that does not block us if we want to implement CSS 2
(b) see CSS3 Paged Media module http://www.w3.org/TR/css3-page
Comment 31 Sören 'Chucker' Kuklau (gone) 2002-11-15 04:36:18 PST
Unless I'm very mistaken, CSS 2.1 is a simplified CSS 2 that removes CSS 2.0
features that were hardly ever implemented by common browsers, so that browser
vendors can try and at least support the whole CSS 2.1.

As Gecko is supposed to do 2.0 (and, later, several 3.0 modules), not 2.1, this
is moot anyway.
Comment 32 Daniel Glazman (:glazou) 2002-11-15 05:03:32 PST
CSS 2.1 lists all the features of CSS 2 that CAN pass the Candidate Recommendation
exit criteria, including two interoperable implementations. It also fixes some
known bugs in CSS 2 and clarifies some parts. The CSS 2 spec stil stands, still
is a call for implementation, still is the source for CSS 3.
Comment 33 rods (gone) 2002-11-21 15:39:10 PST
Created attachment 107083 [details] [diff] [review]
full patch 

full patch
Comment 34 rods (gone) 2002-12-02 08:41:33 PST
Created attachment 107911 [details] [diff] [review]
full final patch for review

This should be the full patch for review:
Here is what it doesn:
1) The first half of the patch (so to speak) is all CSS work for implmenting
the the parsing and creation of the CSS @page rules
2) Th second half of the patch are the changes necessary for making it all work
with the PrintEngine (PE).
3) It would have all been small and straight forward  up until the point where
you you can have a frameset with different rules for each document. Meaning one
doc can print landscape and the other doc can print portrait.

It was always a desirable feature of our PE that mulitple file printed to the
same job. This became a problem for because print drivers do not always support
different page orientations within the same job. So I have to make the PE be
able to print each document as a separate job, from beginning to end. Meaning
from the reflow step all the way thru.

So all these changes to the PE enable it to still print non-css page documents
as a single print job and CSS page docs as mulitple print jobs.

How the CSS @page rules interface with the PE is as follows:

First the PE determines if there are any @page rule that need to be considered.
If so, it enbables a new "Use CSS Page Settings" checkbox in the print dialog.
If the user print with the checkbox on, it will use the print settings as
"overrides" to anything the user set in the dialog. If the user clicks it off,
then it print normally.

As I mentioned before, everything is very straight forward when printing a
single document or a frameset "AsIs." It get complicated when printing "Each
Doc Sep" when the CSS page rules are used.

Note that the Print Dialog will not reflect any of the rule settings.
Comment 35 rods (gone) 2002-12-02 08:48:01 PST
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

dcone and jst, please r and sr the non CSS portions of this bug, I will get
someone else to do that.
Comment 36 Daniel Glazman (:glazou) 2002-12-03 04:12:01 PST
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

>Index: content/shared/public/nsPageStyleContext.h
>===================================================================
>
> ...
>
>+static DeletePageStyleContext(void * aElement, void * aData) {
>+  delete (nsPageStyleContext*) aElement;
>+}

gcc 3.2 on RedHat8 chokes on this. You need to specify a type
for DeletePageStyleContext().

>Index: content/base/src/nsPrintEngine.h
>===================================================================
>
> ...
>
@@ -66,6 +67,7 @@
> #include "nsIDocumentViewer.h"
> #include "nsIDocumentViewerPrint.h"
> 
>+#define MOZ_LAYOUTDEBUG
> #ifdef MOZ_LAYOUTDEBUG
> #include "nsIDebugObject.h"
> #endif

Do you want to always be in LAYOUTDEBUG or did you just forget to
remove that #define ?
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2002-12-03 16:27:42 PST
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

- In nsPrintEngine.cpp:

+class MultipleFileNameMgr {
+public:
+  MultipleFileNameMgr() : mCount(0), mFileName(nsnull) {}
+  ~MultipleFileNameMgr();
+
+  void SetBaseName(const nsXPIDLString& aName);

Don't use string implementation classes in method signatures when we have
abstract interfaces. I.e. use const nsAString& here.

+  const PRUnichar* GetNextFileName();
+
+protected:
+  nsXPIDLString mBaseName;
+  PRInt32	 mCount;
+  PRUnichar*	 mFileName;

Why not make this an nsXPIDLString (or an nsString) too?

+// Assumes ".ps" extensions

Is this safe, i.e. is it enforced by code?

+void 
+MultipleFileNameMgr::SetBaseName(const nsXPIDLString& aName)
+{
+  mCount    = 0;
+  mBaseName = aName;
+  mBaseName.SetLength(nsCRT::strlen(aName.get())-3);

Don't re-calculate the string length here, use aName.Length().

- In MultipleFileNameMgr::GetNextFileName():

+  if (mBaseName.Length() == 0) {

Use mBaseName.IsEmpty().

+  nsCString tmpName;
+  tmpName.AssignWithConversion(mBaseName.get());
+  tmpName.AppendInt(mCount++);
+  nsAutoString strName;
+  strName.AssignWithConversion(tmpName.get());
+  strName.AppendWithConversion(".ps");
+  mFileName = ToNewUnicode(strName);
+  return mFileName;

How about:

+  nsAutoString tmpName(mBaseName);
+  tmpName.AppendInt(mCount++);
+  mFileName = tmpName + NS_LITERAL_STRING(".ps");
+  return mFileName.get();

... once mFileName is an nsString or nsXPIDLString? No character conversion, no
extra malloc (inside the nsCString).

- In nsPrintEngine::DisplayPrintDialog():

+  PRBool cachedSilently;
+  aPS->GetPrintSilent(&cachedSilently);
+  aPS->SetPrintSilent(aPrintSilently);

There's a bunch of places where you do an early return and not set back the
cachedSilently value, is that ok?

- In nsPrintEngine::CreatePrinterDevices():

+      nsCOMPtr<nsIDeviceContext> dx;
+      rv = mPresContext->GetDeviceContext(getter_AddRefs(dx));
+      if (NS_SUCCEEDED(rv)) {

GetDeviceContext() doesn't, and shouldn't, return an error code just because
there is no device context. Check for a non-null dx here (and IMO it's fine to
ignore the return code).

+	 rv = dx->GetDeviceContextFor(devspec, *getter_AddRefs(aPO->mPrintDC));
+	 if (NS_SUCCEEDED(rv)) {

Same thing goes here, IMNSHO.

- In nsPrintEngine::GetFileNameForPrintSettings():

+  PRUnichar* fileName;
+  aPS->GetToFileName(&fileName);
+
+  if (fileName) {
...
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    nsMemory::Free(fileName);

The NS_ENSURE_SUCCESS() leaks fileName, change fileName over to an
nsXPIDLString.

- In nsPrintEngine::PerformPrepareDocument():

+  PRUnichar * docTitleStr;
+  PRUnichar * docURLStr;
...
+  if (docTitleStr) nsMemory::Free(docTitleStr);
+  if (docURLStr) nsMemory::Free(docURLStr);

Any reason not to make those nsXPIDLString's? Same thing in
nsPrintEngine::PerformBeginDocument().

Other than that, sr=jst for the non-CSS changes in this patch.
Comment 38 dcone (gone) 2002-12-06 11:26:34 PST
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

r=dcone.
Comment 39 Boris Zbarsky [:bz] 2003-04-06 10:01:52 PDT
So.. what's the state of this patch?
Comment 40 Roland Mainz 2003-04-06 15:03:55 PDT
Boris Zbarsky wrote:
> So.. what's the state of this patch?

It's rotting around... ;-(((
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2003-04-06 15:16:16 PDT
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

I should probably review it.
Comment 42 Daniel Wang 2003-04-07 23:51:04 PDT
*** Bug 72321 has been marked as a duplicate of this bug. ***
Comment 43 Daniel Wang 2003-04-08 01:05:55 PDT
Created attachment 119789 [details]
testcase
Comment 44 Robert Kaiser 2003-07-20 12:25:06 PDT
so what's happening to this... could you perhaps ask somebody else for sr if
dbaron doesn't come around to do it?

I guess it may even suffer from bitrot already :(
Comment 45 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2003-07-20 14:28:38 PDT
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

Clearing dcone's review+ since (1) I don't think a review+ for a patch of this
complexity without any comments makes any sense and (2) it needs module owner
review.
Comment 46 Christian Ehrlicher 2004-03-02 22:20:54 PST
Just wanted to know what's with this bug.
I think it's a really nice feature for printing and it is official CSS2 afaik.
Comment 47 Paul Kopacz 2004-04-23 08:56:13 PDT
Does anyone know the status of this patch/issue?  It is the only thing keeping
our entire office on MSIE (and an ungodly ActiveX margin setting plugin).  Thanks.
Comment 48 Brooks Williams 2004-05-03 15:29:44 PDT
This was slated for 1.3a?  Is there a chance we'll get this fixed soon?  It
looks like a patch was submitted, but that was a year and a half ago now...  
Comment 49 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2004-06-24 18:12:03 PDT
*** Bug 248573 has been marked as a duplicate of this bug. ***
Comment 50 Daniel Kasak 2004-06-24 20:11:06 PDT
Sorry about adding a duplicate bug report ( Bug 248573 ). I searched for @page
on the search page, honest...

Does anyone know what the plans are re: this bug. I see that most of the
activity is from 2 years ago.

I'm developing a report generator, and I *really* need to be able to tell the
browser to print in landscape mode. Does anyone know:

1) If @page support is planned
2) If there is another way to tell Mozilla that the page should be printed in
landscape mode.
Comment 51 Daniel Kasak 2004-09-27 21:11:29 PDT
Seriously ... is anyone even remotely interesting in this bug anymore?

Having a "Yeah it's still being considered" or a "no, and get lost" answer from
a developer would at least let those interested know where they stand.
Comment 52 Jeff Walden [:Waldo] (remove +bmo to email) 2004-09-27 21:38:13 PDT
(In reply to comment #51)
> Seriously ... is anyone even remotely interesting in this bug anymore?

Yes, people are interested in it.  Yes, people are also interested (often more
interested, it seems) in other bugs.  Yes, said people are working on other bugs
instead of this one right now, although that could change any day without prior
notice.

And in answer to two unspoken questions:

Yes, people would be more interested in it if you (or a proxy, i.e. a hired
third-party coder) came forward with a patch since you're interested in it so
much.  Yes, they'd be interested enough to work with you to get the bug fixed if
you were willing to accept feedback on the patch, incorporate it into any
revisions, and drive it into the trunk.

Please respond via private email if you wish to continue this discussion. 
Bugzilla is not the place for advocacy discussions.
Comment 53 Justin Wood (:Callek) 2004-12-06 21:35:33 PST
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

2 years, and 60 thousand attachments later;

David is this r? out of date due to severe rot?
Comment 54 Ivar Snaaijer 2005-06-21 07:35:45 PDT
CC spam, and to reitterate that this is also a problem for Firefox (1.0.4)
Comment 55 Daniel Kasak 2005-06-21 15:30:59 PDT
Since there is not progress on this bug, those interested in using @page for
designing printable reports might want to check out my open-source project,
PDF::ReportWriter, which is available at:
http://entropy.homelinux.org/axis_not_evil/
Comment 56 PikeUK 2005-09-01 01:25:39 PDT
*** Bug 306628 has been marked as a duplicate of this bug. ***
Comment 57 David E. Ross 2005-09-16 08:50:05 PDT
Reference comment #7:  

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.11) Gecko/20050728

According to Boris Zbarski:
"We only implement page-break-before and page-break-after, and the only value we 
really recognize is 'always'."  

See <URL:http://www.oakparkfoundation.org/grant_log.html>, which has a long
table after a short introduction.  The table begins printing on a new page
despite the use of 
  table { page-break-inside:  auto;
	  page-break-before: avoid }

Changing Summary.  CSS2.1 has replaced CSS2.  Further, this is a general problem
with the implementation of Paged Media and not merely with the @page rule.   
Comment 58 Robert Kaiser 2005-09-16 09:58:35 PDT
David: I don't understand your subject change. First, this is about much more
than the page-break property, and second, I believe we still don't implement
@page at all (including some page break stuff, but mainly page margins, page
size, etc. which is what this bug is all about).
Comment 59 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2005-09-16 10:08:22 PDT
This bug, in fact, has almost nothing to do with the page-break-* properties.
Comment 60 David E. Ross 2005-09-17 10:56:10 PDT
I have submitted new bug report 308970 relative to page breaks.  
Comment 61 Christian :Biesinger (don't email me, ping me on IRC) 2005-11-03 02:41:10 PST
*** Bug 314902 has been marked as a duplicate of this bug. ***
Comment 62 mpj17 2006-02-27 16:58:09 PST
It appears that the "@page" rule (page boxes) has made its way back into CSS 2.1, sometime between November 2001 and June 2005: http://www.w3.org/TR/CSS21/page.html#page-box

It would be nice to have support for margins, at least.
Comment 63 Not interested in Mozilla any more 2006-02-28 04:15:41 PST
Page orientation and paper size would seem to be very important also.
Comment 64 Jo Hermans 2006-03-23 04:22:11 PST
*** Bug 331451 has been marked as a duplicate of this bug. ***
Comment 65 Ben 2006-03-24 18:35:10 PST
it seems that it would be most appropriate as an implementation method to parse @page rules in css as extra rules for printing.  Basically, treat them as though they were on a separate stylesheet for printed media, since that's all we need.

We should make sure to implement not only margins etc for the page itself, but also that the selector '@page foo' applies on paged media, which as far as I'm aware, for mozilla browsers will just be printing, right?
Comment 66 Phil Ringnalda (:philor, back in August) 2006-08-22 15:15:08 PDT
*** Bug 349759 has been marked as a duplicate of this bug. ***
Comment 67 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2007-03-28 08:03:38 PDT
So, the questions I'd liked to have answered about this patch (and hopefully would have asked if it hadn't already been bitrotted and ownerless when I made the review request to myself) are:

 * What behavior is it trying to implement?  Parts of the patch support page selectors, but in the end does it just ignore them and use only @page rules without them?

 * What does it do with the information in the @page rules?  Does it implement margin and margin-* ?  Does it implement size?

 * How does it make the page size information in the @page rules interact with the information on available paper sizes from the printer driver?

 * How does it interact with the user interface?  It seems there are some pretty complicated user interface issues about how to reflect what's going to happen to the user and how to handle settings that the user makes.  (And it seems this gets even more complicated in the context of printing separate frames, or in the context of page selectors.)
Comment 68 Samuel Sidler (old account; do not CC) 2007-04-03 16:41:54 PDT
*** Bug 275782 has been marked as a duplicate of this bug. ***
Comment 69 Joshua Cranmer [:jcranmer] 2007-11-21 15:07:06 PST
(In reply to comment #67)
>  * What behavior is it trying to implement?  Parts of the patch support page
> selectors, but in the end does it just ignore them and use only @page rules
> without them?

The patch for nsCSSParser says:
if (!pageName.IsEmpty() || !pagePseudo.IsEmpty()) {
  REPORT_UNEXPECTED(NS_LITERAL_STRING("page names and page pseudo-classes are not supported"));
  UngetToken();
  return PR_FALSE;
}

So it doesn't even try to handle anything other than vanilla @page blocks.

It appears to only accept margin-* and size for rules (the latter is not in CSS 2.1).

>  * What does it do with the information in the @page rules?  Does it implement
> margin and margin-* ?  Does it implement size?

As far as I can tell, the size property does not accept values of type <page-size>, but yes it does handle both of these attributes. These values are ultimately handled by nsPageStyleContext and used in nsPrintEngine

>  * How does it make the page size information in the @page rules interact with
> the information on available paper sizes from the printer driver?

Short answer: they don't appear to do so. None of the named stuff is used, everything else gets shunted off to nsPrintSettings to handle.

>  * How does it interact with the user interface?  It seems there are some
> pretty complicated user interface issues about how to reflect what's going to
> happen to the user and how to handle settings that the user makes.  (And it
> seems this gets even more complicated in the context of printing separate
> frames, or in the context of page selectors.)

The UI appears to be a checkbox in the print dialog that says "Use CSS page settings in document"; for what little exists in CSS 2.1, this should be sufficient. Implementing this a la CSS 3 LC would require more settings.

Parting note:
This is the first time I've looked into CSS code; I also believe that the person who wrote this included a little more than code to implement @page...

My question is this: should we focus on just getting CSS 2.1 first and in the tree or go all out and include the current WD for CSS 3 as well?
Comment 70 Garth Wallace 2007-11-21 18:57:31 PST
(In reply to comment #65)
> We should make sure to implement not only margins etc for the page itself, but
> also that the selector '@page foo' applies on paged media, which as far as I'm
> aware, for mozilla browsers will just be printing, right? 

Don't forget Print Preview.

(In reply to comment #65)
> My question is this: should we focus on just getting CSS 2.1 first and in the
> tree or go all out and include the current WD for CSS 3 as well?

My preference is 2.1 before 3. Isn't 2.1 a Candidate Recommendation, and 3 a Working Draft?
Comment 71 Ryan Jones 2008-06-23 03:22:24 PDT
Is there any chance that this should be wanted/blocking Gecko 1.9.1?
Comment 72 d 2009-06-07 02:38:47 PDT
Gecko is closing up to reach full CSS2 compability, but this has been left out. Something to look into for 1.9.2?
Comment 73 John P Baker 2010-01-25 01:55:12 PST
*** Bug 541914 has been marked as a duplicate of this bug. ***
Comment 74 Paul 2010-01-25 02:56:03 PST
How i can see this problem not solved since Joseph T. Duncan have been maked his first post. 6 year passing.. and no release with full support of CSS 2, i don't whant 2 think about CSS3 it's make me unhappy. Unfortunately i can't make anything usefull 2 help developers with thoose hard work
P.S Sorry for my english and offtopic, now i will type, that my users have to switch to Landscape manually))
Comment 75 David E. Ross 2010-03-24 18:18:20 PDT
The chart at <https://developer.mozilla.org/En/Mozilla_CSS_support_chart> indicates (1) that ALL of CSS 2.1 is implemented and (2) that @page is a CSS 3 feature.  However, @page is specified in Section 13.2 of the current (8 Sep 09) "W3C Candidate Recommendation" for CSS 2.1.  

This is the second case I have discovered where the "Mozilla CSS support chart" improperly attributed a CSS 2.1 feature to CSS 3, thereby making the false assertion that all of CSS 2.1 is implemented.  The first case is the page-break feature cited in bug #132035.
Comment 76 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-03-24 18:41:49 PDT
(In reply to comment #75)
> The chart at <https://developer.mozilla.org/En/Mozilla_CSS_support_chart>
> indicates (1) that ALL of CSS 2.1 is implemented and (2) that @page is a CSS 3
> feature.  However, @page is specified in Section 13.2 of the current (8 Sep 09)
> "W3C Candidate Recommendation" for CSS 2.1.  

The specification links in that chart (which, by the way, was not written by Gecko implementers) point to the newest specification, not the oldest.
Comment 77 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2010-03-24 18:45:10 PDT
In slightly more detail:

The specification links in that chart generally point to the newest specification, not the oldest.  I find that somewhat confusing too, but, then again, I didn't write it.  If you want to fix it, you're welcome to, as it's a wiki; I don't have the time to fix it right now.

However, complaints about that chart don't belong in this bug.
Comment 78 Édouard Lopez 2010-12-08 03:03:20 PST
Support still missing for:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101203 Firefox-4.0/4.0b8pre
Comment 79 David E. Ross 2011-06-08 10:48:41 PDT
@page is a CSS 2.1 capability.  Today, CSS 2.1 reached "Recommendation" status.  See <http://www.w3.org/2011/05/css-pr>.  

Given the asserted compliance with standards by Mozilla products, this should now have Severity = "Major" (major loss of function).
Comment 80 Daniel Glazman (:glazou) 2011-06-08 11:04:20 PDT
(In reply to comment #79)

> @page is a CSS 2.1 capability.  Today, CSS 2.1 reached "Recommendation"
> status.  See <http://www.w3.org/2011/05/css-pr>.  
> 
> Given the asserted compliance with standards by Mozilla products, this
> should now have Severity = "Major" (major loss of function).

No. A bug filed for a missing, not implemented, feature is a RFE.
Switching back to "enhancement". Other than that, I agree, it's time
to have it implemented in Gecko.
Comment 81 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-06-08 11:36:05 PDT
I guess the parts of @page that I really don't understand (i.e., I have no idea what implementing them would mean) have been moved to css3-page, so implementing the CSS 2.1 parts of @page seems reasonably doable.

The hardest part is probably figuring out how to do layout when the containers are different widths, i.e., what roc described in http://lists.w3.org/Archives/Public/www-style/2011Jun/0039.html .

That said, I think fantasai volunteered last week to write a spec for how that should work.
Comment 82 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-25 19:51:26 PDT
I don't think we should be spending much time writing specs for things we aren't implementing.
Comment 83 fantasai 2011-09-26 11:40:15 PDT
CSS2.1 allows treating all pages as having the same ICB width, so we don't need to block @page margins on that. As for the spec on varying-width pagination, it's informally written here:
  http://lists.w3.org/Archives/Public/www-style/2011Sep/0301.html
Comment 84 David E. Ross 2011-09-28 12:53:47 PDT
Re comment #82:  Does this mean that portions of the final CSS 2.1 specification that have not yet been implemented in Gecko will not be implemented?
Comment 85 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-28 16:09:36 PDT
No, it doesn't mean that.
Comment 86 Michael Ventnor 2011-10-05 23:15:56 PDT
I've been working on this for the past few days.

Currently trying to figure out how to implement the cascading rules in CSS3, but I may not do selector support with the first WIP.
Comment 87 Michael Ventnor 2011-10-10 23:58:28 PDT
Created attachment 566135 [details] [diff] [review]
WIP-1

This adds support for parsing declarations within the @page, but no nested margin @ rules. We also don't support selectors yet, I've been spending all day figuring out how the heck our selectors code works and getting nowhere.
We then create a style context and attach it to the page content frame. dbaron, if you're OK with this I'll start working on the rendering part.
Comment 88 Michael Ventnor 2011-10-18 02:31:36 PDT
Created attachment 567696 [details] [diff] [review]
WIP-1 rendering

Provides support for @page rendering, including proper margin handling and border-background rendering. Managed to remove some code in the process!

Want feedback on what I have so far, still some issues I've been looking into and not solved yet.
Comment 89 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-18 02:42:55 PDT
Comment on attachment 567696 [details] [diff] [review]
WIP-1 rendering

Review of attachment 567696 [details] [diff] [review]:
-----------------------------------------------------------------

Looks somewhat reasonable.
Comment 90 Ivan Enderlin 2011-11-30 02:26:25 PST
Hello,

@page in CSS3 (please, see the dedicated module http://w3.org/TR/css3-page/) is also not implemented. There is a lot of useful features, such as: size, which is very useful to export/print slides written in HTML. Are they any plan to implement this module? I didn't find related bugs.
Comment 91 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-12-06 15:45:26 PST
Comment on attachment 566135 [details] [diff] [review]
WIP-1

Given that the editor's draft of css3-page seems to be moving towards
letting a lot of properties apply to @page, this approach (constructing
a regular style context) seems like it's probably the right approach.

Commenting out an attribute in nsIDOMCSSPageRule.idl means you
should change the IID.

The way you apply both ::-moz-pagecontent and @page rules to the same
style context seems a bit weird, but I don't have a better idea right
now.

In nsLayoutUtils::PaintFrame -- who's now responsible for drawing the
canvas background when printing?

The nsPresShell.cpp changes look like they were intended for some other
patch.

The set of properties valid inside an @page rule is different from the
set valid in style rules.  Note that CSS 2.1 says:
  # In CSS 2.1, only the margin properties ('margin-top',
  # 'margin-right', 'margin-bottom', 'margin-left', and 'margin') apply
  # within the page context.
whereas
http://www.w3.org/TR/2004/CR-css3-page-20040225/#page-margin-border-padding
says:
 # The margin, border and padding properties apply to the page box.
whereas css3-page editor's draft allows a lot more:
  http://dev.w3.org/csswg/css3-page/#page-properties
There are two possibilities here:  we could handle it at parsing time
(the way we do @font-face, i.e., change ParsePageRule to do something
other than just call ParseDeclarationBlock) or at cascading time (like
we do for ::first-line and ::first-letter).  However, given that the
margin box stuff coming to @page is going to require separate parsing
code for @page anyway, it seems like it might be better to do it at
parse time, though the parse time option also requires more object model
code (i.e., changes to nsCSSRules.cpp/h that look more like the
@font-face code).  This also provides more useful error messages to
authors since they get a syntax error report when they use a property
that doesn't apply to @page rules.  I'm still unsure which is the better
way to go here.

ParsePageRule should probably also at least have a comment suggesting
that you'll want selector parsing there.


In nsCSSRuleProcessor::AppendPageRules, please {} the single-line
if body.

In nsCSSRuleProcessor.h, you shouldn't copy the broken indentation --
that's just a side-effect of the PRBool -> bool substitution.

nsStyleSet::ResolvePageContentFrameStyle should just use
ruleWalker.Forward(), not ForwardOnPossiblyCSSRule().  But depending on
what should happen about !important, see below, you probably need to do
more to handle !important.  See:
http://lists.w3.org/Archives/Public/www-style/2011Dec/0188.html

This'll also need merging for the PRBool removal.


There's one other somewhat bigger problem, perhaps (I think we do
sometimes do reresolution on print presentations... though maybe we
don't anymore; in either case it should work), which is that we need to
make style *reresolution* lead to the same style.  I'm not quite sure
what the right thing to do is:  one option is to special-case the
pageContent pseudo inside of ResolveAnonymousBoxStyle rather than making
a separate function for it, and then things would just work.  That may
well be the best option.  Otherwise you'd probably have to add special
cases to a bunch of places in nsFrameManager::ReResolveStyleContext.


With those things fixed I think this looks good.  Sorry for taking so
long to get to it.
Comment 92 Julian Viereck 2012-06-04 12:32:44 PDT
Created attachment 629882 [details] [diff] [review]
WIP-1-rebase

Rebased version of WIP-1. Applying this patch and compiling I get the following error when starting FF:

  WARNING: Last startup was detected as a crash.: file /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/toolkit/components/startup/nsAppStartup.cpp, line 940
  ###!!! ASSERTION: Class info data out of sync, you forgot to update nsDOMClassInfo.h and nsDOMClassInfo.cpp! Fix this, mozilla will not work without this fixed!: 'Error', file /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/dom/base/nsDOMClassInfo.cpp, line 4484
  WARNING: NS_ENSURE_SUCCESS(rv, 0L) failed with result 0xC1F30001: file /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/dom/base/nsDOMClassInfo.cpp, line 5092
  ###!!! ASSERTION: Class info data out of sync, you forgot to update nsDOMClassInfo.h and nsDOMClassInfo.cpp! Fix this, mozilla will not work without this fixed!: 'Error', file /Users/jviereck/develop/moz/pdfjs/ff_hg_callback/dom/base/nsDOMClassInfo.cpp, line 4484
Comment 93 Julian Viereck 2012-06-04 12:34:57 PDT
Created attachment 629883 [details] [diff] [review]
WIP-1 rendering-rebase

Rebased version of "WIP-1 rendering".

Note that this and the previous submitted rebased patch worked for me some time back. Also note, that I did the rebasing and it "worked", but it would be good for someone that has more insight to the code base and understand what's happening if my way of doing the rebasing makes sense.
Comment 94 Brendan Dahl [:bdahl] 2012-06-15 16:12:23 PDT
Created attachment 633715 [details] [diff] [review]
WIP-2-Rendering
Comment 95 Brendan Dahl [:bdahl] 2012-06-15 16:24:30 PDT
Created attachment 633718 [details] [diff] [review]
WIP-2-Style

@dbaron I think I've addressed all the review feedback from you except for the following because I wanted to hear if this is still an issue before I work on it:
> There's one other somewhat bigger problem, perhaps (I think we do
> sometimes do reresolution on print presentations... though maybe we
> don't anymore; in either case it should work), which is that we need to
> make style *reresolution* lead to the same style.  I'm not quite sure
> what the right thing to do is:  one option is to special-case the
> pageContent pseudo inside of ResolveAnonymousBoxStyle rather than making
> a separate function for it, and then things would just work.  That may
> well be the best option.  Otherwise you'd probably have to add special
> cases to a bunch of places in nsFrameManager::ReResolveStyleContext.

On the rule parsing code... I started to go the route that the @font-face rule does by parsing everything itself e.g. creating ParsePageRule(), ParsePageDescriptor(), and ParsePageDescriptorValue() and having all the allowed properties in nsCSSPageDescList.h.  For CSS2.1 this seems easy enough since there's only the margin property allowed.  However, looking ahead to CSS3 lots of page properties are added that are exactly the same properties allowed in regular rule sets, which led me to think I shouldn't be creating custom CSS_PAGE_DESC properties. So what I did instead, is just add a check for when we are parsing the @page rule to only allow the margin properties.  Let me know what you think of this (feels a bit hacky).

Also, I've added a test case for the margin parsing code and I'd like to add tests for the rendering part of the code too, but I'm not finding anything for that.
Comment 96 Brendan Dahl [:bdahl] 2012-07-09 17:41:04 PDT
Created attachment 640457 [details] [diff] [review]
WIP-2-Rendering

Fixes the top and bottom margins.  Still not sure if we need to account for padding in the calculation:
+  // ASK: do we need to account for padding here?
   nscoord expectedPageContentHeight = 
-    NSToCoordCeil((GetSize().height - mPD->mReflowMargin.TopBottom()) / scale);
+    NSToCoordCeil(GetSize().height / scale);
Comment 97 Brendan Dahl [:bdahl] 2012-07-18 14:45:38 PDT
Created attachment 643599 [details] [diff] [review]
WIP-2-Rendering

Adds print ref tests for margins.
Comment 98 Brendan Dahl [:bdahl] 2012-07-24 09:03:38 PDT
Created attachment 645336 [details] [diff] [review]
WIP-2-Rendering

Previous patch was the wrong patch in the queue.
Comment 99 Boris Zbarsky [:bz] 2012-07-25 12:53:43 PDT
> So what I did instead, is just add a check for when we are parsing the @page rule to
> only allow the margin properties.  Let me know what you think of this (feels a bit
> hacky).

Are we guaranteed that @page descriptors will always be actual CSS properties?

As long as this is guaranteed, I think just checking whether a prop is allowed in @page is fine, though I think we should do that via the per-property flags in nsCSSPropList.h instead of hardcoding an out-of-band list.
Comment 100 Brendan Dahl [:bdahl] 2012-07-25 13:28:06 PDT
Just to record a bit of conversation in IRC:
me: for css 2.1 only margin is allowed in the @page rule, so it guaranteed for that. In css3 the only descriptor that i can see that isn't a regular css descriptor is "size", but there is a lot of new syntax for styling the margin boxes
http://www.w3.org/TR/css3-page/#margin-at-rules

bz:
as long as there's any descriptor that's not actually a CSS property, you need a different storage mechanism
or allowing 'size' in arbitrary rulesets
so it sounds like we will in fact want the separate thing long-term, possibly
it's a bit annoying.  :(
maybe we can stuff it into the proplist file somehow
given the amount of overlap
Comment 101 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-08-21 16:25:41 PDT
Comment on attachment 633718 [details] [diff] [review]
WIP-2-Style

Sorry for taking so long to get back to you on this.  For some reason I was thinking it was a review request (not really an excuse, though) rather than a request for specific feedback.

(In reply to Brendan Dahl from comment #95)
> @dbaron I think I've addressed all the review feedback from you except for
> the following because I wanted to hear if this is still an issue before I
> work on it:
> > There's one other somewhat bigger problem, perhaps (I think we do
> > sometimes do reresolution on print presentations... though maybe we
> > don't anymore; in either case it should work), which is that we need to
> > make style *reresolution* lead to the same style.  I'm not quite sure
> > what the right thing to do is:  one option is to special-case the
> > pageContent pseudo inside of ResolveAnonymousBoxStyle rather than making
> > a separate function for it, and then things would just work.  That may
> > well be the best option.  Otherwise you'd probably have to add special
> > cases to a bunch of places in nsFrameManager::ReResolveStyleContext.

I think you do need to address this.

> On the rule parsing code... I started to go the route that the @font-face
> rule does by parsing everything itself e.g. creating ParsePageRule(),
> ParsePageDescriptor(), and ParsePageDescriptorValue() and having all the
> allowed properties in nsCSSPageDescList.h.  For CSS2.1 this seems easy
> enough since there's only the margin property allowed.  However, looking
> ahead to CSS3 lots of page properties are added that are exactly the same
> properties allowed in regular rule sets, which led me to think I shouldn't
> be creating custom CSS_PAGE_DESC properties. So what I did instead, is just
> add a check for when we are parsing the @page rule to only allow the margin
> properties.  Let me know what you think of this (feels a bit hacky).

I think this approach is fine, but you should use an enum for the context rather than a boolean aIsPageRule, and you should put the relevant data in the property flags as Boris described.  (To implement 'size', we could then also have a property flag for a property that is *not* valid in a regular declaration.)

I think you may also run into DOM interface issues regarding CSS2Properties.  I think you should copy the approach Boris took with @font-face in bug 765588.

> Also, I've added a test case for the margin parsing code and I'd like to add
> tests for the rendering part of the code too, but I'm not finding anything
> for that.

I'm not sure what you mean here.
Comment 102 Brendan Dahl [:bdahl] 2012-08-31 14:28:08 PDT
Created attachment 657446 [details] [diff] [review]
Part 1 - rendering v1
Comment 103 Brendan Dahl [:bdahl] 2012-08-31 14:35:12 PDT
Created attachment 657450 [details] [diff] [review]
Part 2 - style v1

I've attempted to address all the feedback so far.  It should be noted the goal of this patch is to only add support for css2 @page margins.  It should be easy to add more css3 @page support, but I do not want to do that in this patch.

(In reply to David Baron [:dbaron] from comment #101)
> I think you may also run into DOM interface issues regarding CSS2Properties.
> I think you should copy the approach Boris took with @font-face in bug
> 765588.
I can't seem to trigger the issue that @font-face had.

> > Also, I've added a test case for the margin parsing code and I'd like to add
> > tests for the rendering part of the code too, but I'm not finding anything
> > for that.
> 
> I'm not sure what you mean here.
I hadn't found the printing reftests ye, but I found them and added one for this.
Comment 104 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-02 19:03:13 PDT
Comment on attachment 657446 [details] [diff] [review]
Part 1 - rendering v1

Review of attachment 657446 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsPageContentFrame.cpp
@@ +100,5 @@
>    NS_ASSERTION(NS_FRAME_IS_COMPLETE(fixedStatus), "fixed frames can be truncated, but not incomplete");
>  
>    // Return our desired size
> +  aDesiredSize.width = aReflowState.ComputedWidth();
> +  if (aReflowState.ComputedHeight() != NS_UNCONSTRAINEDSIZE) {

I think we can assert that ComputedHeight() != NS_UNCONSTRAINEDSIZE.

::: layout/generic/nsPageFrame.cpp
@@ +108,5 @@
> +      }
> +    }
> +
> +    maxSize.width -= pageContentMargin.LeftRight() / scale;
> +    if (maxSize.height != NS_UNCONSTRAINEDSIZE) {

I think we can assert maxSize.height != NS_UNCONSTRAINEDSIZE here. I don't know what it would mean to have a page that's unconstrained height.

@@ +110,5 @@
> +
> +    maxSize.width -= pageContentMargin.LeftRight() / scale;
> +    if (maxSize.height != NS_UNCONSTRAINEDSIZE) {
> +      maxSize.height -= pageContentMargin.TopBottom() / scale;
> +    }

I think we need to do something here to prevent width/height going negative. And should probably add a test for someone doing stupid-large @page margins.
Comment 105 Brendan Dahl [:bdahl] 2012-09-05 11:33:31 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #104)
> I think we can assert that ComputedHeight() != NS_UNCONSTRAINEDSIZE.
> 

Just going off what a comment says in nsPageFrame.cpp "When the reflow size is NS_UNCONSTRAINEDSIZE it means we are reflowing a single page to print selection. So this means we want to use NS_UNCONSTRAINEDSIZE without altering it". I haven't verified myself though if this is how it still works.

> I think we need to do something here to prevent width/height going negative.
> And should probably add a test for someone doing stupid-large @page margins.

What would we consider stupid large? Anything that's bigger than the available space/2 or?
Comment 106 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-05 21:52:14 PDT
(In reply to Brendan Dahl from comment #105)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #104)
> > I think we can assert that ComputedHeight() != NS_UNCONSTRAINEDSIZE.
> > 
> 
> Just going off what a comment says in nsPageFrame.cpp "When the reflow size
> is NS_UNCONSTRAINEDSIZE it means we are reflowing a single page to print
> selection. So this means we want to use NS_UNCONSTRAINEDSIZE without
> altering it". I haven't verified myself though if this is how it still works.

OK, ignore my comment then.

> > I think we need to do something here to prevent width/height going negative.
> > And should probably add a test for someone doing stupid-large @page margins.
> 
> What would we consider stupid large? Anything that's bigger than the
> available space/2 or?

Good question. At least we have to prevent width/height from going negative. Even if we do that, a zero-height page could easily lead to us creating an infinite number of pages. How about we set the minimum width and height to something small and arbitrary, say 100px?
Comment 107 Brendan Dahl [:bdahl] 2012-09-18 13:12:09 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #106)
> Good question. At least we have to prevent width/height from going negative.
> Even if we do that, a zero-height page could easily lead to us creating an
> infinite number of pages. How about we set the minimum width and height to
> something small and arbitrary, say 100px?

I implemented something like this by resetting the margins to the default size if height or width is less than 10px. Chrome seems to do something similar whereas IE just shows 1 blank page. Do you have a preference on which way we handle it?

Also, this has brought up another question on the expected behavior of margins and scaling: 
If I have @page left/right margins set to 4in and the page is 10in and I also have "Ignore scaling and Shrink to Fit Page" checked then the content gets scaled down to 2in. I suppose this seems correct and IE does this(chrome doesn't have this option), but I want to hear more feedback on the expected behavior.
Comment 108 Brendan Dahl [:bdahl] 2012-09-18 15:48:24 PDT
Created attachment 662341 [details] [diff] [review]
Part 1 - rendering v3

I've added two new test cases for: 1) huge margins 2) a page that should result in zero width.  As mentioned above, to handle these I made it so if the page's width or height is less than a pixel then the page margins get reset back to the defaults.
Comment 109 Brendan Dahl [:bdahl] 2012-09-18 15:50:59 PDT
Created attachment 662342 [details] [diff] [review]
Part 2 - style v3

Small update to rebase and add the MOZ_OVERRIDE stuff for page rule.
Comment 110 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-18 17:01:03 PDT
(In reply to Brendan Dahl from comment #107)
> I implemented something like this by resetting the margins to the default
> size if height or width is less than 10px. Chrome seems to do something
> similar whereas IE just shows 1 blank page. Do you have a preference on
> which way we handle it?

What you did sounds fine.

> Also, this has brought up another question on the expected behavior of
> margins and scaling: 
> If I have @page left/right margins set to 4in and the page is 10in and I
> also have "Ignore scaling and Shrink to Fit Page" checked then the content
> gets scaled down to 2in. I suppose this seems correct and IE does
> this(chrome doesn't have this option), but I want to hear more feedback on
> the expected behavior.

That behavior sounds right.
Comment 111 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-15 16:22:56 PDT
Comment on attachment 662342 [details] [diff] [review]
Part 2 - style v3

>+       DOM_CLASSINFO_MAP_BEGIN(CSSPageRule, nsIDOMCSSPageRule)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMCSSPageRule)
>+  DOM_CLASSINFO_MAP_END

Fix the weird indentation of the first line (which is a tab, which
you shouldn't have).

>diff --git a/dom/interfaces/css/nsIDOMCSSPageRule.idl b/dom/interfaces/css/nsIDOMCSSPageRule.idl

You should at least document why you're removing the selectorText
property.  Perhaps it should be commented out rather than removed?

>+  nsAutoPtr<css::Declaration> declaration(ParseDeclarationBlock(true, eCSSContext_Page));

This should have a line break in it somewhere so it doesn't go over 80
characters.

>+    REPORT_UNEXPECTED(PEBadSelectorKeyframeRuleIgnored);

I think you should remove this line; you're not dealing with keyframe
rules, and ParseDeclaration already deals with error reporting.  (I
think it doesn't belong in the place you copied it from, either.  If
you could remove it there, i.e., from the null-declaration case in
ParseKeyframeRule, that would be good.)

>-    value = tk->mNumber;
>+    value = tk->mNumber; 

Don't introduce trailing whitespace.


In ParseDeclaration, please assert that aContext is equal to either
eCSSContext_General or eCSSContext_Page.


I think you should also add CSS_PROPERTY_APPLIES_TO_PAGE_RULE to:
-moz-margin-start{,-value}
-moz-margin-end{,-value}
{margin-left,margin-right,-moz-margin-start,-moz-margin-end}-{ltr,rtl}-source

Most of these aren't directly parseable, but I think it's probably best
that way for clarity, and I think you should support margin-start/end.


+  // ASK: Does this rebase make sense??? 
+  n += mPageRules.SizeOfExcludingThis(aMallocSizeOf);
+

yes, and remove the comment and extra whitespace


nsCSSPageRule needs some merging with bug 753517 and bug 795221.


The handling of !important is incorrect based on the replies to the
message cited in comment 91.  You need to associate an ImportantRule
object with the rule when the declaration HasImportantData(), as
StyleRule does, and then need to put the important rules into the rule
tree in your added code in nsStyleSet::ResolveAnonymousBoxStyle.  (This
instead of mapping the important data directly in
nsCSSPageRule::MapRuleInfoInto, which means it doesn't override other
rules.)

And mSheet references need to change to GetStyleSheet() for additional
merging.


nsCSSRules.h:

drop the style changes to nsCSSKeyframeStyleDeclaration.

>+  nsAutoPtr<mozilla::css::Declaration>       mDeclaration;
>+  // lazily created when needed:
>+  nsRefPtr<nsCSSPageStyleDeclaration>    mDOMDeclaration;

Fix "mDOMDeclaration" to line up with "mDeclaration".

test_page_parser.html:

>+  <meta http-equiv="content-type" content="text/html; charset=utf-8">

just <meta charset="UTF-8">

And could you at least add a comment saying that the margin-*-value
and margin-*-source properties showing up isn't really the expected
result; it's something we'd like to fix?


r=dbaron with those things fixed, though I should probably take a quick look at the changes to !important handling
Comment 112 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-17 18:51:59 PDT
To respond to your question on IRC (since you left IRC before I saw it):

> [2012-10-17 15:10:29] <bdahl> I finally have some time to return to @page. I think I may be over complicating things or misunderstood you.  I'm trying http://pastebin.mozilla.org/1869414 but it seems if I want the correct Forward method in nsRuleWalker to be called for the important rule than I should actually have getImportantRule return a StyleRule instead of an ImportantRule. http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleWalker.h#38


That pastebin has:

  if (aPseudoTag == nsCSSAnonBoxes::pageContent) {
    // Add any @page rules that are specified.
    nsTArray<nsCSSPageRule*> rules;
    nsPresContext* presContext = PresContext();
    presContext->StyleSet()->AppendPageRules(presContext, rules);
    for (PRUint32 i = 0, i_end = rules.Length(); i != i_end; ++i) {
      nsRefPtr<mozilla::css::ImportantRule> importantRule;
      rules[i]->GetImportantRule(getter_AddRefs(importantRule));
      if (importantRule) {
        ruleWalker.Forward(importantRule);
      } else {
        ruleWalker.Forward(rules[i]);
      }
    }
  }

What you need is something more like:

  if (aPseudoTag == nsCSSAnonBoxes::pageContent) {
    // Add any @page rules that are specified.
    nsTArray<nsCSSPageRule*> rules;
    nsTArray<css::ImportantRule*> importantRules;
    nsPresContext* presContext = PresContext();
    presContext->StyleSet()->AppendPageRules(presContext, rules);
    for (PRUint32 i = 0, i_end = rules.Length(); i != i_end; ++i) {
      ruleWalker.Forward(rules[i]);
      css::ImportantRule* importantRule = rules[i]->GetImportantRule();
      if (importantRule) {
        importantRules.AppendElement(importantRule);
      }
    }
    for (PRUint32 i = 0, i_end = importantRules.Length(); i != i_end; ++i) {
      ruleWalker.Forward(importantRule);
    }
  }
Comment 113 Brendan Dahl [:bdahl] 2012-10-18 09:44:02 PDT
Hah, I was over complicating it! I was thinking I needed to use something built into handle applying the important rules.

Last question(hopefully):
> >diff --git a/dom/interfaces/css/nsIDOMCSSPageRule.idl b/dom/interfaces/css/nsIDOMCSSPageRule.idl
> 
> You should at least document why you're removing the selectorText
> property.  Perhaps it should be commented out rather than removed?
> 

I believe the original author of the patch commented it out since it is unused.  Is it better to leave it commented out then remove it?
Comment 114 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-18 10:08:52 PDT
I'd say leave it commented out, since the attribute remains in the DOM spec, and the DOM interfaces generally just have what's in the relevant spec.  (But not implementing it for now probably makes sense since we don't implement @page selectors.)
Comment 115 Brendan Dahl [:bdahl] 2012-10-18 13:54:24 PDT
Created attachment 672942 [details] [diff] [review]
Part 1 - rendering v4

Minor update after rebase, carrying forward r+.
Comment 116 Brendan Dahl [:bdahl] 2012-10-18 13:59:02 PDT
Created attachment 672943 [details] [diff] [review]
Part 2 - style v4

Should address all review feedback.
Comment 117 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-18 16:25:21 PDT
Comment on attachment 672943 [details] [diff] [review]
Part 2 - style v4

nsCSSPageRule::ChangeDeclaration needs to set mImportantRule to null.

You also need an nsCSSPageStyleDeclaration::GetParentObject
implementation for merging with bug 753517.

And I'd prefer if you could avoid including StyleRule.h in nsCSSRules.h --
can you just forward-declare ImportantRule like you did StyleRule in the
old patch?

r=dbaron with that
Comment 118 Brendan Dahl [:bdahl] 2012-10-19 09:07:48 PDT
(In reply to David Baron [:dbaron] from comment #117)
> And I'd prefer if you could avoid including StyleRule.h in nsCSSRules.h --
> can you just forward-declare ImportantRule like you did StyleRule in the
> old patch?

I need the full definition to create an ImportantRule nsCSSPageRule::GetImportantRule() (line 2488 https://bugzilla.mozilla.org/attachment.cgi?id=672943&action=diff#a/layout/style/nsCSSRules.cpp_sec2).

Is there a different way I should be doing this?
Comment 119 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-19 09:15:36 PDT
#include StyleRule.h in nsCSSRules.cpp and forward-declare in nsCSSRules.h.
Comment 120 Brendan Dahl [:bdahl] 2012-10-19 09:56:58 PDT
I also forgot to mention nsRefPtr doesn't like working with an incomplete type in nsCSSRules.h.  I can use a regular pointer, but I was under the impression that was generally frowned upon in MC.
Comment 121 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-10-24 09:20:52 PDT
OK, I guess just #include it then.


Also, we need to make sure that vh/vw/vmin/vmax units get ignored and treated as auto (instead of doing something crazy).
Comment 122 Brendan Dahl [:bdahl] 2012-10-29 17:32:59 PDT
Created attachment 676408 [details] [diff] [review]
Part 2 - style v5

> Also, we need to make sure that vh/vw/vmin/vmax units get ignored and
> treated as auto (instead of doing something crazy).

Those units are not currently ignored, but I tried out a several test cases and it seems to behave how I would expect it as a percent of the page width.  I also tried out some bad values and the code that protects against margins that are too large/small seems to work fine for this case as well.

If we really want to ignore can we file a follow up bug to change that?
Comment 123 Brendan Dahl [:bdahl] 2012-10-30 09:25:50 PDT
Created attachment 676638 [details] [diff] [review]
Part 2 - style v6

Fixes parse flags rebase.
Comment 124 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-11-09 11:53:03 PST
Comment on attachment 676638 [details] [diff] [review]
Part 2 - style v6

r=dbaron
Comment 125 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-11-09 11:55:41 PST
(In reply to Brendan Dahl from comment #122)
> Created attachment 676408 [details] [diff] [review]
> Part 2 - style v5
> 
> > Also, we need to make sure that vh/vw/vmin/vmax units get ignored and
> > treated as auto (instead of doing something crazy).
> 
> Those units are not currently ignored, but I tried out a several test cases
> and it seems to behave how I would expect it as a percent of the page width.
> I also tried out some bad values and the code that protects against margins
> that are too large/small seems to work fine for this case as well.

What does it mean for the page width to be a percentage of the page width?

> If we really want to ignore can we file a follow up bug to change that?

I think we do need to do something before this ships.
Comment 126 Brendan Dahl [:bdahl] 2012-11-09 16:49:15 PST
> What does it mean for the page width to be a percentage of the page width?

e.g. If I set the margins on an 8.5in wide page to 10vw the margins print as .85in.
Comment 127 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-11-09 16:59:33 PST
(In reply to Brendan Dahl from comment #126)
> > What does it mean for the page width to be a percentage of the page width?
> 
> e.g. If I set the margins on an 8.5in wide page to 10vw the margins print as
> .85in.

Except the vw unit is supposed to be a percentage of the initial containing block, which is the size *inside* those margins, not the size of the page.

And then it gets worse if you consider the 'size' property in @page.


See:
http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths
http://www.w3.org/TR/CSS21/visudet.html#containing-block-details
http://dev.w3.org/csswg/css3-page/#page-box-page-rule
Comment 128 Brendan Dahl [:bdahl] 2012-11-12 17:18:01 PST
Checkin-needed for part 1 and part 2 patches.  Ignoring the new css units will be in a subsequent patch.
Comment 129 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2012-11-13 11:23:42 PST
I filed bug 811391 on the viewport units issue.  And as I said on IRC, I'm ok with this landing without fixing that bug.
Comment 130 Ryan VanderMeulen [:RyanVM] 2012-11-13 16:02:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/30776e402787
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6b3d2856a4

Per IRC discussion, we're going to let this bug close out when it hits m-c. Followup work can be done in new bugs.
Comment 131 Ryan VanderMeulen [:RyanVM] 2012-11-13 17:20:41 PST
Unfortunately, we had to back this out due to reftest failures :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7efaf3ac7a

https://tbpl.mozilla.org/php/getParsedLog.php?id=17007619&tree=Mozilla-Inbound

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-block-page-break-inside-avoid-8.html | image comparison (==), max difference: 255, number of differing pixels: 171552
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-block-page-break-inside-avoid-13.html | image comparison (==), max difference: 255, number of differing pixels: 171552
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-float-page-break-inside-avoid-2.html | image comparison (==), max difference: 255, number of differing pixels: 16920
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-float-page-break-inside-avoid-5.html | image comparison (==), max difference: 255, number of differing pixels: 55296
Comment 134 Jean-Yves Perrier [:teoli] 2012-11-19 03:18:37 PST
I've updated:
https://developer.mozilla.org/en-US/docs/CSS/@page and
https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers

Note that if I have read the bug correctly, the pseudo-classes :first, :left, and :right, which can be used as page selectors are not (yet) implemented. Is this correct? (I didn't find a specific bug for them, so I'm not 100% sure, but the code seems clear)
Comment 135 Brendan Dahl [:bdahl] 2012-11-19 09:07:30 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #134)
> I've updated:
> https://developer.mozilla.org/en-US/docs/CSS/@page and
> https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers
> 
> Note that if I have read the bug correctly, the pseudo-classes :first,
> :left, and :right, which can be used as page selectors are not (yet)
> implemented. Is this correct? (I didn't find a specific bug for them, so I'm
> not 100% sure, but the code seems clear)

That is correct. I've filed bug 813187 for the page selectors.
Comment 136 Jean-Yves Perrier [:teoli] 2012-12-11 05:31:57 PST
I created:
https://developer.mozilla.org/en-US/docs/DOM/CSSPageRule

(I forgot it initially).
Comment 137 stephen.cunliffe 2013-02-19 12:33:05 PST
If I read this bug correctly (and the associated comments) the root issue is that setting the @page{size:landscape;} CSS doesn't result in the page previewing/printing in landscape orientation like other browsers (e.g. Chrome) do.

I see that this bug was marked fixed for Mozilla19... if this is the same as the just released Firefox v19.0 then I'm afraid it still doesn't work.

Can anyone else confirm this (I have a test case if needed)
Comment 138 Jean-Yves Perrier [:teoli] 2013-02-20 11:27:18 PST
Stephen, if I'm not mistaken, size was a proposed CSS property in CSS2 but never implemented and removed in CSS2 (level 1). So it is expected not to work. It may reappear in a future CSS spec post 2.1, but is not here for the moment.
Comment 139 stephen.cunliffe 2013-02-20 12:44:52 PST
This really sucks.  I'm not sure if I want to focus my frustration at the W3C, the CSS3 group, Firefox/Mozilla or what.

Printing from the web has always had issues but a few little sprinkles of CSS have made it much better.

It is awesome that Chrome supports:
<style type="text/css" media="print">
@page{
    size:landscape;
}
</style>

as a hint to the browser that the page layout was designed for printing in landscape orientation to provide a default when the user goes to print.

I fully realize that serving up a PDF gives me full control (and that's the only option I currently have) but it would have been very nice if Firefox enabled the "hint" to default the orientation when printing.

Now to track down the CSS working group to determine what happened to this property and if it will ever return.

Disappointing...
Comment 140 philippe (part-time) 2013-02-20 15:57:44 PST
(In reply to stephen.cunliffe from comment #139)

> <style type="text/css" media="print">
> @page{
>     size:landscape;
> }
> </style>
... 
> Now to track down the CSS working group to determine what happened to this
> property and if it will ever return.

You'll want to have a look at:
http://dev.w3.org/csswg/css3-page/#page-size

(note: editor's draft, the whole module is currently under discussion on the www-style mailing list)
Comment 141 Jake 2013-03-04 06:44:32 PST
Our IT department has removed all Firefox browsers from each computer across the entire organization based on the fact that @page is not supported. All of our invoices and quotes that are generated on the site need to have support that allow users (internal and external) to print directly from the browser and without PDF plugins.

@page {size:landscape;} works terrific in Chrome.

It is my hope that Mozilla fix the issue as soon as possible.
Comment 142 Gérard Talbot 2013-03-14 09:43:23 PDT
(In reply to stephen.cunliffe from comment #137)
> If I read this bug correctly (and the associated comments) the root issue is
> that setting the @page{size:landscape;} CSS doesn't result in the page
> previewing/printing in landscape orientation like other browsers (e.g.
> Chrome) do.

Bug report 115199 was about @page in CSS2.1. And it has been fixed.

> I see that this bug was marked fixed for Mozilla19... if this is the same as
> the just released Firefox v19.0 then I'm afraid it still doesn't work.
> 
> Can anyone else confirm this (I have a test case if needed)

(In reply to stephen.cunliffe from comment #139)
> This really sucks.  I'm not sure if I want to focus my frustration at the
> W3C, the CSS3 group, Firefox/Mozilla or what.

Everything (new features or fixing a bug) often starts with a stable specification, a good bug report (often with a few good tests) and a suitable/proactive attitude.

> Printing from the web has always had issues but a few little sprinkles of
> CSS have made it much better.
> 
> It is awesome that Chrome supports:
> <style type="text/css" media="print">
> @page{
>     size:landscape;
> }
> </style>
> 
> as a hint to the browser that the page layout was designed for printing in
> landscape orientation to provide a default when the user goes to print.

A good and reduced test case is always welcomed.

How to Really, Really Help Developers on Bugs -- Minimal Testcases
https://wiki.mozilla.org/MozillaQualityAssurance:Triage#How_to_Really.2C_Really_Help_Developers_on_Bugs_--_Minimal_Testcases
 
Reducing testcases
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Reducing_testcases?redirectlocale=en-US&redirectslug=Reducing_testcases

> I fully realize that serving up a PDF gives me full control (and that's the
> only option I currently have) but it would have been very nice if Firefox
> enabled the "hint" to default the orientation when printing.
> 
> Now to track down the CSS working group to determine what happened to this
> property and if it will ever return.
> 
> Disappointing...

(In reply to Jake from comment #141)
> Our IT department has removed all Firefox browsers from each computer across
> the entire organization based on the fact that @page is not supported. 

As far as CSS 2.1 is involved, @page is supported in Firefox 19 and there are tests proving this.

> All
> of our invoices and quotes that are generated on the site need to have
> support that allow users (internal and external) to print directly from the
> browser and without PDF plugins.
> 
> @page {size:landscape;} works terrific in Chrome.

That's CSS3 paged media ... which I'm sure is worth implementing.

> It is my hope that Mozilla fix the issue as soon as possible.

Can you create a new bug report and attach a reduced test case to it?

How to Write a Proper Bug
https://quality.mozilla.org/docs/bugzilla/starter-kit/how-to-write-a-proper-bug/

Bug writing guidelines
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines

I am also wondering: what happens when users with Firefox do File/Print Preview, push the Landscape button and push the Print button? Isn't this a suitable workaround?
(...)
I just tried this and it worked as expected.

Gérard
Comment 143 Christian Fersch 2013-03-14 10:03:06 PDT
Sorry for spamming, but Gérard – you have my utmost respect for that calm and helpful answer.
Comment 144 Gérard Talbot 2013-03-14 14:59:18 PDT
(In reply to Jake from comment #141)
> Our IT department has removed all Firefox browsers from each computer across
> the entire organization based on the fact that @page is not supported. All
> of our invoices and quotes that are generated on the site need to have
> support that allow users (internal and external) to print directly from the
> browser and without PDF plugins.
> 
> @page {size:landscape;} works terrific in Chrome.


Jake,

Submit this as a workaround at your IT department.

1- Type in about:config in the addressbar of a new tab in Firefox 19

2- In the search field, type print.postscript.orientation

3- Right-click the property and modify it from portrait to landscape: Value should become landscape and the line should now be bold

4- Close the tab and open a new tab, load any webpage and print it (File/Print...Ctrl+P): it should be printed in landscape. I have tried this and it works (Firefox 19.0.2 under Linux KDE 4.10.1 i686 32bits). 

Again, this is not an implementation of 
@page {size: landscape;}
. It merely is a temporary workaround and a rational trade-off between removing all Firefox browsers from each computer across your entire organization and a complete and ready-working @page {size: landscape;}; this is furthermore relevant if only invoices and quotes are going to be printed and need to be in landscape.

Gérard
Comment 145 Emmanuel Bourg 2013-03-14 16:06:16 PDT
This bug was probably misinterpreted. The original reporter explicitly mentioned being unable to specify the orientation of the page. The bug also duplicates 6 bugs mentioning the lack of orientation support. So closing the bug was probably a bit "hasty".
Comment 146 stephen.cunliffe 2013-03-14 16:22:59 PDT
Thanks Gérard

Agreed, pressing the landscape button in the preview does solve this from a user perspective. (if they know the page was optimized for a landscape print)

However as developers working on web applications it would be nice to present the desired orientation with the simple CSS3 @page hint that way we can default landscape/portrait printing just as other browsers handle it.

Since it appears that (as noted above) the focus of this issue was actually elsewhere I will file a separate bug rather than continue this comment chain.
Comment 147 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-03-14 16:30:11 PDT
(In reply to Emmanuel Bourg from comment #145)
> This bug was probably misinterpreted. The original reporter explicitly
> mentioned being unable to specify the orientation of the page. The bug also
> duplicates 6 bugs mentioning the lack of orientation support. So closing the
> bug was probably a bit "hasty".

Bug reports are used to track development work through the development process.  A bug that already has 146 comments is already useless for that, and any further work is more likely to be completed if it's tracked by a separate bug that is clearly about the particular work that needs to be done.  (See http://dbaron.org/log/20120816-bug-system for a little more detail about my thoughts on bug tracking.)
Comment 148 Gérard Talbot 2013-03-14 16:45:58 PDT
comment #130 says:
"we're going to let this bug close out when it hits m-c. Followup work can be done in new bugs."

So, now is okay to create a new bug report for that well desired 
@page {size: landscape;}

(In reply to stephen.cunliffe from comment #146)
> it would be nice to
> present the desired orientation with the simple CSS3 @page hint that way we
> can default landscape/portrait printing just as other browsers handle it.

Agreed.

> Since it appears that (as noted above) the focus of this issue was actually
> elsewhere I will file a separate bug rather than continue this comment chain.

Good!
Comment 149 Jean-Yves Perrier [:teoli] 2013-03-15 01:39:42 PDT
I filled bug 851441 for the size property.

Note You need to log in before you can comment on or make changes to this bug.