This is happening with Reddit too on Beta and Dev. See the attached video.

Deleted: test.webm 6.6 MB

	test.webm 6.6 MB View Download

Re#1: What's happening in this video seems to be WAI. The page is supposed to switch to the mobile viewport layout, and that's what seems to be happening.

I foresee a lot complaining (and you guys getting bug reports) in r/chromeos and Chromebook Central if that's the case.

I'm seeing this bug as well.  Some sites zoom in and somewhat change layout when in tablet mode.

Hopefully this gets fixed.

If it is indeed switching to a mobile layout in tablet mode we need an option to stop that.  Personally one of the big reasons I bought a Chromebook was so that I don't have to use any mobile websites.  If my Chromebook starts trying to force mobile sites onto me, it will be a good reason to look at switching to a windows tablet.

Agreed with Omri that we should always use the desktop viewport style.

bokan@ I'm planning to disable `viewport_enabled` and `viewport_meta_enabled` as well as keep the viewport_style to DEFAULT: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.cc?type=cs&q=ChromeContentBrowserClientChromeOsPart::OverrideWebkitPrefs&sq=package:chromium&g=0&l=75

Do you see any issues for tablet mode if we make those changes?

Also, what's the difference between `viewport_enabled` and `viewport_meta_enabled`?

> Do you see any issues for tablet mode if we make those changes?

There's some "mobile" features gated on the viewport_enabled flag. One example is the "zoom in on textbox" that happens when a small editable is tapped. Another was the disambiguation popup when multiple items are in the tap area - but that appears to be controlled by an additional setting which is probably off for a tablet. IIRC there shouldn't be much more than these (except of course, viewport behavior). 

I would also check whether TextAutosizing is enabled and if that's making a difference. 

> Also, what's the difference between `viewport_enabled` and `viewport_meta_enabled`?

We were considering enabling the @viewport CSS rule that was to be a replacement for the viewport meta tag so these two allowed enabling one or the other. However, @viewport has failed to get traction and is going to be removed (it never shipped from behind a flag) so they're effectively the same thing. You must enable/disable both for viewport <meta> handling. 

re: the screenshot in OP, that doesn't look like just a mobile viewport - that looks like we're loading the page way zoomed in so there's probably a bug there. The video in #1 looks more like a page just being optimized for a mobile device.

One other place I came across just now that depends on this setting is whether scrollbars are created and managed by the visual or layout viewport for the main frame's scrollbars. This will affect whether you see the Android-style "solid color" scrollbars or the native looking Chrome OS ones and they'll behave somewhat differently. Keep an eye out on the scrollbars if you  do change this.

Thanks, bokan@ for the details!
Yes, to get rid of the issue in the video in #1, all I had to do was to disable `viewport_meta_enabled`. 

Omri, we need to discuss these differences and test them together to decide what to do.

It seems there are a couple of issues here:
1. Page zooms unexpectedly - seems that Ahmed fixed that by removing viewport_meta_enabled.
2. Page moved to mobile version - we should revert that, and again, I think Ahmed already took care of it.
3. Page resizes to fit width - this is good and expected that in tablet mode the whole page would fit as there is no mouse to scroll around. 

We are looking into also providing a "Request tablet site" in the menu soon.

I think we also want to keep using the native looking Chrome OS scrolls even in tablet mode. That implies disabling `viewport_enabled` as well. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9ef14fc54809521f4011afcbd1844e77e0ee2f3

commit b9ef14fc54809521f4011afcbd1844e77e0ee2f3
Author: Ahmed Fakhry <afakhry@google.com>
Date: Tue Jul 17 18:36:57 2018

Adjust page mobile-like behavior in tablet mode

This prevents using the mobile-viewport and the
Android-style scrollbars for pages in tablet mode.
This gives a better UX on tablets and prevents
unexpected page layout changes when switching to
tablet mode.

BUG= 831473 
TEST=Manually by visiting old.reddit.com/r/chromeos
     and switching to tablet mode. Page layout should
     not change to the mobile one, and ChromeOS
     scrollbars should remain in use.
     Also modified modified browser_tests.

