Bug 70370 - Implement CSS Device Adaptation
Summary: Implement CSS Device Adaptation
Status: RESOLVED DUPLICATE of bug 95959
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Thiago Marcos P. Santos
URL: http://dev.w3.org/csswg/css-device-ad...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-18 14:58 PDT by David Storey
Modified: 2012-09-06 03:23 PDT (History)
37 users (show)

See Also:


Attachments
Work in progress of the CSS Device Adaptation (22.63 KB, patch)
2012-01-22 08:33 PST, Thiago Marcos P. Santos
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Work in progress of the CSS Device Adaptation v2 (47.60 KB, patch)
2012-06-28 07:31 PDT, Thiago Marcos P. Santos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Work in progress of the CSS Device Adaptation v3 (54.39 KB, patch)
2012-06-29 06:47 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Work in progress of the CSS Device Adaptation v4 (54.29 KB, patch)
2012-06-29 08:25 PDT, Thiago Marcos P. Santos
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Work in progress of the CSS Device Adaptation v5 (54.91 KB, patch)
2012-06-29 13:34 PDT, Thiago Marcos P. Santos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (157.70 KB, application/zip)
2012-06-29 17:01 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Storey 2011-10-18 14:58:23 PDT
There is a proposal for doing the viewport meta element via  a CSS @viewport -at-rule instead. It would be nice to have this in WebKit.

Joe Lambert wrote a post which included some rational for wanting something like this in CSS [0]

"Current meta tag support is useful but it does pose some problems, especially if you wish to treat different devices differently. Suppose you’re using media queries to render your site differently for iPhone & iPad, you may wish to prevent scaling on iPhone but not on iPad. This (as far as I have found) isn’t possible without conditionally adding the metatags server side.

"If these viewport controls were controlled via CSS then we could use the same media queries to deliver different settings to different devices!"

[0] http://blog.joelambert.co.uk/2011/10/13/ios5-for-web-developers
Comment 1 Shane Stephens 2011-10-19 20:28:42 PDT
Relevant ED specification: http://dev.w3.org/csswg/css-device-adapt/
Comment 2 Kenneth Rohde Christiansen 2012-01-03 12:38:15 PST
Assigning to Thiago, as he is working on this.
Comment 3 Thiago Marcos P. Santos 2012-01-22 08:33:17 PST
Created attachment 123473 [details]
Work in progress of the CSS Device Adaptation

Update on the current status of my implementation: parsing is almost done. I had a problem of properties like zoom, height and width (which is a shorthand in viewport) having a different semantics from other specs. First I though about prefixing like -webkit-viewport-zoom, but that would completely break compatibility with other implementations. I had to create a state whenever the parser is inside a viewport block and go to a different path for property validation.
Comment 4 WebKit Review Bot 2012-01-22 08:37:03 PST
Attachment 123473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSParser.cpp:683:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/css/CSSParser.cpp:696:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/css/CSSParser.cpp:704:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/css/CSSParser.cpp:708:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Gyuyoung Kim 2012-01-22 09:02:39 PST
Comment on attachment 123473 [details]
Work in progress of the CSS Device Adaptation

Attachment 123473 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11284535
Comment 6 WebKit Review Bot 2012-01-22 09:07:56 PST
Comment on attachment 123473 [details]
Work in progress of the CSS Device Adaptation

Attachment 123473 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11114059
Comment 7 Collabora GTK+ EWS bot 2012-01-22 10:55:50 PST
Comment on attachment 123473 [details]
Work in progress of the CSS Device Adaptation

Attachment 123473 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11114084
Comment 8 Kenneth Rohde Christiansen 2012-01-22 15:30:23 PST
Comment on attachment 123473 [details]
Work in progress of the CSS Device Adaptation

View in context: https://bugs.webkit.org/attachment.cgi?id=123473&action=review

I am just looking the patch through to get a sense of it, so just some minor comments

> Source/WebCore/ChangeLog:7
> +        Current WIP of CSS Device Adaptation implementation
> +        https://bugs.webkit.org/show_bug.cgi?id=70370
> +
> +        Reviewed by NOBODY (OOPS!).
> +

To ease the pre-review it would be good if you could start by writing a great changelog

> Source/WebCore/css/CSSParser.cpp:662
> +    // Some properties needs remapping because their names clashes with
> +    // other existing properties names but the semantics are different

a nit, in webkit we end our comments with a punctuation mark

> Source/WebCore/css/CSSParser.cpp:678
> +    switch (static_cast<CSSPropertyID>(propId)) {
> +    case CSSPropertyMinWidth:
> +        propId = CSSPropertyWebkitViewportMinWidth;
> +        break;
> +    case CSSPropertyMaxWidth:
> +        propId = CSSPropertyWebkitViewportMaxWidth;
> +        break;
> +    case CSSPropertyMinHeight:
> +        propId = CSSPropertyWebkitViewportMinHeight;
> +        break;
> +    case CSSPropertyMaxHeight:
> +        propId = CSSPropertyWebkitViewportMaxHeight;
> +        break;
> +    case CSSPropertyZoom:
> +        propId = CSSPropertyWebkitViewportZoom;
> +    }

What if it is none of these (new ones could be added in the future)?

> Source/WebCore/css/CSSParser.cpp:703
> +    case CSSPropertyMaxZoom:
> +    case CSSPropertyWebkitViewportZoom:
> +        if (id == CSSValueAuto)
> +            validPrimitive = true;
> +        else
> +            validPrimitive = (!id && validUnit(value, FNumber | FPercent | FNonNeg, m_strict));
> +        break;

Maybe add some comment that MinZoom, MaxZoom and ViewportZoom are handled alike

