Attach the test2.html for test

Deleted: test2.html 1.0 KB

test2.html 1.0 KB View Download

*sigh*, my phone doesn't show it as HTML when I open it from my downloads

Also, is there a reason the HTML is in quirks mode?

Going to try this one: http://output.jsbin.com/kijeguqace

Oh, it's quirks mode so you don't have to set a height on html/body, ok.

What I see when I do this is that the video ends up at the bottom of the screen, and the viewport is bigger than the screen size (?). Maybe a viewport issue?

What do you mean by viewport? window.innerWidth/Height? or one of LayoutView/FrameView's internal sizes?

Uh, I guess I meant that I can scroll even though things are supposedly screen-sized (ie. 100%)

Right, but I did some digging on this when loading just a webm file and found that the reason was that the LayoutVideo (IIRC) was explicitly being given a large y offset. Clicking buttons in the video player (and causing layout) would shrink the y offset by half each time. I don't know layout well enough to diagnose where that's coming from. I did capture a stack trace of how the video gets the vertical offset, though I didn't think to record the viewport size, I'll look into that again. In the mean time, here's the trace, though I'm not sure how much use it is:

#0  setLocation () at ../../third_party/WebKit/Source/core/layout/LayoutBox.h:291
#1  0x9c7dfa8a in setFlowAwareLocationForChild () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:686
#2  0x9c7e10bc in adjustAlignmentForChild () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1646
#3  0x9c7de8d6 in alignChildren () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1688
#4  0x9c7de2c0 in repositionLogicalHeightDependentFlexItems () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:327
#5  0x9c7de0c8 in layoutFlexItems () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:830
#6  0x9c7ddae4 in layoutBlock () at ../../third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:289
#7  0x9c7af832 in layout () at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:879
#8  0x9c7b9e52 in positionAndLayoutOnceIfNeeded () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:596
#9  0x9c7b9ff6 in layoutBlockChild () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:646
#10 0x9c7bbd98 in layoutBlockChildren () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1122
#11 0x9c7b94a4 in layoutBlockFlow () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:417
#12 0x9c7b90f6 in layoutBlock () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:325
#13 0x9c7af832 in layout () at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:879
#14 0x9c7b9e52 in positionAndLayoutOnceIfNeeded () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:596
#15 0x9c7b9ff6 in layoutBlockChild () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:646
#16 0x9c7bbd98 in layoutBlockChildren () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1122
#17 0x9c7b94a4 in layoutBlockFlow () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:417
#18 0x9c7b90f6 in layoutBlock () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:325
#19 0x9c7af832 in layout () at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:879
#20 0x9c7b9e52 in positionAndLayoutOnceIfNeeded () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:596
#21 0x9c7b9ff6 in layoutBlockChild () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:646
#22 0x9c7bbd98 in layoutBlockChildren () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1122
#23 0x9c7b94a4 in layoutBlockFlow () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:417
#24 0x9c7b90f6 in layoutBlock () at ../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:325
#25 0x9c7af832 in layout () at ../../third_party/WebKit/Source/core/layout/LayoutBlock.cpp:879
#26 0x9c833e5a in layoutContent () at ../../third_party/WebKit/Source/core/layout/LayoutView.cpp:191
#27 0x9c83419a in layout () at ../../third_party/WebKit/Source/core/layout/LayoutView.cpp:291
#28 0x9c62cd40 in layoutFromRootObject () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:819
#29 performLayout () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:883
#30 0x9c62d718 in layout () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:1032
#31 0x9c631c4e in updateStyleAndLayoutIfNeededRecursiveInternal () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2590
#32 0x9c63115e in updateStyleAndLayoutIfNeededRecursive () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2570
#33 0x9c630c00 in updateLifecyclePhasesInternal () at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2416
#34 0x9c70cca8 in updateAllLifecyclePhases () at ../../third_party/WebKit/Source/core/page/PageAnimator.cpp:82
#35 0x9b605b20 in updateAllLifecyclePhases () at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:1976
#36 0x9b606bc8 in resizeViewWhileAnchored () at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:1882
#37 0x9b606d36 in resize () at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:1919
#38 0x9ae9246a in Resize () at ../../content/renderer/render_widget.cc:1215
#39 0x9ae8ac4a in OnResize () at ../../content/renderer/render_view_impl.cc:2595

(previous comment went to the wrong bug, so I deleted it)

bokan - basically that function will center the flex item in the flexbox, so the size of the LayoutFlexibleBox and the viewport are really important here.

Thanks, I'll check sizes of both

I'm now fairly certain this is not a viewport issue, all the viewport values are correct by the time we get around to layout (in particular, FrameView::m_layoutSize which is the only one that should matter). I think I've narrowed down the problem but I'm not sure what the solution might be or why it's happening.

(See bokan.ca/bugs/606423.html for repro, the Download button is not necessary)

When the page first loads in portrait, the "#content" DIV's LayoutBox will get an m_intrinsicContentLogicalHeight equal to the height of the video + space above. We then rotate the phone. While we're in layout() for #content, we call LayoutFlexBox::layoutBlock -> LayoutFlexBox::layoutFlexItems. Here, we compute the availableFreeSpace: sumFlexBaseSize is correctly the video's height while containerMainInnerSize is still equal to the old m_intrinsicContentLogicalHeight calculated during portrait orientation. I suspect the bug here is that we didn't call updateLogicalHeight on the #content DIV yet and, sure enough, that seems to happen at the end of layoutFlexItems. This means the LayoutFlexBox thinks there's more free space than there actually is.