Change-Id: Id858c03d37515f48b0f3515f099853999e3aafdd
Reviewed-on: https://chromium-review.googlesource.com/1139075
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575732}
[modify] https://crrev.com/b9ef14fc54809521f4011afcbd1844e77e0ee2f3/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.cc
[modify] https://crrev.com/b9ef14fc54809521f4011afcbd1844e77e0ee2f3/chrome/browser/ui/ash/tablet_mode_page_behavior_browsertest.cc

This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/caf5bc3b3f241674677bd3ac814f924b29633a6c

commit caf5bc3b3f241674677bd3ac814f924b29633a6c
Author: Ahmed Fakhry <afakhry@google.com>
Date: Thu Jul 19 01:51:43 2018

[Merge to M-68] Adjust page mobile-like behavior in tablet mode

This prevents using the mobile-viewport and the
Android-style scrollbars for pages in tablet mode.
This gives a better UX on tablets and prevents
unexpected page layout changes when switching to
tablet mode.

TBR=oshima@chromium.org
BUG= 831473 
TEST=Manually by visiting old.reddit.com/r/chromeos
     and switching to tablet mode. Page layout should
     not change to the mobile one, and ChromeOS
     scrollbars should remain in use.
     Also modified modified browser_tests.

(cherry picked from commit b9ef14fc54809521f4011afcbd1844e77e0ee2f3)

Change-Id: Id858c03d37515f48b0f3515f099853999e3aafdd
Reviewed-on: https://chromium-review.googlesource.com/1139075
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#575732}
Reviewed-on: https://chromium-review.googlesource.com/1142888
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#719}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/caf5bc3b3f241674677bd3ac814f924b29633a6c/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.cc
[modify] https://crrev.com/caf5bc3b3f241674677bd3ac814f924b29633a6c/chrome/browser/ui/ash/tablet_mode_page_behavior_browsertest.cc

On b/112587987, bokan@ said:

> Changing the default page scale factor causes us to set needs_reset in PageScaleConstraintsSet which says the page needs to reset it's scale factor to the initial value (i.e. minimally zoomed out): https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/page_scale_constraints_set.h?sq=package:chromium&g=0&l=104
> 
> However, the mobile viewport was turned off in  https://crbug.com/831473 . In that case, the minimum scale factor should always be 1 - perhaps that's being calculated incorrectly? It looks like we still set:
>
> web_prefs->shrinks_viewport_contents_to_fit = true.

> in OverrideWebkitPrefs - I think that should be off. If that fails, you could try printing out the values in PageScaleConstraintsSet::AdjustFinalConstraintsToContentsSize and seeing what there is off.

I'm now able to repro reliably. The key point is to have a browser window with small bounds before entering tablet mode (See attached video).

I logged the values both in:
blink::PageScaleConstraintsSet::AdjustFinalConstraintsToContentsSize(), and
blink::PageScaleConstraints::FitToContentsWidth() 

However I couldn't find any differences between the values inside those two function when it's working properly or when the zoom is too large.

The problem must be somewhere else.

Deleted: page_zoom-2018-08-27_15.32.53.mp4 716 KB

	page_zoom-2018-08-27_15.32.53.mp4 716 KB View Download

Does the page scale (scale_ variable) change in  VisualViewport::DidSetScaleOrLocation? If so, can you grab a stack trace using base::debug::stack_trace.

By the way, can I repro this on a Linux build of ChromeOS or do I need a physical device?

bokan@ you don't need a physical device. You can repro this on cros linux build by going to tablet mode by pressing (Ctrl + Alt + Shift + T), but for that shortcut to work you need to add the command-line arg: `--ash-debug-shortcuts`.

You may need to go into and out of tablet mode several times to get it to repro.


When this happens, |scale_| changes from 1 to 4 inside VisualViewport::DidSetScaleOrLocation().

Here's a stack trace:- I will continue digging.