> Source/WebCore/css/CSSParser.cpp:8114
> +{
> +    ASSERT(!m_inViewportRule);
> +    m_inViewportRule = true;

cant this happen in illegal css code? (just wondering, im no css guru :-)

> Source/WebCore/css/WebKitCSSViewportRule.cpp:51
> +    String result = "@-webkit-viewport { ";
> +    result += m_style->cssText();
> +    result += "}";

There should be a string building in webkit which might be more effective
Comment 9 Thiago Marcos P. Santos 2012-06-28 07:31:54 PDT
Created attachment 149954 [details]
Work in progress of the CSS Device Adaptation v2

The current TODO list for this patch is:

- Add a feature flag, like CSS_DEVICE_ADAPTATION.
- Add js bindings.
- Add layout tests.
- Add the new files to xcode build system.
- Currently viewport tags inside media queries affected by viewport size are ignore (to avoid circular dependencies).
- em units are not supported.
- The resolution property is not implemented is the only property not implemented.

The current status is pretty functional on the Qt port and it passes on the IE10 test suite.
http://samples.msdn.microsoft.com/ietestcenter/default.htm#css3deviceadaptation

The patch is a squash of my personal git branch, which is more readable IMO.
http://www.tmpsantos.com.br/cgi-bin/gitweb.cgi?p=webkit.git;a=shortlog;h=refs/heads/css_device_adaptation

Should I make this bug a [meta] and split this task?

Comments are welcome.
Comment 10 Build Bot 2012-06-28 07:50:22 PDT
Comment on attachment 149954 [details]
Work in progress of the CSS Device Adaptation v2

Attachment 149954 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13111077
Comment 11 Build Bot 2012-06-28 08:28:45 PDT
Comment on attachment 149954 [details]
Work in progress of the CSS Device Adaptation v2

Attachment 149954 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13119030
Comment 12 WebKit Review Bot 2012-06-28 09:02:48 PDT
Comment on attachment 149954 [details]
Work in progress of the CSS Device Adaptation v2

Attachment 149954 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13119036
Comment 13 Thiago Marcos P. Santos 2012-06-29 06:47:03 PDT
Created attachment 150167 [details]
Work in progress of the CSS Device Adaptation v3
Comment 14 Thiago Marcos P. Santos 2012-06-29 08:25:46 PDT
Created attachment 150182 [details]
Work in progress of the CSS Device Adaptation v4
Comment 15 WebKit Review Bot 2012-06-29 09:32:13 PDT
Comment on attachment 150182 [details]
Work in progress of the CSS Device Adaptation v4

Attachment 150182 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13119364
Comment 16 Build Bot 2012-06-29 10:46:58 PDT
Comment on attachment 150182 [details]
Work in progress of the CSS Device Adaptation v4

Attachment 150182 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13113409
Comment 17 Thiago Marcos P. Santos 2012-06-29 13:34:56 PDT
Created attachment 150250 [details]
Work in progress of the CSS Device Adaptation v5
Comment 18 Build Bot 2012-06-29 14:05:23 PDT
Comment on attachment 150250 [details]
Work in progress of the CSS Device Adaptation v5

Attachment 150250 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13106542
Comment 19 WebKit Review Bot 2012-06-29 17:01:09 PDT
Comment on attachment 150250 [details]
Work in progress of the CSS Device Adaptation v5

Attachment 150250 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13119525

New failing tests:
dom/xhtml/level3/core/nodeisequalnode22.xhtml
dom/xhtml/level1/core/hc_nodeinsertbeforenewchilddiffdocument.xhtml
dom/xhtml/level3/core/nodeinsertbefore13.xhtml
dom/xhtml/level3/core/nodereplacechild37.xhtml
dom/xhtml/level1/core/hc_attrappendchild5.xhtml
dom/xhtml/level3/core/nodeisequalnode21.xhtml
dom/xhtml/level3/core/nodeinsertbefore08.xhtml
dom/xhtml/level3/core/nodeisequalnode11.xhtml
dom/html/level1/core/hc_elementwrongdocumenterr.html
dom/xhtml/level3/core/nodeisequalnode07.xhtml
dom/html/level1/core/hc_attrinsertbefore6.html
dom/html/level1/core/hc_attrappendchild5.html
dom/html/level1/core/hc_nodeappendchildnewchilddiffdocument.html
dom/html/level1/core/hc_namednodemapwrongdocumenterr.html
dom/xhtml/level3/core/documentadoptnode32.xhtml
dom/xhtml/level1/core/hc_nodeappendchildnewchilddiffdocument.xhtml
dom/xhtml/level3/core/documentadoptnode14.xhtml
dom/xhtml/level1/core/hc_namednodemapwrongdocumenterr.xhtml
dom/html/level1/core/hc_nodeinsertbeforenewchilddiffdocument.html
dom/xhtml/level3/core/nodeissamenode01.xhtml
dom/xhtml/level1/core/hc_attrinsertbefore6.xhtml
dom/xhtml/level1/core/hc_nodereplacechildnewchilddiffdocument.xhtml
dom/xhtml/level3/core/nodeinsertbefore07.xhtml
dom/xhtml/level1/core/hc_elementwrongdocumenterr.xhtml
dom/xhtml/level3/core/nodeinsertbefore23.xhtml
dom/xhtml/level3/core/nodeisequalnode01.xhtml
dom/html/level1/core/hc_nodereplacechildnewchilddiffdocument.html
Comment 20 WebKit Review Bot 2012-06-29 17:01:17 PDT
Created attachment 150275 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 21 Thiago Marcos P. Santos 2012-09-06 03:23:20 PDT
Moving to a [Meta].

*** This bug has been marked as a duplicate of bug 95959 ***