Bug 74487 - Support orientation attribute of viewport meta tag.
Summary: Support orientation attribute of viewport meta tag.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-14 00:07 PST by Ryuan Choi
Modified: 2012-02-01 04:26 PST (History)
14 users (show)

See Also:


Attachments
Patch (11.48 KB, patch)
2011-12-14 03:12 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2011-12-20 17:51 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
rebase_patch (13.21 KB, patch)
2012-01-16 19:23 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (17.17 KB, patch)
2012-02-01 03:34 PST, Ryuan Choi
kenneth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-12-14 00:07:33 PST
Orientation attribute of viewport enables web pages to request whether to lock orientation of contents.
This is optional attribute of the spec: http://dev.w3.org/csswg/css-device-adapt/
Comment 1 Ryuan Choi 2011-12-14 03:12:05 PST
Created attachment 119188 [details]
Patch
Comment 2 Gyuyoung Kim 2011-12-14 18:47:26 PST
CC'ing kenneth, he is main developer for viewport meta tag.
Comment 3 Kenneth Rohde Christiansen 2011-12-15 03:39:43 PST
I have personally been against this as it poses some big usability problems. It is highly confusing for users if the browser automatically rotates when clicking on a link or navigating session history. It is also confusing for the users when the web browser suddently doesn't want to rotate.

So I suggest that we only respect the properly for stand-alone web apps and for fullscreen (FullscreenAPI spec).
Comment 4 Kenneth Rohde Christiansen 2011-12-15 03:42:07 PST
Comment on attachment 119188 [details]
Patch

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

> Source/WebCore/dom/ViewportArguments.h:46
> +    ViewportOrientationAuto,

Can't this use the ValueAuto?

> Source/WebKit/efl/ewk/ewk_view.h:84
> + *  - "viewport,orientation,changed", Ewk_Viewport_Orientation: reports that orientation of viewport was changed.
>   *  - "viewport,changed", void: reports that viewport was changed.

Why doesnt the below cover this?
Comment 5 Ryuan Choi 2011-12-15 06:55:27 PST
(In reply to comment #3)
> I have personally been against this as it poses some big usability problems. It is highly confusing for users if the browser automatically rotates when clicking on a link or navigating session history. It is also confusing for the users when the web browser suddently doesn't want to rotate.
> 
> So I suggest that we only respect the properly for stand-alone web apps and for fullscreen (FullscreenAPI spec).

Thanks for the review.

I agree. it can make usability problem for the almost case without you mentioned.
I think that webkit should just notify this attribute for the application such as web apps framework or browser application which is in fullscreen mode.

(In reply to comment #4)
> (From update of attachment 119188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119188&action=review
> 
> > Source/WebCore/dom/ViewportArguments.h:46
> > +    ViewportOrientationAuto,
> 
> Can't this use the ValueAuto?

I considered but I thought that ViewportOrientationXXX is clear to convert WebCore's enum into WebKit port's enum.

> 
> > Source/WebKit/efl/ewk/ewk_view.h:84
> > + *  - "viewport,orientation,changed", Ewk_Viewport_Orientation: reports that orientation of viewport was changed.
> >   *  - "viewport,changed", void: reports that viewport was changed.
> 
> Why doesnt the below cover this?

IMHO, orientation is not proper value for the additional data of "viewport,changed".
ViewportArgument looks reasonable as the additional data of "viewport,changed".

For now, WebKit/Efl doesn't need to share ViewportArgument values via "viewport,changed" signal because it provide computed ViewportAttribute using ewk_view_viewport_attributes_get().
Comment 6 Kenneth Rohde Christiansen 2011-12-16 02:17:17 PST
I sent an email to www-style:

http://lists.w3.org/Archives/Public/www-style/2011Dec/0402.html
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-12-19 16:52:44 PST
Comment on attachment 119188 [details]
Patch

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

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:299
> +void DumpRenderTreeChrome::onViewportOrientationChanged(void*, Evas_Object*, void* eventInfo)
> +{
> +    Ewk_Viewport_Orientation* orientation = static_cast<Ewk_Viewport_Orientation*>(eventInfo);
> +    switch (*orientation) {
> +    case EWK_VIEWPORT_ORIENTATION_AUTO:
> +        printf("viewport orientation changed to auto\n");
> +        break;
> +    case EWK_VIEWPORT_ORIENTATION_PORTRAIT:
> +        printf("viewport orientation changed to portrait\n");
> +        break;
> +    case EWK_VIEWPORT_ORIENTATION_LANDSCAPE:
> +        printf("viewport orientation changed to landscape\n");
> +        break;
> +    }
> +}

On the EFL side, this method does not make sense. There is no layout test checking for these messages.
Comment 8 Ryuan Choi 2011-12-20 15:16:17 PST
(In reply to comment #7)
> (From update of attachment 119188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119188&action=review
> 
> > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:299
> > +void DumpRenderTreeChrome::onViewportOrientationChanged(void*, Evas_Object*, void* eventInfo)
> > +{
> > +    Ewk_Viewport_Orientation* orientation = static_cast<Ewk_Viewport_Orientation*>(eventInfo);
> > +    switch (*orientation) {
> > +    case EWK_VIEWPORT_ORIENTATION_AUTO:
> > +        printf("viewport orientation changed to auto\n");
> > +        break;
> > +    case EWK_VIEWPORT_ORIENTATION_PORTRAIT:
> > +        printf("viewport orientation changed to portrait\n");
> > +        break;
> > +    case EWK_VIEWPORT_ORIENTATION_LANDSCAPE:
> > +        printf("viewport orientation changed to landscape\n");
> > +        break;
> > +    }
> > +}
> 
> On the EFL side, this method does not make sense. There is no layout test checking for these messages.

Sorry, test case was missed in this patch.
This method is for new test case.

I'll update patch for the test case.
Comment 9 Ryuan Choi 2011-12-20 17:51:05 PST
Created attachment 120126 [details]
Patch
Comment 10 Ryuan Choi 2012-01-16 19:23:15 PST
Created attachment 122703 [details]
rebase_patch
Comment 11 Kenneth Rohde Christiansen 2012-01-17 05:04:23 PST
Comment on attachment 122703 [details]
rebase_patch

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

> LayoutTests/fast/viewport/viewport-orientation-expected.txt:3
> +viewport orientation changed to landscape
> +
> +viewport orientation changed to auto

I think this should be checked like any other viewport test... here you are testing your specific implementation which sends this out as a separate signal
Comment 12 Ryuan Choi 2012-02-01 03:34:57 PST
Created attachment 124922 [details]
Patch
Comment 13 Kenneth Rohde Christiansen 2012-02-01 03:38:24 PST
Comment on attachment 124922 [details]
Patch

orientation is controversal and will probably be removed from the CSS DA spec.
Comment 14 Ryuan Choi 2012-02-01 04:26:25 PST
(In reply to comment #13)
> (From update of attachment 124922 [details])
> orientation is controversal and will probably be removed from the CSS DA spec.

Close the bug because of above reason.