1503656 - Android cutout support for CSS env() safe area insets

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[Chris Peterson [:cpeterson]]

We need to detect the display cutout in order to get the safe areas for env() API bug 1462233.

See https://webkit.org/blog/7929/designing-websites-for-iphone-x/ for fancy diagrams.

See https://bugs.chromium.org/p/chromium/issues/detail?id=825890 for Blink's Android implementation.

Emilio needs to know the safe area insets in nsIDocument / nsPresContext, similar with how viewport works. The insets do change when orientation changes and such though.

Comment 1

[Chris Peterson [:cpeterson]]

Makoto, are you actively working on Android P support now? Can you include this bug as part of the Android P work?

Comment 2

[Makoto Kato [:m_kato]]

(In reply to Chris Peterson [:cpeterson] from comment #1)
> Makoto, are you actively working on Android P support now? Can you include
> this bug as part of the Android P work?

Since android.test package is removed from P SDK (https://developer.android.com/sdk/api_diff/28/changes), we cannot build RobocopTest when changing to P SDK (bug 1504105).

Comment 3

[Chris Peterson [:cpeterson]]

OK. I'll mark this bug as P3 because its blocked on Android P support and AFAIK doesn't block Emilio from landing bug 1462233 on desktop.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Bug 1503455 has been solved, now we can use the APIs for cutout?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

Bug 1503455 has been solved, now we can use the APIs for cutout?

Yes, we can use API 28's API.

Comment 7

[Chris Peterson [:cpeterson]]

James says this feature may be important for fullscreen PWAs or for Fenix without a top toolbar.

Comment 8

[Chris Peterson [:cpeterson]]

Gecko support already exists. Someone just needs to connect it to GV.

Comment 9

[Chris Peterson [:cpeterson]]

Bumping from priority P3 to milestone M9 because the Fenix issue is in Fenix's Q4 backlog:

https://github.com/mozilla-mobile/fenix/issues/4149

Comment 10

[Chris Peterson [:cpeterson]]

Linking to GeckoView's PWA meta bug 1561593

Comment 11

[Chris Peterson [:cpeterson]]

Tentatively scheduling for GV's October sprint.

Comment 12

[Chris Peterson [:cpeterson]]

We want to work on this in October.

Comment 13

[Makoto Kato [:m_kato]]

No binding interface for safe area implementation. Bug 1462233 uses hardcoded value for safe area.

Comment 14

[Tiger Oakes]

Could the safe area be adjusted from the Android Components side? It would be nice to later use this to support transparent navigation bars in addition to transparent status bars down the line.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Tiger Oakes from comment #14)

Could the safe area be adjusted from the Android Components side? It would be nice to later use this to support transparent navigation bars in addition to transparent status bars down the line.

Yeah, I think we'd probably implement this by passing a DisplayCutout[1] (or perhaps an internal type that's similar?) to GeckoDisplay, with GeckoView automatically setting the cutouts for the display it's on by default. We could then easily add something to GeckoView that allowed apps to set additional cutouts or safe areas.

[1] https://developer.android.com/reference/android/view/DisplayCutout

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Each safe area has a single value, so if you override the value given by the system, it's possible that the value is smaller than the system one. I don't think it's a good idea... Or additional areas means adding new env variables?

Comment 17

[Makoto Kato [:m_kato]]

I am writing prototype...

I think that safe area should be stored in nsIWidget (nsWindow and update it via BrowserParentChild), then stylo can get it from widget. safearea isn't static value (it is updated dynamically via changing View position/size by full screen etc) on Android.

GV side is stored in GeckoSession and GeckoDisplay, then it can update safearea to widget's.

emilio, is there a way to re-calcuate env() value? CssEnvironment is used at first parsing, but safearea is updated dynamically, so I want ot re-compute env() value. (RebuildAllStyleData won't help it. but my usage is mistake?)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Can you upload your WIP somehow? env() is evaluated at computed-value time, just like custom properties, so it should work, afaict.

Comment 19

[Makoto Kato [:m_kato]]

Attachment: WIP

This is WIP. This works on non-e10s mode only since I don't implement IPC.
And of course, I have to move CSsEnvironment to Gecko's specific implementation since Servo keeps static implementation.
I don't know how to change CssEnvironment to Unparsed state...

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9104567 [details] [diff] [review]
WIP

Review of attachment 9104567 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffect.cpp
@@ +1181,5 @@
> +            nsContentUtils::GetContextForContent(mTarget->mElement);
> +        if (!presContext) {
> +          aRv.Throw(NS_ERROR_FAILURE);
> +          return;
> +        }

I don't think you can do this, this will throw in display: none iframes. I honestly think we should just land https://phabricator.services.mozilla.com/D9625, this whole thing is a hack.

::: layout/style/GeckoBindings.cpp
@@ +1941,5 @@
>  void Gecko_PrintfStderr(const nsCString* aStr) {
>    printf_stderr("%s", aStr->get());
>  }
> +
> +static LayoutDeviceIntMargin Gecko_GetSafeArea(const Document* aDoc) {

nit: No Gecko_ prefix for static functions, and `aDoc` should never be null.

@@ +1945,5 @@
> +static LayoutDeviceIntMargin Gecko_GetSafeArea(const Document* aDoc) {
> +  if (aDoc) {
> +    nsPIDOMWindowOuter* window = aDoc->GetWindow();
> +    if (window) {
> +      nsCOMPtr<nsIWidget> widget =

Is refcounting this off the main thread thread-safe? Probably not, right?

If we store the VariableValues in the `CssEnvironment` this shouldn't be needed, I think. But again I'm happy to refactor that myself once you get it working or something if you don't have the cycles.

::: layout/style/GeckoBindings.h
@@ +692,5 @@
>      const mozilla::dom::Document*);
>  
>  void Gecko_PrintfStderr(const nsCString*);
>  
> +int Gecko_GetSafeAreaLeft(const mozilla::dom::Document*);

Should add a comment that they return CSS pixels. Also probably should be floats?

::: servo/components/style/custom_properties.rs
@@ +31,5 @@
> +///
> +/// XXX I should sparate CssEnvironment struct to gekco's and servo's like media query.
> +#[derive(Debug)]
> +pub struct CssEnvironment {
> +    document: *const structs::Document,

Instead of this, maybe we should pass the `&Device` down to get(), I think it should be straight-forward-ish. Either that or keep the safe area values in `CssEnvironment` (see below). That would make easier to implement the optimization to avoid a full document restyle even if the safe-area values are not used.

Even though `Device` lives in the media queries module, it is already more than that, as it's used for stuff like viewport units, body color quirk, etc.

@@ +56,5 @@
> +    let area_str = area.to_string() + "px";
> +    let mut input = ParserInput::new(&area_str);
> +    let mut input = Parser::new(&mut input);
> +    let (first_token_type, css, last_token_type) =
> +        parse_self_contained_declaration_value(&mut input, None).unwrap();

This is going to be a lot of overhead each time that the variable is evaluated, we should really keep these around.

I did the css parser dance because they were static, but we should do something like `VariableValue::pixels(number: f32) -> Self` that just constructs the token tree manually or something. Anyhow I can take care of that either as part of this patch or a followup if you prefer.

::: widget/android/nsWindow.cpp
@@ +2309,5 @@
> +  if (ps) {
> +    nsPresContext* pc = ps->GetPresContext();
> +    if (pc) {
> +      // XXX safe area is updated, so we have to rebuild style...
> +      pc->RebuildAllStyleData(nsChangeHint(0), RestyleHint::RecascadeSubtree());

I don't have a phone with a notch, so I cannot debug this, but a few thougts:

 * Is the prescontext here really for the top level content document? Or do you need to call it in subdocuments too?
 * I think conceptually `RecascadeSubtree` should work, but you could try `RestyleSubtree` which is a stronger hint to be 100% sure.
 * I also think we only want to do this if the page uses the `env()` variables, and ideally we'd only want to restyle the specific elements that use it, but that can be a followup.

::: widget/nsIWidget.h
@@ +2120,5 @@
>  
> +  /**
> +   * Get safe area except to cutout
> +   */
> +  virtual void GetSafeArea(mozilla::LayoutDeviceIntMargin& aSafeArea) {

Any reason this uses an outparam rather than returning a LayoutDeviceIntMargin?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

If the suggestions above for some reason don't work, is there any chance you can tell me how to test / debug it, maybe on the emulator?

Comment 22

[Makoto Kato [:m_kato]]

Thank you emilio.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

I don't have a phone with a notch, so I cannot debug this, but a few thougts:

• Is the prescontext here really for the top level content document? Or do
  you need to call it in subdocuments too?
• I think conceptually RecascadeSubtree should work, but you could try
  RestyleSubtree which is a stronger hint to be 100% sure.
• I also think we only want to do this if the page uses the env()
  variables, and ideally we'd only want to restyle the specific elements that
  use it, but that can be a followup.

RebuildAllSytles doesn't reparse env() block. MediaFeatureValuesChangedAllDocuments can re-parse env().

If the suggestions above for some reason don't work, is there any chance you can tell me how to test / debug it, maybe on the emulator?

Also, to test cutout, create Android 9 image by emulator, then set cutout by "Simulate a display with a cutout" option by developer option. This WIP has GVE fix that is full screen removes system notification area.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to Makoto Kato [:m_kato] from comment #22)

RebuildAllSytles doesn't reparse env() block. MediaFeatureValuesChangedAllDocuments can re-parse env().

Ah, cool, so it was really about which document you call it on. In non-e10s you're dealing with the root chrome document and the content document. You'd need to invalidate environment variables at least in the top level content document too, yeah.

Comment 24

[Chris Peterson [:cpeterson]]

Rolling over to November sprint.

Comment 25

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 3. Don't use hardcoded value for safearea.

Current implementation of safe area uses hard coded value.
To implement safe area support on Gecko, I need Gecko specific code for CssEnvironment.

Comment 26

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 1. Always use CssEnvironment from media query's device.

Although CssEnvironment is in Device of media query implementation, some code
creates CssEnvironment instance without Device. So I would like always to use it from Device of media query.

Depends on D52504

Comment 27

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 2. Use Device for parameter instead of CssEnvironment.

CssEnvironment alwasy is in Device, so use Devie as parameter instead of CssEnvironment.

Depends on D52506

Comment 28

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 4. Add safe area to nsIWidget.

Safe area value is stored in nsIWidget for stylo.

Depends on D52507

Comment 29

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 5. Get safe area insets from Gecko.

Add binding to get safe area from Gecko.

Depends on D52508

Comment 30

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 7. Notify of safe area insets changed.

Safe area may be changed by changing windows size and/or fullscreen mode. So it is observed from
parent process.

Depends on D52509

Comment 31

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 6. Rebuild CSS environment when safe area insets are changed. r?emilio

safe area is changed dynamically by window size is chaged. So if it is changed, we have to re-parse CSS.

Depends on D52510

Comment 32

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 8. Android implementation of safe area insets support

Depends on D52512

Comment 33

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 9. Add full screen support to GVE that notification area and etc are hidden.

Actually, although GVE supports full screen, but it doesn't disappears system notification area. To check cutout area,
I would like to change it that disappears notifications.

Depends on D52513

Comment 34

[Hiroyuki Ikezoe (:hiro)]

(In reply to Makoto Kato [:m_kato] from comment #33)

Created attachment 9107791 [details]
Bug 1503656 - Part 9. Add full screen support to GVE that notification area and etc are hidden.

Actually, although GVE supports full screen, but it doesn't disappears system notification area. To check cutout area,
I would like to change it that disappears notifications.

This should be applied only if there is "viewport-fit=cover" in a meta viewport tag. That's why I added bug 1574307 into the dependency list of this bug.

Comment 35

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)

(In reply to Makoto Kato [:m_kato] from comment #33)

Created attachment 9107791 [details]
Bug 1503656 - Part 9. Add full screen support to GVE that notification area and etc are hidden.

Actually, although GVE supports full screen, but it doesn't disappears system notification area. To check cutout area,
I would like to change it that disappears notifications.

This should be applied only if there is "viewport-fit=cover" in a meta viewport tag. That's why I added bug 1574307 into the dependency list of this bug.

Thanks, I should abandon this GVE fix from this series. We may add API and/or parameter for fullscreen API.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

One more note; WidgetUtils::DOMWindowToWidget looks apparently fission-incompatible, if it hasn't yet filed for that we should file a new bug. As for media query stuff in fission world, it's probably not yet worked, I've filed bug 1592895 but there may be other issues.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)

One more note; WidgetUtils::DOMWindowToWidget looks apparently fission-incompatible, if it hasn't yet filed for that we should file a new bug. As for media query stuff in fission world, it's probably not yet worked, I've filed bug 1592895 but there may be other issues.

bug 1597509 was just filed for this issue.

Comment 38

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 7. Notify of safe area insets changed.

Depends on D52512

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Makoto, mind if I land some of the CSS changes here (preserving you as an author of course) so that other contributors don't need to repeat this work in bug 1432090?

Comment 40

[Makoto Kato [:m_kato]]

Emilio, should I land layout part of this bug with leave-open? Latest 2 patch is DOM/IPC and Android part. So I won't add more layout side patch for this bug.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

Sure, that works for me :-)

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63a8102f2ffb
Part 1. Always use CssEnvironment from media query's device. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e7231baeb836
Part 2. Use Device for parameter instead of CssEnvironment. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b8ab06d233b3
Part 3. Don't use hardcoded value for safearea. r=emilio

Comment 43

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/63a8102f2ffb
https://hg.mozilla.org/mozilla-central/rev/e7231baeb836
https://hg.mozilla.org/mozilla-central/rev/b8ab06d233b3

Comment 44

[vzare]

Has this issue been fixed?

Comment 45

[Emily Toop (:fluffyemily)]

Makoto can you please update this issue with details of when the remaining patches might land?

Comment 46

[Makoto Kato [:m_kato]]

Emilio, what safearea value of embedded document? As long as last week discussion (https://github.com/w3c/csswg-drafts/issues/4670), you have responsive. Is it OK like Blink? (sub document is 0px) since Fenix runs on Android only and Blink doesn't run on iOS.

If sub document is 0px, I guess that we don't care for fission.

Comment 47

[Emilio Cobos Álvarez (:emilio)]

I think it's ok to behave like Blink for now. If the working group reconsiders we can always change the behavior there.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

I don't see any automated test cases in the patch set here. I think we should either have automated tests or do manual QA.

Given that on an Android phone having a cutout region at top of the phone, IIUC the spec, the behavior should be

1. Portrait mode on non fullscreen state, safe-area-inset-top should be zero, but on fullscreen mode it's the value of the cutout
2. Landscape mode both on fullscreen/non-fullscreen state, safe-area-inset-left should be the cutout region (given that the left side is now the top of the phone)

That's my understandings, but I could be wrong.

Comment 49

[Makoto Kato [:m_kato]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48)

I don't see any automated test cases in the patch set here. I think we should either have automated tests or do manual QA.

Given that on an Android phone having a cutout region at top of the phone, IIUC the spec, the behavior should be

1. Portrait mode on non fullscreen state, safe-area-inset-top should be zero, but on fullscreen mode it's the value of the cutout

GeckoView side doesn't know whether current web content isn't converted for notch. (hide navigation bar etc). And our test emulator is old version, so no way to add test on Android.

2. Landscape mode both on fullscreen/non-fullscreen state, safe-area-inset-left should be the cutout region (given that the left side is now the top of the phone)

It depends on application side. As general, navigation bar etc aren't hidden, so safe-area-insets will be 0. Its behavior for landscape is iOS, not Android.

Comment 50

[Makoto Kato [:m_kato]]

(In reply to Makoto Kato [:m_kato] from comment #49)

It depends on application side. As general, navigation bar etc aren't hidden, so safe-area-insets will be 0. Its behavior for landscape is iOS, not Android.

LAYOUT_IN_DISPLAY_CUTOUT_MODE_DEFAULT maybe iOS like behavior on Android.

Comment 51

[Makoto Kato [:m_kato]]

Attachment: Bug 1503656 - Part 9. Update API changes.

Depends on D52513

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91b847efe591
Part 4. Add safe area to nsIWidget. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f31efa4ea2ec
Part 5. Get safe area insets from Gecko. r=emilio
https://hg.mozilla.org/integration/autoland/rev/332ceea26151
Part 6. Rebuild CSS environment when safe area insets are changed. r=emilio
https://hg.mozilla.org/integration/autoland/rev/43fdc889beac
Part 7. Notify of safe area insets changed. r=smaug
https://hg.mozilla.org/integration/autoland/rev/7111f9b5ad06
Part 8. Android implementation of safe area insets support r=snorp
https://hg.mozilla.org/integration/autoland/rev/27faa3b167a9
Part 9. Update API changes. r=geckoview-reviewers,agi

Comment 53

[Natalia Csoregi [:nataliaCs]]

Backed out as requested.

backout: https://hg.mozilla.org/integration/autoland/rev/7f5e7b94428c62ad7d543e0651b063c59e542aba

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91b200320416
Part 4. Add safe area to nsIWidget. r=emilio
https://hg.mozilla.org/integration/autoland/rev/85c44fd76dab
Part 5. Get safe area insets from Gecko. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3ba0b7054273
Part 6. Rebuild CSS environment when safe area insets are changed. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d155d510d1a1
Part 7. Notify of safe area insets changed. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ade8355963a2
Part 8. Android implementation of safe area insets support r=snorp
https://hg.mozilla.org/integration/autoland/rev/e2fb6fc115a4
Part 9. Update API changes. r=geckoview-reviewers,agi

Comment 55

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/91b200320416
https://hg.mozilla.org/mozilla-central/rev/85c44fd76dab
https://hg.mozilla.org/mozilla-central/rev/3ba0b7054273
https://hg.mozilla.org/mozilla-central/rev/d155d510d1a1
https://hg.mozilla.org/mozilla-central/rev/ade8355963a2
https://hg.mozilla.org/mozilla-central/rev/e2fb6fc115a4

Comment 56

[Makoto Kato [:m_kato]]

done.