Support orientation attribute of viewport meta tag.

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Ryuan Choi]

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/

[Ryuan Choi]

Created attachment 119188 [details]
Patch

[Gyuyoung Kim]

CC'ing kenneth, he is main developer for viewport meta tag.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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?

[Ryuan Choi]

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

[Kenneth Rohde Christiansen]

I sent an email to www-style:

http://lists.w3.org/Archives/Public/www-style/2011Dec/0402.html

[Raphael Kubo da Costa (:rakuco)]

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.

[Ryuan Choi]

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

[Ryuan Choi]

Created attachment 120126 [details]
Patch

[Ryuan Choi]

Created attachment 122703 [details]
rebase_patch

[Kenneth Rohde Christiansen]

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

[Ryuan Choi]

Created attachment 124922 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 124922 [details]
Patch

orientation is controversal and will probably be removed from the CSS DA spec.

[Ryuan Choi]

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