New issue
Advanced search Search tips

Issue 891471 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Unable to open drop down menus in webpages in tablet mode

Project Member Reported by afakhry@chromium.org, Oct 2

Issue description

On ToT, see attached video. This is related to top-chrome hiding. It doesn't happen outside tablet mode, nor when the SlideTopChromeWithPageScrolls feature is disabled.
 
drop_downs-2018-10-02_13.17.51.mp4
139 KB View Download
Labels: ReleaseBlock-Beta
Cc: omrilio@chromium.org
This happens as a result of opening the drop-down, which creates a new RenderWidget, which in turn creates a LayerTreeView > LayerTreeHost > LayerTreeHostImpl > LayerTreeImpl.

#2 0x7f29a9445a46 content::RenderWidget::InitializeLayerTreeView()
#3 0x7f29a015d62f blink::WebPagePopupImpl::InitializeLayerTreeView()
#4 0x7f29a015d3ef blink::WebPagePopupImpl::Initialize()
#5 0x7f29a0176a44 blink::WebViewImpl::OpenPagePopup()
#6 0x7f29a034dee9 blink::InternalPopupMenu::Show()
#7 0x7f29a033df1b blink::HTMLSelectElement::ShowPopup()
#8 0x7f29a033e6a9 blink::HTMLSelectElement::MenuListDefaultEventHandler()
#9 0x7f29a033f7bd blink::HTMLSelectElement::DefaultEventHandler()

This newly created LayerTreeImpl is initialized with identity `top_controls_shown_ratio_` which is 0.f.

When it's time to generate a new compositor frame, this 0.f value of `top_controls_shown_ratio_` is used to generate a new RenderFrameMetaData, which is then sent to the Browser here:

#1 content::RenderFrameMetadataObserverImpl::OnRenderFrameSubmission()
#2 cc::LayerTreeHostImpl::GenerateCompositorFrame()
#3 cc::LayerTreeHostImpl::DrawLayers()

The browser's `RenderWidgetHostViewAura::OnDidUpdateVisualPropertiesComplete()` ends up sending it to the `TopControlsSlideControllerChromeOS::SetShownRatio()`, which causes the shown ratio to be set to 0 very briefly.

I believe Android avoids this by initializing `RenderWidgetHostViewAndroid::prev_top_shown_pix_` to 0.f [1], so for a new renderer, `top_changed` is false [2], and we don't send the value to the Java side.


[1]: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?sq=package:chromium&q=prev_top_shown_pix_&g=0&l=182

[2]: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?sq=package:chromium&q=RenderWidgetHostViewAndroid::UpdateControls&g=0&l=1342-1343
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 5

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

commit faa32d27c2b6851e86f309417cdfc6c6c2e97872
Author: Ahmed Fakhry <afakhry@chromium.org>
Date: Fri Oct 05 15:56:16 2018

top-chrome slide: set shown ratio for main frame widget only

Opening a drop-down menu in a webpage will create a new
RenderWidget with a new LayerTreeHostImpl, whose value
of the top_controls_shown_ratio_ (initialized to 0) will
be pushed to the browser when a new compositor frame is
generated. This will cause top-chrome to hide briefly
on opening the menu which will result in closing the
menu immediately.

This CL fixes the issue by allowing only the main frame
widget to set the shown ratio.

BUG= 891471 
TEST=Manual, added a new browser test.

Change-Id: I8ead7f903d56e68523a7654abc21aa6e5ca7b690
Reviewed-on: https://chromium-review.googlesource.com/c/1258348
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597146}
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/chrome/browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/chrome/test/data/top_controls_scroll/top_controls_scroll.html
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/faa32d27c2b6851e86f309417cdfc6c6c2e97872/content/browser/web_contents/web_contents_impl.h

Status: Fixed (was: Started)

Sign in to add a comment