I'm not sure if the bug is that the intrinsicContentLogicalHeight is out of date or that we shouldn't be using it at all. At the time we compute the free space, nothing in the layout tree above the VIDEO element has a height (in m_frameRect) yet. In any case, I'm not familiar enough with layout to make any real prognosis so take this with a grain of salt :)

Over to Christian. 

Thanks! I'll keep investigating.

Flexbox doesn't use m_intrinsicContentLogicalHeight for this purpose (uh... not intentionally anyway). In mainAxisContentExtent we calculate it in such a way that it should be correct for this purpose. So it's actually weird that it's wrong...

dgrogan -- do you want to take an exciting opportunity to learn about flexbox code and fix this bug? :-)

Based on comment 16 this ought to largely consist of figuring out why mainAxisContentExtent returns outdated values after rotation.

Sure, but only if it can wait a few weeks. Can it?

We are planning on launching the feature which would get affected by this with M52. We'd like to get the fix in before that if possible.

David, can you explain Android viewports to me? I got in several different states and don't fully understand them all.

https://goo.gl/photos/dDueLXGXBWn8ZnU3A

In the third picture there, landscape with URL bar, what is the viewport size supposed to be? I guess- how does the presence of the URL bar affect the height of the page?  I noticed that there's a relayout happening when the URL bar disappears, which rather surprised me (see the difference in margins between picture 1 and 3).

But I believe (?) that the actual bug is shown in the last picture, triggered by rotating the phone from portrait to landscape while the urlbar is not being shown. It produces a different result from other ways to get to landscape w/o urlbar, which definitely seems like a bug.

The WebViewImpl's size, and thus the layout size, does not include the top controls. This means that when top controls are hidden/shown we get a resize from the renderer and we update the layout. (Side note: this is something I'm trying to change in  issue 428132 ).

So the difference between the first picture and the third is that, in the third, the layout height will be smaller by the size of the top controls (~100px). I think the video is appropriately sized, just positioned incorrectly. That's why the margins are a little thicker, because the size is smaller due to the smaller layout height.

The last pic has seeming problem that the video has margins on all sides. My expectation is that it would be sized such that it fills one of the dimensions. Perhaps there's a max size though.

I think both the third *and* last pics show the problem that I looked at, the margin above the video. In the third, I don't think there should be a vertical margin at all (and it should be vertically centered in the last). In fact, once you get it in that state (e.g. third picture), try clicking the play/pause button a few times. I found that each time I pushed the button it would shrink the top margin by exactly half. This doesn't repro 100% of the time but at least 1/3 or so. I also didn't find top controls to make a difference.

FYI: I'm OOO for 2 weeks starting tomorrow. If you need more from me, today's the day :)

OK thanks. So:

with urlbar, height = screen height - urlbar [- android controls]
without urlbar: height = screen height [- android controls]

So we relayout when this changes.

Hmm, thanks for the additional detail, I'll keep investigating with that

That's correct.

simple fix: https://codereview.chromium.org/1971053003

As you suggested/predicted, the video does now also cover the full width of the page. (and is centered)

Now I just have to make sure this didn't break any tests & write a testcase!

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

commit ed528b2c5a2eef209222118fc34b7d9e325c9939
Author: cbiesinger <cbiesinger@chromium.org>
Date: Thu May 12 23:02:38 2016

Use new intrinsic content height when computing min-height: min-content

Instead of using the cached intrinsic content height, we should use
the newly provided one, because that one is not out of date.

BUG= 606423 

Review-Url: https://codereview.chromium.org/1971053003
Cr-Commit-Position: refs/heads/master@{#393385}

[add] https://crrev.com/ed528b2c5a2eef209222118fc34b7d9e325c9939/third_party/WebKit/LayoutTests/css3/flexbox/resize-min-content-flexbox.html
[modify] https://crrev.com/ed528b2c5a2eef209222118fc34b7d9e325c9939/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/ed528b2c5a2eef209222118fc34b7d9e325c9939/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

Your change meets the bar and is auto-approved for M51 (branch: 2704)

Fixed in 52.0.2735.0 and am now merging to the 51 branch too.

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

commit 65bf99891b6b06580139f4ecd8944ab7789b6980
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Fri May 13 16:31:10 2016

Use new intrinsic content height when computing min-height: min-content

Instead of using the cached intrinsic content height, we should use
the newly provided one, because that one is not out of date.

BUG= 606423 

Review-Url: https://codereview.chromium.org/1971053003
Cr-Commit-Position: refs/heads/master@{#393385}
(cherry picked from commit ed528b2c5a2eef209222118fc34b7d9e325c9939)

Review URL: https://codereview.chromium.org/1980603002 .

Cr-Commit-Position: refs/branch-heads/2704@{#536}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[add] https://crrev.com/65bf99891b6b06580139f4ecd8944ab7789b6980/third_party/WebKit/LayoutTests/css3/flexbox/resize-min-content-flexbox.html
[modify] https://crrev.com/65bf99891b6b06580139f4ecd8944ab7789b6980/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/65bf99891b6b06580139f4ecd8944ab7789b6980/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp