Bug 131831 - [EFL][WK2] Changing page zoom factor does not work correctly if fixed layout is used.
Summary: [EFL][WK2] Changing page zoom factor does not work correctly if fixed layout ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eunmi Lee
URL:
Keywords:
Depends on: 126022 131962
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-17 17:31 PDT by Eunmi Lee
Modified: 2017-03-11 10:32 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.77 KB, patch)
2014-04-17 17:41 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2014-05-01 00:08 PDT, Eunmi Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2014-04-17 17:31:10 PDT
We can test changing page zoom factor with fixed layout using MiniBrowser as follow option.
> MiniBrowser -L 1
But it does not work correctly - page is not zoomed correctly.
It's caused by fitting logic of PageViewportController.

solution:
1. set user interaction if page zoom is changed.
2. use content size which does not apply page zoom factor.
Comment 1 Eunmi Lee 2014-04-17 17:41:06 PDT
Created attachment 229601 [details]
Patch
Comment 2 Gyuyoung Kim 2014-04-30 00:40:35 PDT
Comment on attachment 229601 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:246
> +    impl->pageViewportController().setHadUserInteraction(true);

I don't think we have to set "true" whenever ewk_view_page_zoom_set() is called. Shouldn't we call it only one time ?
Comment 3 Eunmi Lee 2014-04-30 03:46:55 PDT
Comment on attachment 229601 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:246
>> +    impl->pageViewportController().setHadUserInteraction(true);
> 
> I don't think we have to set "true" whenever ewk_view_page_zoom_set() is called. Shouldn't we call it only one time ?

we can check hadUserInteraction value and set true only if it is false. I will update patch.
Comment 4 Gyuyoung Kim 2014-04-30 04:12:23 PDT
Comment on attachment 229601 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3055
> +    // We have to find better solution.

Eunmi, as you know, this looks one of workaround solutions. I'm also investigating how to solve this problem fundamentally. If you have spare time with your nice condition, please take a look fundamental problem.
Comment 5 Gyuyoung Kim 2014-04-30 04:14:46 PDT
Comment on attachment 229601 [details]
Patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:3055
>> +    // We have to find better solution.
> 
> Eunmi, as you know, this looks one of workaround solutions. I'm also investigating how to solve this problem fundamentally. If you have spare time with your nice condition, please take a look fundamental problem.

One more comment: I also think that page zoom on fixed layout should work same as "fixed layout is off".
Comment 6 Gyuyoung Kim 2014-04-30 04:25:57 PDT
Comment on attachment 229601 [details]
Patch

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

>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:3055
>>> +    // We have to find better solution.
>> 
>> Eunmi, as you know, this looks one of workaround solutions. I'm also investigating how to solve this problem fundamentally. If you have spare time with your nice condition, please take a look fundamental problem.
> 
> One more comment: I also think that page zoom on fixed layout should work same as "fixed layout is off".

Second comment:

RenderView::viewWidth() and RenderView::viewHeight() value have been affected by on/off of fixedlayout. When fixed layout is on, with/height look be multiplied by pageZoomFactor. If we don't apply effectiveZoom(), when the fixed layout is enabled without this patch, page zoom works as it is off.

 int RenderView::viewWidth() const
 {
     int width = 0;
     if (!shouldUsePrintingLayout()) {
         width = frameView().layoutWidth();
         width = frameView().useFixedLayout() ? ceilf(style().effectiveZoom() * float(width)) : width;
     }
     ...
 }


RenderView::viewWidth()
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderView.cpp#L1109
Comment 7 Eunmi Lee 2014-05-01 00:08:53 PDT
Created attachment 230568 [details]
Patch
Comment 8 Gyuyoung Kim 2014-05-09 03:36:06 PDT
Comment on attachment 230568 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3119
> +        newSize.scale(1 / m_pageZoomFactor);

As mentioned in comment #6, RenderView::viewWidth/Height() multiplies the pageZoomFactor(= effectiveZoom()) with width/height when fixed layout is enabled. So, I think we need to check if the calculation logic is correct from EFL port perspective. I think this patch looks a workaround patch. I'd like to clear r?,cq? until finishing the investigation.
Comment 9 Michael Catanzaro 2017-03-11 10:32:31 PST
Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.