Closed Bug 1503656 Opened 6 years ago Closed 5 years ago

Android cutout support for CSS env() safe area insets

Categories

(GeckoView :: General, enhancement, P2)

Product:
GeckoView
Component:
General
Platform:
Unspecified
Android
Type:
enhancement

Tracking

(firefox70 wontfix, firefox71 wontfix, firefox72 wontfix, firefox75 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox75 --- fixed

People

(Reporter: cpeterson, Assigned: m_kato)

References

Details

(Whiteboard: [geckoview:m1910] [geckoview:m1911] [geckoview:m1912][geckoview:m74])

Attachments

(10 files, 2 obsolete files)

WIP
(deleted), patch
Details | Diff | Splinter Review
Bug 1503656 - Part 3. Don't use hardcoded value for safearea.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 1. Always use CssEnvironment from media query's device.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 2. Use Device for parameter instead of CssEnvironment.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 4. Add safe area to nsIWidget.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 5. Get safe area insets from Gecko.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 6. Rebuild CSS environment when safe area insets are changed. r?emilio
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 8. Android implementation of safe area insets support
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 7. Notify of safe area insets changed.
(deleted), text/x-phabricator-request
Details
Bug 1503656 - Part 9. Update API changes.
(deleted), text/x-phabricator-request
Details
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.
Summary: Android cutout support for CSS env() variables → Android cutout support for CSS env() safe area insets
Makoto, are you actively working on Android P support now? Can you include this bug as part of the Android P work?
Blocks: android-p
Flags: needinfo?(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).
Flags: needinfo?(m_kato)
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.
No longer blocks: android-p
Depends on: android-p
No longer depends on: build-android-p
Priority: -- → P3
Whiteboard: [geckoview] → [geckoview:p3]

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

(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.

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

See Also: → https://github.com/mozilla-mobile/fenix/issues/4149
Depends on: 1574307

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

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

Priority: P3 → P2
Whiteboard: [geckoview:p3] → [geckoview:fenix:m9]

Linking to GeckoView's PWA meta bug 1561593

Blocks: 1561593
Rank: 10
Component: Widget: Android → General
Product: Core → GeckoView
Whiteboard: [geckoview:fenix:m9]

Tentatively scheduling for GV's October sprint.

Whiteboard: [geckoview:m1910]

We want to work on this in October.

status-firefox70: --- → wontfix
status-firefox71: --- → affected
Priority: P2 → P1
Assignee: nobody → m_kato

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

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.

(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

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?

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?)

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)
Attached patch WIP (deleted) — — Splinter Review

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...

Flags: needinfo?(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?

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?

Flags: needinfo?(emilio)

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.

(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.

Rolling over to November sprint.

status-firefox71: affected → wontfix
status-firefox72: --- → affected
Whiteboard: [geckoview:m1910] → [geckoview:m1910] [geckoview:m1911]
Depends on: 1594980

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

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

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

Depends on D52506

Safe area value is stored in nsIWidget for stylo.

Depends on D52507

Add binding to get safe area from Gecko.

Depends on D52508

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

Depends on D52509

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

Depends on D52510

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

(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.

(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.

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.

Attachment #9107791 - Attachment is obsolete: true
Attachment #9107783 - Attachment description: Bug 1503656 - Part 2. Always use CssEnvironment from media query's device. → Bug 1503656 - Part 1. Always use CssEnvironment from media query's device.
Attachment #9107784 - Attachment description: Bug 1503656 - Part 3. Use Device for parameter instead of CssEnvironment. → Bug 1503656 - Part 2. Use Device for parameter instead of CssEnvironment.
Attachment #9107781 - Attachment description: Bug 1503656 - Part 1. Split CssEnvironment implementation to Gecko and Servo. → Bug 1503656 - Part 3. Don't use hardcoded value for safearea.
Attachment #9107786 - Attachment description: Bug 1503656 - Part 5. Get safe area from Gecko. → Bug 1503656 - Part 5. Get safe area insets from Gecko.
Attachment #9107787 - Attachment description: Bug 1503656 - Part 6. Notify of safe area changed. → Bug 1503656 - Part 6. Notify of safe area insets changed.
Attachment #9107789 - Attachment description: Bug 1503656 - Part 7. Rebuild CSS environment when safe area is changed. → Bug 1503656 - Part 7. Rebuild CSS environment when safe area insets are changed.
Attachment #9107790 - Attachment description: Bug 1503656 - Part 8. Android implementation of safe area support → Bug 1503656 - Part 8. Android implementation of safe area insets support
Attachment #9107785 - Attachment description: Bug 1503656 - Part 4. Add safe area to nsIWidget. → Bug 1503656 - Part 4. Add safe area insets to nsIWidget.

(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.

Attachment #9107785 - Attachment description: Bug 1503656 - Part 4. Add safe area insets to nsIWidget. → Bug 1503656 - Part 4. Add safe area to nsIWidget.
Attachment #9107787 - Attachment description: Bug 1503656 - Part 6. Notify of safe area insets changed. → Bug 1503656 - Part 7. Notify of safe area insets changed.
Attachment #9107789 - Attachment description: Bug 1503656 - Part 7. Rebuild CSS environment when safe area insets are changed. → Bug 1503656 - Part 6. Rebuild CSS environment when safe area insets are changed.
Attachment #9107789 - Attachment description: Bug 1503656 - Part 6. Rebuild CSS environment when safe area insets are changed. → Bug 1503656 - Part 6. Rebuild CSS environment when safe area insets are changed. r?emilio

Depends on D52512

Attachment #9107787 - Attachment is obsolete: true
Whiteboard: [geckoview:m1910] [geckoview:m1911] → [geckoview:m1910] [geckoview:m1911] [geckoview:m1912]

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?

Flags: needinfo?(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.

Flags: needinfo?(m_kato) → needinfo?(emilio)

Sure, that works for me :-)

Flags: needinfo?(emilio)
Keywords: leave-open
Whiteboard: [geckoview:m1910] [geckoview:m1911] [geckoview:m1912] → [geckoview:m1910] [geckoview:m1911] [geckoview:m1912][geckoview:m74]
Pushed by m_kato@ga2.so-net.ne.jp: 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

Has this issue been fixed?

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

Flags: needinfo?(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.

Flags: needinfo?(m_kato) → needinfo?(emilio)

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

Flags: needinfo?(emilio)
Depends on: 1612785

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.

(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.

  1. 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.

(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.

Priority: P1 → P2

Depends on D52513

Pushed by m_kato@ga2.so-net.ne.jp: 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
Pushed by m_kato@ga2.so-net.ne.jp: 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

done.

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox75: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Regressions: 1621241
Regressions: 1621543
Blocks: 1432090
No longer depends on: 1432090
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: