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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Taking a look now.

Comment 2

[Seth Fowler [:seth] [:s2h]]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Ignore @page declarations involving viewport units.

Proposed patch.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Tests.

Tests.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=a0a14b08430f

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Ignore @page declarations involving viewport units.

Fixed a minor bug. Everything looks good. Ready for review.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Tests.

Also fixed a bug in the tests. Ready for review.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=9c24a16c2760

Comment 9

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

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

Comment 10

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

Comment on attachment 700120 [details] [diff] [review]
(Part 2) Tests.

r=dbaron

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

Try job looks OK. Requestin checkin.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a00bf2ba8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c20904b75bcc

Comment 14

[(no longer active)]

Backed out because of build bustage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b0ebc1fe57e9

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de0d5d8572d0

Comment 15

[Daniel Holbert [:dholbert]]

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

Comment 16

[Daniel Holbert [:dholbert]]

Re-landed, w/ constructor init list reordered in the first cset to fix the build warning:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/057e4f6d513f
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ec36f0d72b88

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/057e4f6d513f
https://hg.mozilla.org/mozilla-central/rev/ec36f0d72b88

Comment 18

[Seth Fowler [:seth] [:s2h]]

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.

Comment 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/15220b4f357d
https://hg.mozilla.org/releases/mozilla-beta/rev/2cf4c078ae00

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(Also, this is in my queue to land on Aurora once it reopens)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3cb24a9e2635
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c9f221756b6

Comment 23

[Ada [:adalucinet]]

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.

Comment 24

[Jean-Yves Perrier [:teoli]]

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?

Comment 25

[Jean-Yves Perrier [:teoli]]

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)

Comment 26

[Simon Sapin (:SimonSapin)]

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.