|#3 0x7f413be0c1b5 blink::VisualViewport::DidSetScaleOrLocation()
|#4 0x7f413be0b269 blink::VisualViewport::SetScaleAndLocation()
|#5 0x7f413be0b83d blink::VisualViewport::SetScale()
|#6 0x7f413bdee35b blink::RotationViewportAnchor::RestoreToAnchor()
|#7 0x7f413bdee1f5 blink::RotationViewportAnchor::~RotationViewportAnchor()
|#8 0x7f413bc5a11e blink::WebViewImpl::ResizeWithBrowserControls()
|#9 0x7f415ac0300e content::RenderViewImpl::ResizeWebWidgetForWidget()
|#10 0x7f415ac19641 content::RenderWidget::ResizeWebWidget()
|#11 0x7f415ac19c20 content::RenderWidget::SynchronizeVisualProperties()
|#12 0x7f415ac159c4 content::RenderWidget::OnSynchronizeVisualProperties()
|#13 0x7f41586e85dd _ZN4base20DispatchToMethodImplIPN5media29GpuVideoDecodeAcceleratorHostEMS2_FvRK50AcceleratedVideoDecoderHostMsg_PictureReady_ParamsENSt3__15tupleIJS4_EEEJLm0EEEEvRKT_T0_OT1_NS9_16integer_sequenceImJXspT2_EEEE
|#14 0x7f41586e8538 _ZN4base16DispatchToMethodIPN5media29GpuVideoDecodeAcceleratorHostEMS2_FvRK50AcceleratedVideoDecoderHostMsg_PictureReady_ParamsENSt3__15tupleIJS4_EEEEEvRKT_T0_OT1_
|#15 0x7f415ac2b424 _ZN3IPC16DispatchToMethodIN7content12RenderWidgetEMS2_FvRKNS1_16VisualPropertiesEEvNSt3__15tupleIJS3_EEEEEvPT_T0_PT1_OT2_
|#16 0x7f415ac24b01 _ZN3IPC8MessageTI40ViewMsg_SynchronizeVisualProperties_MetaNSt3__15tupleIJN7content16VisualPropertiesEEEEvE8DispatchINS4_12RenderWidgetES9_vMS9_FvRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_
|#17 0x7f415ac132c7 content::RenderWidget::OnMessageReceived()
|#18 0x7f415ac04824 content::RenderViewImpl::OnMessageReceived()
|#19 0x7f415cf70948 IPC::MessageRouter::RouteMessage()
|#20 0x7f4157a1a8e8 content::ChildThreadImpl::ChildThreadMessageRouter::RouteMessage()
|#21 0x7f415cf708d8 IPC::MessageRouter::OnMessageReceived()
|#22 0x7f4157a21fc0 content::ChildThreadImpl::OnMessageReceived()
|#23 0x7f415cf22d15 IPC::ChannelProxy::Context::OnDispatchMessage()
|#24 0x7f415cf28adf _ZN4base8internal13FunctorTraitsIMN3IPC12ChannelProxy7ContextEFvRKNS2_7MessageEEvE6InvokeIS9_RK13scoped_refptrIS4_EJS7_EEEvT_OT0_DpOT1_
|#25 0x7f415cf28a3f _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN3IPC12ChannelProxy7ContextEFvRKNS4_7MessageEEJRK13scoped_refptrIS6_ES9_EEEvOT_DpOT0_
|#26 0x7f415cf289cd _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE7RunImplIRKSA_RKNSt3__15tupleIJSC_S6_EEEJLm0ELm1EEEEvOT_OT0_NSJ_16integer_sequenceImJXspT1_EEEE
|#27 0x7f415cf288cc _ZN4base8internal7InvokerINS0_9BindStateIMN3IPC12ChannelProxy7ContextEFvRKNS3_7MessageEEJ13scoped_refptrIS5_ES6_EEEFvvEE3RunEPNS0_13BindStateBaseE
|#28 0x7f415eb03aee _ZNO4base12OnceCallbackIFvvEE3RunEv
|#29 0x7f415eb5406a base::debug::TaskAnnotator::RunTask()
|#30 0x7f415ed25da9 base::sequence_manager::internal::ThreadControllerImpl::DoWork()
|#31 0x7f415ed28671 _ZN4base8internal13FunctorTraitsIMNS_16sequence_manager8internal20ThreadControllerImplEFvNS4_8WorkTypeEEvE6InvokeIS7_RKNS_7WeakPtrIS4_EEJRKS5_EEEvT_OT0_DpOT1_
|#32 0x7f415ed285d5 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMNS_16sequence_manager8internal20ThreadControllerImplEFvNS6_8WorkTypeEERKNS_7WeakPtrIS6_EEJRKS7_EEEvOT_OT0_DpOT1_
|#33 0x7f415ed2854d _ZN4base8internal7InvokerINS0_9BindStateIMNS_16sequence_manager8internal20ThreadControllerImplEFvNS5_8WorkTypeEEJNS_7WeakPtrIS5_EES6_EEEFvvEE7RunImplIRKS8_RKNSt3__15tupleIJSA_S6_EEEJLm0ELm1EEEEvOT_OT0_NSH_16integer_sequenceImJXspT1_EEEE
|#34 0x7f415ed2844c _ZN4base8internal7InvokerINS0_9BindStateIMNS_16sequence_manager8internal20ThreadControllerImplEFvNS5_8WorkTypeEEJNS_7WeakPtrIS5_EES6_EEEFvvEE3RunEPNS0_13BindStateBaseE
|#35 0x7f415eb03aee _ZNO4base12OnceCallbackIFvvEE3RunEv
|#36 0x7f415eb5406a base::debug::TaskAnnotator::RunTask()
|#37 0x7f415ebe23c8 base::MessageLoop::RunTask()
|#38 0x7f415ebe26cb base::MessageLoop::DeferOrRunPendingTask()
|#39 0x7f415ebe2b14 base::MessageLoop::DoWork()
|#40 0x7f415ebe9628 base::MessagePumpDefault::Run()
|#41 0x7f415ebe1b9e base::MessageLoop::Run()
|#42 0x7f415ec88f52 base::RunLoop::Run()
|#43 0x7f415ac41826 content::RendererMain()
|#44 0x7f415ae890b3 content::RunZygote()
|#45 0x7f415ae8b449 content::RunOtherNamedProcessTypeMain()
|#46 0x7f415ae8d841 content::ContentMainRunnerImpl::Run()
|#47 0x7f415ae8296c content::ContentServiceManagerMainDelegate::RunEmbedderProcess()
|#48 0x7f415f1626d1 service_manager::Main()
|#49 0x7f415ae88ae5 content::ContentMain()
|#50 0x5654faf28226 ChromeMain
|#51 0x5654faf28132 main
|#52 0x7f4131d692b1 __libc_start_main
|#53 0x5654faf2800a _start

bokan@ Here's what happens:
1- Switching to tablet mode, we override the page webkit prefs here [1]. We set the new min scale to 0.25f to allow the shrink to fit feature to work.

2- That triggers a call to `blink::RotationViewportAnchor::SetAnchor()`. When this problem happens, `old_minimum_page_scale_factor_` [2] is set to the "new" 0.25f value. When the problem doesn't happen, it's set to the correct "old" 1.f before switching to tablet mode. Apparently there is a race condition.

3- Later when the `RotationViewportAnchor` is destroyed, the dtor calls `RestoreToAnchor()` [3], which calculates the new page scale factor like:

  float new_page_scale_factor =
      old_page_scale_factor_ / old_minimum_page_scale_factor_ *
      page_scale_constraints_set_.FinalConstraints().minimum_scale;

Which results in `new_page_scale_factor` to be set to 4, and the zoom problem happens.

[1]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_content_browser_client_chromeos_part.cc?type=cs&q=ChromeContentBrowserClientChromeOsPart::OverrideWebkitPrefs&g=0&l=77

[2]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/rotation_viewport_anchor.cc?type=cs&q=blink::RotationViewportAnchor::SetAnchor&g=0&l=121

[3]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/rotation_viewport_anchor.cc?type=cs&q=RotationViewportAnchor::RestoreToAnchor&g=0&l=179

So between `SetAnchor()` and `RestoreToAnchor()` the value of

`page_scale_constraints_set_.FinalConstraints().minimum_scale`

changes from 0.25 to 1.f, which causes the problem. The formula in #24 should work if the value stays the same.

The race happens between `blink::PageScaleConstraints::FitToContentsWidth()` and `blink::RotationViewportAnchor::SetAnchor()`. 

If the order is:

1- blink::PageScaleConstraints::FitToContentsWidth()
2- blink::RotationViewportAnchor::SetAnchor()
3- blink::RotationViewportAnchor::RestoreToAnchor()

The problem doesn't happen. `blink::PageScaleConstraints::minimum_scale` is reset to 1 in `FitToContentsWidth()`. 

However, if the order is:

1- blink::RotationViewportAnchor::SetAnchor()
2- blink::PageScaleConstraints::FitToContentsWidth()
3- blink::RotationViewportAnchor::RestoreToAnchor()

The problem happens. `SetAnchor()` stores 0.25f in its `old_minimum_page_scale_factor_` before `FitToContentsWidth()` resets it to 1.f.


I believe this is something specific to tablet mode on chromeos. Going to Tablet Mode maximizes the window, so the webcontents are laid out while those webkit prefs are being modified.

In the correct situation, when FitToContentsWidth() is called first, it's called from this path:


|#2 0x7f381b08d840 blink::PageScaleConstraints::FitToContentsWidth()
|#3 0x7f381b08e1a2 blink::PageScaleConstraintsSet::AdjustFinalConstraintsToContentsSize()
|#4 0x7f381afc3c48 blink::WebViewImpl::RefreshPageScaleFactorAfterLayout()
|#5 0x7f381afc53b1 blink::WebViewImpl::ResizeAfterLayout()
|#6 0x7f381b4c84f4 blink::LayoutView::UpdateAfterLayout()
|#7 0x7f381b3cc15b blink::LayoutBlockFlow::UpdateBlockLayout()
|#8 0x7f381b4c6011 blink::LayoutView::UpdateBlockLayout()
|#9 0x7f381b3bf8f8 blink::LayoutBlock::UpdateLayout()
|#10 0x7f381b4c635f blink::LayoutView::UpdateLayout()
|#11 0x7f381b06d99c blink::LocalFrameView::PerformLayout()
|#12 0x7f381b06bbbf blink::LocalFrameView::UpdateLayout()
|#13 0x7f381b07827b blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursiveInternal()
|#14 0x7f381b0756e9 blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursive()
|#15 0x7f381b074c9d blink::LocalFrameView::RunStyleAndLayoutLifecyclePhases()
|#16 0x7f381b0743ef blink::LocalFrameView::UpdateLifecyclePhasesInternal()
|#17 0x7f381b07356d blink::LocalFrameView::UpdateLifecyclePhases()
|#18 0x7f381b0733b4 blink::LocalFrameView::UpdateAllLifecyclePhases()
|#19 0x7f381b64d4be blink::PageAnimator::UpdateAllLifecyclePhases()
|#20 0x7f381afc03e2 blink::WebViewImpl::UpdateLifecycle()
|#21 0x7f381b0da13b blink::WebViewFrameWidget::UpdateLifecycle()
|#22 0x7f3823e77834 content::RenderWidget::UpdateVisualState()
|#23 0x7f3822816d22 cc::ProxyMain::BeginMainFrame()
|#24 0x7f38228158e2 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIMN2cc9ProxyMainEFvNSt3__110unique_ptrINS4_28BeginMainFrameAndCommitStateENS6_14default_deleteIS8_EEEEENS_7WeakPtrIS5_EEJSB_EEEvOT_OT0_DpOT1_
|#25 0x7f38228157d1 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc9ProxyMainEFvNSt3__110unique_ptrINS3_28BeginMainFrameAndCommitStateENS5_14default_deleteIS7_EEEEEJNS_7WeakPtrIS4_EENS0_13PassedWrapperISA_EEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
|#26 0x7f38259ef745 base::debug::TaskAnnotator::RunTask()
|#27 0x7f3825a7c090 base::sequence_manager::internal::ThreadControllerImpl::DoWork()
|#28 0x7f3825a7dcab _ZN4base8internal7InvokerINS0_9BindStateIMNS_16sequence_manager8internal20ThreadControllerImplEFvNS5_8WorkTypeEEJNS_7WeakPtrIS5_EES6_EEEFvvEE3RunEPNS0_13BindStateBaseE
|#29 0x7f38259ef745 base::debug::TaskAnnotator::RunTask()
|#30 0x7f3825a1c34e base::MessageLoop::RunTask()
|#31 0x7f3825a1c773 base::MessageLoop::DoWork()
|#32 0x7f3825a1fd56 base::MessagePumpDefault::Run()
|#33 0x7f3825a1bee4 base::MessageLoop::Run()
|#34 0x7f3825a4e449 base::RunLoop::Run()
|#35 0x7f3823e863d6 content::RendererMain()
|#36 0x7f3823f554f6 content::RunZygote()
|#37 0x7f3823f565c5 content::ContentMainRunnerImpl::Run()
|#38 0x7f3825da3005 service_manager::Main()
|#39 0x7f3823f54bd4 content::ContentMain()
|#40 0x562ce27330d3 ChromeMain
|#41 0x7f38164752b1 __libc_start_main
|#42 0x562ce2732f4a _start

Thanks, I'll take a look today.

Issue 866682 has been merged into this issue.

Ok - I think I've found the root cause. When we set the default page scale limits we clear but don't immediately recompute the final minimum scale based on the content width - that happens in RefreshPageScaleFactorAfterLayout. If we get the resize before the layout then we'll use the incorrect minimum PSF in the anchor causing the issue.

There's a number of additional things going wrong here (e.g. we're assuming we're doing a rotation) that need fixing. Much of this code was written with Android assumptions baked in. I'll put up a CL shortly.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a03511a9a8b2970388cbc05fd497e7ef216c966a

commit a03511a9a8b2970388cbc05fd497e7ef216c966a
Author: David Bokan <bokan@chromium.org>
Date: Thu Sep 13 04:25:24 2018

Recompute content limits inside PageScaleConstraintsSet

This patch fixes an issue where a page would become zoomed in when
ChromeOS' tablet mode is engaged. The issue here is that tablet mode
changes the default page scale limits and this doesn't recalculate the
correct final limits.

Page scale limits are calculated as a combination of (in order of
increasing precedance): user-agent defined default limits, page defined
limits (meta tag), user-agent overrides. In addition, if the
shrinks_viewport_contents_to_fit flag is enabled, the
final minimum scale factor is constrained to the page's content width
(i.e. so that the page can't be zoomed out further than the content).

Currently, the final "shrinks_viewport_contents_to_fit" constraint is
added separately from the other PageScaleConstraintsSet calculations,
after a layout, in RefreshPageScaleFactorAfterLayout. This is because we
need to layout in order to know the content width.

This bug occurs because tablet-mode changes the default page scale
limits. When that happens, the final constraints are recomputed
immediately but we still just wait for a layout to cause the content
width constraint to be applied. If we read the minimum scale factor
before we layout, it'll be incorrect. This happens in a Resize message
which (incorrectly) assumes we're rotating and sets a rotation anchor,
which uses the minimum page scale factor to adjust the current page
scale factor, causing the bug.

This patch moves the content width constraint to happen as part of the
final constraints computation so that its available immediately after
changing the default limits. Doing so required moving around some code
and making PageScaleConstraintsSet be garbage collected.

Additionally, we now set the "shrinks viewport contents to fit"
setting before changing the default scale limits to ensure it's
available when the scale factor changes.

Bug:  831473 
Change-Id: Id60ff15c93b4b74ee0369a80c45f39632ea17f9d
Reviewed-on: https://chromium-review.googlesource.com/1208898
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590923}
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/content/public/common/web_preferences.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/content/renderer/render_view_impl.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/exported/web_settings_impl.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/exported/web_settings_impl.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/page_scale_constraints_set.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/page_scale_constraints_set.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/rotation_viewport_anchor.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/rotation_viewport_anchor.h
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/settings.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/frame/settings.json5
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/a03511a9a8b2970388cbc05fd497e7ef216c966a/third_party/blink/renderer/core/page/page.h

The patch above should fix the symptom but I think tablet mode is using some settings it probably shouldn't. If "viewport" is disabled we should probably also turn off "shrinks_viewport_contents_to_fit" and "default_minimum_page_scale_factor" should be set to 1. We're not getting anything from them currently since we can only "zoom out" when "viewport_enabled == true". The current values for these settings don't expect to be set when "viewport_enabled = false" so we're likely to have more bugs like this.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e

commit 432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e
Author: David Bokan <bokan@chromium.org>
Date: Fri Sep 14 19:25:26 2018

Improve rotation check on viewport resize

Since rotation used to be supported only on Android, where windows can't
be resized, we made the assumption that if the width changes it must be
a rotation.

This changes in ChromeOS where entering tablet mode enables rotation but
also resizes the window (when entering tablet mode). This caused
inappropriate rotation anchoring.

The underlying issue has been fixed in other patches linked to this bug,
the rotation trigger should be improved to check that the width and
height are swapped.

Bug:  831473 
Change-Id: I0acd39d16319d8cb7819faac4c4ce54b6f7a2e46
Reviewed-on: https://chromium-review.googlesource.com/1224711
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591430}
[modify] https://crrev.com/432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e/third_party/blink/renderer/core/frame/rotation_viewport_anchor_test.cc
[modify] https://crrev.com/432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e/third_party/blink/renderer/core/html/image_document.h
[modify] https://crrev.com/432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e/third_party/blink/renderer/core/html/image_document_test.cc

Hi, any update on this issue? Can we merge it as well?

It's not a trivial change, so I'd be a little nervous merging it this late in M70. How frequently are we hitting this? Does it justify a bit of risk?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb643a6b869cf13fc8a00a7c259073925c531434

commit eb643a6b869cf13fc8a00a7c259073925c531434
Author: David Bokan <bokan@chromium.org>
Date: Mon Sep 24 14:58:14 2018

Revert "Improve rotation check on viewport resize"

This reverts commit 432553b5cb40dde1ae7ac1817dc7e2deecbf8e4e.

Reason for revert: width == height isn't enough since the system bar affects only height. Broke 887064, 887330, 887899

Original change's description:
> Improve rotation check on viewport resize
>
> Since rotation used to be supported only on Android, where windows can't
> be resized, we made the assumption that if the width changes it must be
> a rotation.
>
> This changes in ChromeOS where entering tablet mode enables rotation but
> also resizes the window (when entering tablet mode). This caused
> inappropriate rotation anchoring.
>
> The underlying issue has been fixed in other patches linked to this bug,
> the rotation trigger should be improved to check that the width and
> height are swapped.
>
> Bug:  831473 
> Change-Id: I0acd39d16319d8cb7819faac4c4ce54b6f7a2e46
> Reviewed-on: https://chromium-review.googlesource.com/1224711
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#591430}

TBR=bokan@chromium.org,dtapuska@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  831473 ,887899, 887064 , 887330 
Change-Id: Ib0f3cad366a80506effc62fcc352e1c75f2fcd55
Reviewed-on: https://chromium-review.googlesource.com/1239726
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593536}
[modify] https://crrev.com/eb643a6b869cf13fc8a00a7c259073925c531434/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/eb643a6b869cf13fc8a00a7c259073925c531434/third_party/blink/renderer/core/frame/rotation_viewport_anchor_test.cc
[modify] https://crrev.com/eb643a6b869cf13fc8a00a7c259073925c531434/third_party/blink/renderer/core/html/image_document.h
[modify] https://crrev.com/eb643a6b869cf13fc8a00a7c259073925c531434/third_party/blink/renderer/core/html/image_document_test.cc

Users are hitting it a lot, it's one of the top feedback we are getting.

Ok, requesting merge for CL in #31 - it's been in ToT for almost two weeks and haven't gotten any bugs/complaints so it should be fairly baked.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This is a P1 for Tablet.  Do we have an ETA for this landing?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ef846bae155b8fac9416398c628d892b11c4487

commit 9ef846bae155b8fac9416398c628d892b11c4487
Author: David Bokan <bokan@chromium.org>
Date: Tue Oct 02 16:03:02 2018

Recompute content limits inside PageScaleConstraintsSet

This patch fixes an issue where a page would become zoomed in when
ChromeOS' tablet mode is engaged. The issue here is that tablet mode
changes the default page scale limits and this doesn't recalculate the
correct final limits.

