1586144 - Use the viewport size including maximum possible dynamic toolbar size to resolve viewport units even if the dynamic toobar is hidden and make ICB size static whatever the dynamic toolbar state is

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

It's what Chrome does. And also the size which is not including the dynamic toolbar even if the dynamic toolbar is shown is going to be used for layout stuff (ICB and some such).

To resolve viewport units we use nsPresContext::mVisibleArea, we need to change it. We will add a new variable for the size including the toolbar size in nsPresContext for the top content document and use it there.

Note that the current dynamic toolbar implementation depends on CoordinatorLayout which means the size value which is delivered from GeckoView to nsPresContext::mVisibleArea is INCLUDING the toolbar size, whereas if the consumer of GeckoView doesn't use CoordinatorLayout just like GeckoView Example, the size is NOT including the toolbar size. I've been searching ways to tell whether GeckoView is used in CoordinatorLayout as a child, but I haven't found good ways.

Another thing I am still wondering the way to deliver the toolbar size to the content prescontext.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I thought that mVisibleArea is what we wanted to use, and that's what wouldn't change via the dynamic toolbar stuff?

Anyhow... I assume viewport units and media queries both should be resolved using the same thing... right?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

You mean we want to use mVisibleArea to resolve vh units? And we will have a new value for ICB or something in nsPresContext? I think with the way we need to change nsPresContext::GetVisibleArea call sites. But I might be missing something.

Anyways, both size variable is not going to be changed by the dynamic toolbar transitions, both are static.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ah I thought there was only one static value, not two... Then I guess whatever's easier :)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

With fixes for bug 1590582, I don't see any issues on my patches. (though I am still seeing an issue that accessible caret is wrongly positioned in position:fixed elements but I think it's due to bug 1526268)

One caveat is that with changes in this bug, the area covered by the dynamic toolbar is outside of the content area, so implementers of GeckoView need to call setVerticalClipping with negative value. here is the change I've been using.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5014222b60cc17a5c0986998661d86349bd327c

A JUnit test added for this bug fails frequently on opt builds, hopefully it will be fixed by rewriting screenshot machinery on mobile (bug 1577192), if it won't fix, I will probably disable the test on opt builds.

Also note that I wanted to write fullscreen enter/exit tests but what happens on fullscreen state is totally out of controls of Gecko, for example, in reference-browser they toggle the toolbar state, but we have no way to tell it in Gecko so I gave up writing such tests. FWIW, here is a change for reference-browser I've been using locally for the fullscreen state, with this change on fullscreen state, fullscreen content is rendered on whole screen.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586144 - Drop m{Start,Last}Dist in android/nsWindow.h. r?botond

It's not used in the first place.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586144 - Introduce an API to set the dynamic toolbar maximum height in GeckoView. r?#geckoview-reviewers!,tnikkel!

And deliver the value to the top content pres context, but it's not used in
this commit. The value will be used in the next commit.

One caveat is that areas covered by the dynamic toolbar will be outside
of the content area, which means implementers of GeckoView needs to call
setVerticalClipping with negative values.

Depends on D50416

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586144 - Factor dynamic toolbar max height into layout metrics. r?emilio!,botond!

Now

• nsPresContext::mVisibleArea is excluding the toolbar max height so that
  ICB is now static regardless of the dynamic toolbar transition
• nsPresContext::mSizeForViewportUnits is introduced to resolve viewport units
  which is including the toolbar max height

That means that with the dynamic toolbar max height;

mVisibleArea < mSizeForViewportUnits

Depends on D50417

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586144 - Expand the clipt rect for visual viewport scrollable display item. r?mstange,mattwoodrow

Without this change, the junit test in this commit fail, the failure
rendered result is the area of the position:fixed element covered by
the dynamic toolbar before scrolling is rendered as blank.

Depends on D50418

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1586144 - Expand the FrameMetrics.mLayoutViewport to the size for viewport units. r?botond

Note that this FrameMetrics.mLayoutViewport doesn't represent exact size of
the layout viewport on the main thread, it means the maximum layout viewport
in future on the compositor thread once after the dynamic toolbar is completely
hidden. During the dynamic toolbar transition we don't update any information
on the main thread, which means it's possible that the visual viewport on the
compositor gets bigger than the layout viewport at the time when we send it
to the compositor thread, we have to avoid the situation.

Depends on D50419

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9087a6d08795
Drop m{Start,Last}Dist in android/nsWindow.h. r=botond
https://hg.mozilla.org/integration/autoland/rev/b79f9f171fb1
Introduce an API to set the dynamic toolbar maximum height in GeckoView. r=geckoview-reviewers,tnikkel,snorp
https://hg.mozilla.org/integration/autoland/rev/9f96406e2da1
Factor dynamic toolbar max height into layout metrics. r=emilio,botond
https://hg.mozilla.org/integration/autoland/rev/1f6692cbceba
Expand the clipt rect for visual viewport scrollable display item. r=botond
https://hg.mozilla.org/integration/autoland/rev/828246fa2bdf
Expand the FrameMetrics.mLayoutViewport to the size for viewport units. r=botond

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9087a6d08795
https://hg.mozilla.org/mozilla-central/rev/b79f9f171fb1
https://hg.mozilla.org/mozilla-central/rev/9f96406e2da1
https://hg.mozilla.org/mozilla-central/rev/1f6692cbceba
https://hg.mozilla.org/mozilla-central/rev/828246fa2bdf

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I made a PR for android-components to just add the new API setDynamicToolbarMaxHeight.

To make the dynamic toolbar work as expected, we still need to set the maximum height in reference-browser and we need to call setVerticalClipping with minus values.