fix interaction of viewport units (vh/vw/vmin/vmax) with @page styles

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dbaron, Assigned: seth)

Tracking

({dev-doc-complete})

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 unaffected, firefox19+ fixed, firefox20+ fixed, firefox21 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

This is followup to bug 115199 comment 121 and comments that follow it.

I posted http://lists.w3.org/Archives/Public/www-style/2012Nov/0161.html about the interaction of these units with @page.

We probably need to get his sorted out before we ship viewport units.
Taking a look now.
David and I discussed this IRL and decided that the appropriate choice here is to ignore @page declarations that involve viewport units. Proposed patch will be incoming shortly.
Assignee: nobody → seth
Posted patch (Part 2) Tests. (obsolete) — Splinter Review
Tests.
Fixed a minor bug. Everything looks good. Ready for review.
Attachment #700119 - Flags: review?(dbaron)
Attachment #699466 - Attachment is obsolete: true
Also fixed a bug in the tests. Ready for review.
Attachment #700120 - Flags: review?(dbaron)
Attachment #699467 - Attachment is obsolete: true
Comment on attachment 700119 [details] [diff] [review]
(Part 1) Ignore @page declarations involving viewport units.

r=dbaron

In theory I'd rather this be written using an RAII class (which is both safer under future mutation of the code, and exception-safe if we ever decide to allow C++ exceptions), except I never managed to land https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/321c7aa169d8/auto-toggle .  Perhaps I ought to land it at some point...
Attachment #700119 - Flags: review?(dbaron) → review+
Comment on attachment 700120 [details] [diff] [review]
(Part 2) Tests.

r=dbaron
Attachment #700120 - Flags: review?(dbaron) → review+
> In theory I'd rather this be written using an RAII class

Totally. I saw a mention of AutoToggle in the comments somewhere and thought that would be perfect, but I couldn't find it. (It not having landed explains that!) Let me know if you get that in and I'll be happy to revisit this code and RAII-ify it.
Try job looks OK. Requestin checkin.
Keywords: checkin-needed
This was unlucky enough to land *immediately* after I pushed a cset to make build warnings fatal in /layout/style (in bug 829168).  So despite the green Try runs, the one build warning (from an out-of-order init list) that this introduced in /layout/style made it burn when it landed on m-i. :)

I'll re-land w/ the init list reordered so that this won't cause that build warning (/error).
https://hg.mozilla.org/mozilla-central/rev/057e4f6d513f
https://hg.mozilla.org/mozilla-central/rev/ec36f0d72b88
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment on attachment 700119 [details] [diff] [review]
(Part 1) Ignore @page declarations involving viewport units.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 503720,115199
User impact if declined: Incorrect rendering behavior if @page and viewport units are combined. We want to avoid releasing this behavior into the wild so web developers don't begin to rely on it.
Testing completed (on m-c, etc.): No problems on m-c.
Risk to taking this patch (and alternatives if risky): Low risk. It involves two new features which happened to land in the same release and can potentially interact in a problematic way. This fixes their interaction and should not affect any existing features.
String or UUID changes made by this patch: None.
Attachment #700119 - Flags: approval-mozilla-beta?
Attachment #700119 - Flags: approval-mozilla-aurora?
Comment on attachment 700119 [details] [diff] [review]
(Part 1) Ignore @page declarations involving viewport units.

Low risk fix in support of new features. Approving for Aurora/Beta.
Attachment #700119 - Flags: approval-mozilla-beta?
Attachment #700119 - Flags: approval-mozilla-beta+
Attachment #700119 - Flags: approval-mozilla-aurora?
Attachment #700119 - Flags: approval-mozilla-aurora+
(Also, this is in my queue to land on Aurora once it reopens)
Marking as [qa-] since there is an automated test that verifies the fix. Please remove the tag and add "verifyme" keyword if you need any manual verification/testing from QA.
Whiteboard: [qa-]
https://developer.mozilla.org/en-US/docs/Web/CSS/length
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21

A question regarding the specification of this behavior:
1. CSS Values & Units Module Level 3 explicitly defer the definition of the behavior between viewport-percentage lengths to CSS Paged Media Level 3.
2. The editor's draft of CSS Pages Media has not yet been updated with the new behavior.

Am I correct?
Oups, small error. This feature is in Firefox 19.
I removed the mention in https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21

I didn't add it in https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/19 as there is the mention of the newly implemented support of the behavior.

(The CSS <length> page has the specific info)
Actually, this is still an open issue in the spec: https://www.w3.org/Style/CSS/Tracker/issues/315

Because recent drafts also say that @page should inherit from the root element, there are all kinds of way to introduce circular dependencies.

  :root { font-size: 1vw }
  @page { width: 50em; /* font-size: inherit */ }

Any way I can think of to resolve this breaks one use case or another. I discussed this at length on www-style (unfortunately not all of which is linked form the CSS Tracker issue.) Ping me to dig up the archive links if needed.
You need to log in before you can comment on or make changes to this bug.