Closed Bug 115199 Opened 23 years ago Closed 12 years ago

@page in CSS2 not implemented

Categories

(Core :: Printing: Output, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: krishna, Assigned: bdahl)

References

(Blocks 3 open bugs)

Details

(4 keywords, Whiteboard: parity-Opera, parity-IE8, CSS2.1)

Attachments

(9 files, 17 obsolete files)

6.19 KB, patch
Details | Diff | Splinter Review
232.34 KB, patch
Details | Diff | Splinter Review
1.01 KB, text/html
Details
27.38 KB, patch
dbaron
: feedback+
Details | Diff | Splinter Review
20.20 KB, patch
Details | Diff | Splinter Review
28.22 KB, patch
Details | Diff | Splinter Review
14.40 KB, patch
Details | Diff | Splinter Review
18.54 KB, patch
bdahl
: review+
Details | Diff | Splinter Review
41.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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
Unfortunatly my M$ cohort had a setting wrong and it wont work in any version of
IE either. 
Opera 6.0 seems to work
Could you describe the difference between the actual results and expected results?
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
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We have not implemented much more than the page break, we aren't even paring the
@page rule yet.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Future
*** Bug 136316 has been marked as a duplicate of this bug. ***
This implements the @page and supports "size" and "margin", but doesn't support
and Pseudo names (:first, :right, :left)
Summary: @page in CSS2 not implemented (correctly?) → @page in CSS2 not implemented
Rod, care to explain your design for this?
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 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...
Attached patch patch (obsolete) — Splinter Review
This patch doesn't include the DocumentViewer changes, same as previous patch.
But it cleans up the parsing and includes the nsICSSPageRule.h
Target Milestone: Future → mozilla1.0.1
It didn't include the DOM changes either.  Did you mean to include them?
Target Milestone: mozilla1.0.1 → mozilla1.1alpha
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.
Oops, that didn't have some of the changes that I thought I made.  Here's the
real one.
Attachment #95927 - Attachment is obsolete: true
I am studying your patch, what is "nsPageRuleData" what function does it perform?
nsPageRuleData is something that I forgot to remove in the first patch -- it's
the main thing I corrected in the second one.
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).
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → mozilla1.2beta
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.
Attachment #82852 - Attachment is obsolete: true
Attachment #89939 - Attachment is obsolete: true
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...
I'd like to deCOMify nsStyleContext -- why not start nsPageStyleContext there? 
How is |delete| (or a virtual |Destroy| if needed) harder than |Release|?
Release() is easy to trigger automatically via nsCOMPtr.  If nsAutoPtr works
with nsPageStyleContext, then I withdraw item #3.
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.
*** Bug 174954 has been marked as a duplicate of this bug. ***
The target milestone should be probably changed to mozilla1.2final since 1.2b is
out.
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.
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Attachment #102005 - Flags: review?(bzbarsky)
It's probably worth mentioning that the @page rule has been removed from CSS 
2.1 (due, I believe, to lack of implementations).
> 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
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.
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.
Attached patch full patch (obsolete) — Splinter Review
full patch
Attachment #102005 - Attachment is obsolete: true
Attachment #102100 - Attachment is obsolete: true
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.
Attachment #107083 - Attachment is obsolete: true
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.
Attachment #107911 - Flags: superreview?(jst)
Attachment #107911 - Flags: review?(dcone)
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 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.
Attachment #102005 - Flags: review?(bzbarsky)
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

r=dcone.
Attachment #107911 - Flags: review?(dcone) → review+
So.. what's the state of this patch?
Boris Zbarsky wrote:
> So.. what's the state of this patch?

It's rotting around... ;-(((
Comment on attachment 107911 [details] [diff] [review]
full final patch for review