Page scale limits are calculated as a combination of (in order of
increasing precedance): user-agent defined default limits, page defined
limits (meta tag), user-agent overrides. In addition, if the
shrinks_viewport_contents_to_fit flag is enabled, the
final minimum scale factor is constrained to the page's content width
(i.e. so that the page can't be zoomed out further than the content).

Currently, the final "shrinks_viewport_contents_to_fit" constraint is
added separately from the other PageScaleConstraintsSet calculations,
after a layout, in RefreshPageScaleFactorAfterLayout. This is because we
need to layout in order to know the content width.

This bug occurs because tablet-mode changes the default page scale
limits. When that happens, the final constraints are recomputed
immediately but we still just wait for a layout to cause the content
width constraint to be applied. If we read the minimum scale factor
before we layout, it'll be incorrect. This happens in a Resize message
which (incorrectly) assumes we're rotating and sets a rotation anchor,
which uses the minimum page scale factor to adjust the current page
scale factor, causing the bug.

This patch moves the content width constraint to happen as part of the
final constraints computation so that its available immediately after
changing the default limits. Doing so required moving around some code
and making PageScaleConstraintsSet be garbage collected.

Additionally, we now set the "shrinks viewport contents to fit"
setting before changing the default scale limits to ensure it's
available when the scale factor changes.

Bug:  831473 
Change-Id: Id60ff15c93b4b74ee0369a80c45f39632ea17f9d
Reviewed-on: https://chromium-review.googlesource.com/1208898
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#590923}(cherry picked from commit a03511a9a8b2970388cbc05fd497e7ef216c966a)
Reviewed-on: https://chromium-review.googlesource.com/1257183
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#815}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/content/public/common/web_preferences.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/content/renderer/render_view_impl.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/public/web/web_view.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/exported/web_settings_impl.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/exported/web_settings_impl.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/exported/web_view_impl.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/page_scale_constraints_set.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/page_scale_constraints_set.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/rotation_viewport_anchor.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/rotation_viewport_anchor.h
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/settings.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/frame/settings.json5
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/9ef846bae155b8fac9416398c628d892b11c4487/third_party/blink/renderer/core/page/page.h

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/9ef846bae155b8fac9416398c628d892b11c4487

Commit: 9ef846bae155b8fac9416398c628d892b11c4487
Author: bokan@chromium.org
Commiter: bokan@chromium.org
Date: 2018-10-02 16:03:02 +0000 UTC

Recompute content limits inside PageScaleConstraintsSet

This patch fixes an issue where a page would become zoomed in when
ChromeOS' tablet mode is engaged. The issue here is that tablet mode
changes the default page scale limits and this doesn't recalculate the
correct final limits.

Page scale limits are calculated as a combination of (in order of
increasing precedance): user-agent defined default limits, page defined
limits (meta tag), user-agent overrides. In addition, if the
shrinks_viewport_contents_to_fit flag is enabled, the
final minimum scale factor is constrained to the page's content width
(i.e. so that the page can't be zoomed out further than the content).

Currently, the final "shrinks_viewport_contents_to_fit" constraint is
added separately from the other PageScaleConstraintsSet calculations,
after a layout, in RefreshPageScaleFactorAfterLayout. This is because we
need to layout in order to know the content width.

This bug occurs because tablet-mode changes the default page scale
limits. When that happens, the final constraints are recomputed
immediately but we still just wait for a layout to cause the content
width constraint to be applied. If we read the minimum scale factor
before we layout, it'll be incorrect. This happens in a Resize message
which (incorrectly) assumes we're rotating and sets a rotation anchor,
which uses the minimum page scale factor to adjust the current page
scale factor, causing the bug.

This patch moves the content width constraint to happen as part of the
final constraints computation so that its available immediately after
changing the default limits. Doing so required moving around some code
and making PageScaleConstraintsSet be garbage collected.

Additionally, we now set the "shrinks viewport contents to fit"
setting before changing the default scale limits to ensure it's
available when the scale factor changes.

Bug:  831473 
Change-Id: Id60ff15c93b4b74ee0369a80c45f39632ea17f9d
Reviewed-on: https://chromium-review.googlesource.com/1208898
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#590923}(cherry picked from commit a03511a9a8b2970388cbc05fd497e7ef216c966a)
Reviewed-on: https://chromium-review.googlesource.com/1257183
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#815}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

Merged back to M70.