I should probably review it.
Attachment #107911 - Flags: superreview?(jst) → superreview?(dbaron)
*** Bug 72321 has been marked as a duplicate of this bug. ***
Attached file testcase
Keywords: css2
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 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.
Attachment #107911 - Flags: review+
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.
Assignee: rods → core.printing
Status: ASSIGNED → NEW
Keywords: testcase
QA Contact: sujay → ian
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.
Flags: blocking1.8a?
Flags: blocking1.7?
Flags: blocking1.7? → blocking1.7-
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...  
Assignee: core.printing → nobody
Keywords: helpwanted
Priority: P1 → --
Target Milestone: mozilla1.3alpha → ---
Flags: blocking1.8a? → blocking1.8a-
Flags: blocking1.7.1?
Flags: blocking1.8a2-
Flags: blocking1.7.1?
Flags: blocking1.7.1-
*** Bug 248573 has been marked as a duplicate of this bug. ***
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.
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.
(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 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?
Blocks: 278678
Blocks: css-paged-media
No longer blocks: 278678
No longer blocks: css-paged-media
CC spam, and to reitterate that this is also a problem for Firefox (1.0.4)
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/
*** Bug 306628 has been marked as a duplicate of this bug. ***
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.   
Summary: @page in CSS2 not implemented → Page Breaks in CSS2.1 Incompletely Implemented
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).
This bug, in fact, has almost nothing to do with the page-break-* properties.
Summary: Page Breaks in CSS2.1 Incompletely Implemented → @page in CSS2 not implemented
I have submitted new bug report 308970 relative to page breaks.  
*** Bug 314902 has been marked as a duplicate of this bug. ***
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.
Page orientation and paper size would seem to be very important also.
*** Bug 331451 has been marked as a duplicate of this bug. ***
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?
*** Bug 349759 has been marked as a duplicate of this bug. ***
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.)
(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?
(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?
Is there any chance that this should be wanted/blocking Gecko 1.9.1?
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1? → wanted1.9.2?
Whiteboard: parity-Opera
QA Contact: ian → printing
Whiteboard: parity-Opera → parity-Opera, parity-IE8
Gecko is closing up to reach full CSS2 compability, but this has been left out. Something to look into for 1.9.2?
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2-
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))
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.
(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.
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.
Support still missing for:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101203 Firefox-4.0/4.0b8pre
@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).
Severity: normal → major
Whiteboard: parity-Opera, parity-IE8 → parity-Opera, parity-IE8, CSS2.1
(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.
Severity: major → enhancement
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.
I don't think we should be spending much time writing specs for things we aren't implementing.
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
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?
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.
Attached patch WIP-1Splinter Review
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.
Attachment #566135 - Flags: feedback?(dbaron)
Attached patch WIP-1 renderingSplinter Review
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.
Attachment #567696 - Flags: feedback?(roc)
Comment on attachment 567696 [details] [diff] [review]
WIP-1 rendering

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

Looks somewhat reasonable.
Attachment #567696 - Flags: feedback?(roc) → feedback+
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 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.
Attachment #566135 - Flags: feedback?(dbaron) → feedback+
Assignee: nobody → julian.viereck
Status: NEW → ASSIGNED
Attached patch WIP-1-rebaseSplinter Review
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
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.
Assignee: julian.viereck → nobody
Status: ASSIGNED → NEW
Attached patch WIP-2-Rendering (obsolete) — Splinter Review
Attached patch WIP-2-Style (obsolete) — Splinter Review
@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.
Attachment #633718 - Flags: feedback?(dbaron)
Attached patch WIP-2-Rendering (obsolete) — Splinter Review
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);
Attachment #633715 - Attachment is obsolete: true
Attached patch WIP-2-Rendering (obsolete) — Splinter Review
Adds print ref tests for margins.
Attachment #640457 - Attachment is obsolete: true
Attached patch WIP-2-Rendering (obsolete) — Splinter Review
Previous patch was the wrong patch in the queue.
Attachment #643599 - Attachment is obsolete: true
> 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.
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
Assignee: nobody → bdahl
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.
Attachment #633718 - Flags: feedback?(dbaron)
Attached patch Part 1 - rendering v1 (obsolete) — Splinter Review
Attachment #645336 - Attachment is obsolete: true
Attachment #657446 - Flags: review?(roc)
Attached patch Part 2 - style v1 (obsolete) — Splinter Review
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.
Attachment #633718 - Attachment is obsolete: true
Attachment #657450 - Flags: review?(dbaron)
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.
(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?
(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?
(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.
Attached patch Part 1 - rendering v3 (obsolete) — Splinter Review
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.
Attachment #657446 - Attachment is obsolete: true
Attachment #657446 - Flags: review?(roc)
Attachment #662341 - Flags: review?(roc)
Attached patch Part 2 - style v3 (obsolete) — Splinter Review
Small update to rebase and add the MOZ_OVERRIDE stuff for page rule.
Attachment #657450 - Attachment is obsolete: true
Attachment #657450 - Flags: review?(dbaron)
Attachment #662342 - Flags: review?(dbaron)
(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 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
Attachment #662342 - Flags: review?(dbaron) → review+
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);
    }
  }
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?
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.)
Minor update after rebase, carrying forward r+.
Attachment #662341 - Attachment is obsolete: true
Attachment #672942 - Flags: review+
Attached patch Part 2 - style v4 (obsolete) — Splinter Review
Should address all review feedback.
Attachment #662342 - Attachment is obsolete: true
Attachment #672943 - Flags: review?(dbaron)
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
Attachment #672943 - Flags: review?(dbaron) → review+
(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?
#include StyleRule.h in nsCSSRules.cpp and forward-declare in nsCSSRules.h.
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.
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).
Attached patch Part 2 - style v5 (obsolete) — Splinter Review
> 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?
Attachment #672943 - Attachment is obsolete: true
Attachment #676408 - Flags: review?(dbaron)
Fixes parse flags rebase.
Attachment #676408 - Attachment is obsolete: true
Attachment #676408 - Flags: review?(dbaron)
Attachment #676638 - Flags: review?(dbaron)
Comment on attachment 676638 [details] [diff] [review]
Part 2 - style v6

r=dbaron
Attachment #676638 - Flags: review?(dbaron) → review+
(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.
> 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.
(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
Checkin-needed for part 1 and part 2 patches.  Ignoring the new css units will be in a subsequent patch.
Keywords: checkin-needed
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.
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.
Flags: in-testsuite+
Keywords: checkin-needed
Flags: wanted1.9.2?
Flags: wanted1.9.1?
Flags: blocking1.9.2-
Flags: blocking1.8a2-
Flags: blocking1.8a1-
Flags: blocking1.7.5-
Flags: blocking1.7-
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
Depends on: 811827
https://hg.mozilla.org/mozilla-central/rev/31312fa1abde
https://hg.mozilla.org/mozilla-central/rev/2ea5d36b35a1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 812344
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)
Blocks: 813187
(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.
Depends on: 827591
Depends on: 829817
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)
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.
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...
(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)
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.
No longer blocks: 813187
(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
Sorry for spamming, but Gérard – you have my utmost respect for that calm and helpful answer.
(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
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".
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.
(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 #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!
I filled bug 851441 for the size property.
No longer blocks: css-paged-media
Blocks: 752787
No longer blocks: 1246805
See Also: → 35615
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: