1369815 - Implement 'minimal-ui' and 'standalone' matching for display-mode media queries

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

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

This is part of the web manifest spec and seems pretty useful.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In particular, this is the 'display-mode' media feature.

Bug 1104916 says we already did this.  Is that not the case?

Comment 2

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

Oh, I hadn't seen that, thanks! It looks like it only does 'fullscreen' and 'browser' right now, so we need to add support for 'standalone' and 'minimal-ui' for PWA.

Comment 3

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

Attachment: Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

Review commit: https://reviewboard.mozilla.org/r/167194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167194/

Comment 4

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

Attachment: Bug 1369815 - Add display mode to GeckoViewSettings

Review commit: https://reviewboard.mozilla.org/r/167196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167196/

Comment 5

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

Attachment: Bug 1369815 - Set the display mode for standalone PWA

Review commit: https://reviewboard.mozilla.org/r/167198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167198/

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172406

This won't work because of our lazy-load-register mechanics.

We only load a module's frame script and register its content module when the chrome module is registered (implicitly triggered by assigning a listener in GeckoView).

Since this does not fall into the usual app-provided listener pattern, it would be best to provide a dedicate module for this, with its listener set internally in GeckoView or we could move this to GeckoViewSettings.

Comment 7

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

Attachment: Bug 1369815 - Add mochitest for additional display modes

Review commit: https://reviewboard.mozilla.org/r/167248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167248/

Comment 8

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

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167196/diff/1-2/

Comment 9

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

Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167198/diff/1-2/

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

https://reviewboard.mozilla.org/r/167198/#review172496

Comment 11

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

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167196/diff/2-3/

Comment 12

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

Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167198/diff/2-3/

Comment 13

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

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167248/diff/1-2/

Comment 14

[Eugen Sawin [:esawin]]

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172514

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:38
(Diff revision 3)
> +
> +        DisplayMode(int mode) {
> +            mMode = mode;
> +        }
> +
> +        public int value() {

Do we want to expose the value publicly? If not, we don't need that method.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoViewSettings.java:58
(Diff revision 3)
> +
> +    /*
> +     * Key to specify which display-mode we should use
> +     */
> +    public static final Key<Boolean> USE_DISPLAY_MODE =
> +        new Key<Boolean>("useDisplayMode");

Should be just "displayMode", the "use"-prefix indicates a boolean setting.

Also, please set the default value in the constructor.

We should also change the order of default-initialization to assure that USE_MULTIPROCESS is set first, so that the frame scripts for the other settings are loaded.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:42
(Diff revision 3)
>  
>    onSettingsUpdate() {
>      debug("onSettingsUpdate: " + JSON.stringify(this.settings));
>  
>      this.useTrackingProtection = !!this.settings.useTrackingProtection;
> +    this.useDisplayMode = this.settings.useDisplayMode;

Same: this.displayMode.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:83
(Diff revision 3)
>      }
>      parentNode.appendChild(this.browser);
> +
> +    // Re-set the display mode, as we probably need to set it on
> +    // a different docshell now
> +    this.useDisplayMode = this.useDisplayMode;

We don't support switching e10s modes after startup, so we should not need this.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:86
(Diff revision 3)
> +    // Re-set the display mode, as we probably need to set it on
> +    // a different docshell now
> +    this.useDisplayMode = this.useDisplayMode;
> +  }
> +
> +  get useDisplayMode() {

displayMode

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:90
(Diff revision 3)
> +
> +  get useDisplayMode() {
> +    return this._displayMode;
> +  }
> +
> +  set useDisplayMode(aMode) {

displayMode

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:92
(Diff revision 3)
> +    return this._displayMode;
> +  }
> +
> +  set useDisplayMode(aMode) {
> +    if (!this.useMultiprocess) {
> +      this.window.QueryInterface(Ci.nsIInterfaceRequestor)

I assume we don't need to set this for the chrome window with e10s?

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:95
(Diff revision 3)
> +  set useDisplayMode(aMode) {
> +    if (!this.useMultiprocess) {
> +      this.window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDocShell)
> +                   .displayMode = aMode;
> +    } else {

Is it unsafe to load this frame script with e10s disabled? We might not need the check at all.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8895917 [details]
Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

https://reviewboard.mozilla.org/r/167194/#review172568

This is going in the right direction, but I'd like to see another version of the patch.

I think we should handle dynamic changes to the docshell's display mode values, just like for example nsDocShell::SetDeviceSizeIsPageSize does.  But we will need to call MediaFeatureValuesChangedAllDocuments rather than MediaFeatureValuesChanged:

http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/base/nsPresContext.h#283-289

(It might be that it's not possible for the value to change once it's set by the app manifest mechanism, but other users of nsIDocShell::SetDisplayMode might come along in the future.)

::: docshell/base/nsDocShell.h:1053
(Diff revision 1)
>    // origin attribute set.
>    uint32_t mPrivateBrowsingId;
>  
>    nsString mInterceptedDocumentId;
>  
> +  uint32_t mDisplayMode;

Please add a comment saying what this is, and what kinds of values it can take.

::: docshell/base/nsIDocShell.idl:1158
(Diff revision 1)
> +  const unsigned long DISPLAY_MODE_BROWSER = 1;
> +  const unsigned long DISPLAY_MODE_STANDALONE = 2;
> +  const unsigned long DISPLAY_MODE_FULLSCREEN = 3;
> +  const unsigned long DISPLAY_MODE_MINIMAL_UI = 4;

I think we should use the same constant values that the display-mode media feature uses to represent these values, unless there is a good reason not to:

http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/layout/style/nsStyleConsts.h#1266-1270

::: layout/style/nsMediaFeatures.cpp:310
(Diff revision 1)
> +  if (!aPresContext) {
> +    return;
> +  }

Is there a specific reason to return without setting a value?  Previously, we would set NS_STYLE_DISPLAY_MODE_BROWSER if we didn't have a pres context.  Whether that's an observable change, I'm not sure.  Maybe calling window.matchMedia() on a display:none iframe's window would stop matching `(display-mode: browser)` in that case?

I think we should leave the old behaviour.

::: layout/style/nsMediaFeatures.cpp:334
(Diff revision 1)
> +  }
> +
> +  int32_t displayMode = NS_STYLE_DISPLAY_MODE_BROWSER;

Then add a static_assert in here that the values of the nsIDocShell constants and the NS_STYLE_DISPLAY_MODE_* ones match up, and drop the switch statement and just assign it.

Also, please add a comment to nsStyleConsts.h where the NS_STYLE_DISPLAY_MODE_* constants are defined to point to GeckoViewSettings.java where the values have to match up and we have no way of asserting this.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

https://reviewboard.mozilla.org/r/167248/#review172570

::: layout/style/test/chrome/test_display_mode.html:87
(Diff revision 2)
> +  // Test entering display mode mode through docshell
> +  setDisplayMode(Components.interfaces.nsIDocShell.DISPLAY_MODE_STANDALONE);
> +  shouldApply("all and (display-mode: standalone)");
> +  shouldNotApply("all and (display-mode: fullscreen)");
> +  shouldNotApply("all and (display-mode: browser)");
> +  shouldNotApply("all and (display-mode: minimal-ui)");

I'm a little surprised this passes without informing the pres context that the docshell's display mode value changed.  Can you additionally test some computed value based on an @media rule in the document, e.g.

  @media (display-mode: standalone) {
    #display { color: blue; }
  }

then check that it's blue after the display mode is updated?

Comment 17

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

Comment on attachment 8895917 [details]
Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167194/diff/1-2/

Comment 18

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

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167196/diff/3-4/

Comment 19

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

Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167198/diff/3-4/

Comment 20

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

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167248/diff/2-3/

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8895917 [details]
Bug 1369815 - Add display mode to nsIDocShell and use it for media queries

https://reviewboard.mozilla.org/r/167194/#review173248

Looks good, thanks!

::: docshell/base/nsIDocShell.idl:1169
(Diff revision 2)
> +  const unsigned long DISPLAY_MODE_STANDALONE = 2;
> +  const unsigned long DISPLAY_MODE_FULLSCREEN = 3;
> +
> +  /**
> +   * Display mode for this docshell. Defaults to DISPLAY_MODE_BROWSER.
> +   * Media queries only look at the value in the parent docshell.

Parent or top-most?

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b956ceaaf8b
Add display mode to nsIDocShell and use it for media queries r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25a5921d961
Add display mode to GeckoViewSettings r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a758c86dd864
Set the display mode for standalone PWA r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e018aed68975
Add mochitest for additional display modes r=heycam

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b0238e34be
fix JS linting error r=me

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1369817, bug 1369815, bug 1389269 and bug 1126479 for crashing in mochitest-chrome-3's layout/style/test/chrome/test_stylesheet_clone_import_rule.html on debug:

bug 1369817
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af3a98934c427226e095246a098008bc3474ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd81631bfad32fdc2fa46a8937d189caa21cb3d

bug 1369815
https://hg.mozilla.org/integration/mozilla-inbound/rev/7780a4af90284e7f812d44ec32adb68dcbcc94b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/76f15802d69c2835eaa13e92df4bee34637f3b6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4871dc084c88feecdbabe1b3a9acdb5ffaa71a2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbb1bd8335d0c49ac477e60e85a03029c8c6a17
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc485e4ba834888fb7f5bbb97f9a91c33c1a485

bug 1389269
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e49684354331b02d3e425d2afcd757559bd7db

bug 1126479
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec2a9eac2d9c7ec52543e5b77a6ee38c3f6879f
https://hg.mozilla.org/integration/mozilla-inbound/rev/337a6416a05bc3a9fac980eaa54084b78cd639b6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4fb119706335b308f049949e5641565b035e4f80&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123274657&repo=mozilla-inbound

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

So Try shows this is indeed from this patchset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06dc0a37528eddc566bf22c4db30090d2e24922c&selectedJob=123311402

Comment 26

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

Cameron, the mochitest appears to cause a shutdown leak, and I don't understand why. It looks like simply setting the displayMode on the docshell causes this. Do you have any idea what could be going on?

Comment 27

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

Comment on attachment 8895918 [details]
Bug 1369815 - Add display mode to GeckoViewSettings

https://reviewboard.mozilla.org/r/167196/#review172514

> Do we want to expose the value publicly? If not, we don't need that method.

We do, because we need the integer for the setInteger() method.

> I assume we don't need to set this for the chrome window with e10s?

Correct, for e10s we only need to set the parent docshell in the content process.

> Is it unsafe to load this frame script with e10s disabled? We might not need the check at all.

It's safe, but it sets the value on the wrong docshell.

Comment 28

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

Comment on attachment 8895976 [details]
Bug 1369815 - Add mochitest for additional display modes

https://reviewboard.mozilla.org/r/167248/#review172570

> I'm a little surprised this passes without informing the pres context that the docshell's display mode value changed.  Can you additionally test some computed value based on an @media rule in the document, e.g.
> 
>   @media (display-mode: standalone) {
>     #display { color: blue; }
>   }
> 
> then check that it's blue after the display mode is updated?

I'm calling nsPresContext::MediaFeatureValuesChangedAllDocuments() when the display mode changes, isn't that enough? I can add some more rules.

Comment 29

[Cameron McCormack (:heycam)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #28)
> I'm calling nsPresContext::MediaFeatureValuesChangedAllDocuments() when the
> display mode changes, isn't that enough?

Yes, that should be enough.  But I think you weren't calling it in the patch at the time I looked at the test, so I was surprised it would be passing without it.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #26)
> Cameron, the mochitest appears to cause a shutdown leak, and I don't
> understand why. It looks like simply setting the displayMode on the docshell
> causes this. Do you have any idea what could be going on?

I don't, unfortunately.  Xidorn, do you have any idea what might be causing a leak with this @import-related test?  (Not stylo.)

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Hmm, this seems to be the same issue as bug 1387490 actually. There is nothing to do about the @import test. It crashes simply because it happens to be the last test before shutdown. That says, bug 1387490 affects not only stylo, but also non-stylo, as shown in this case.

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

FWIW, I didn't manage to run this test locally... It seems that the iframe page doesn't load for me. I have no idea why.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7d96d920d6
Add display mode to nsIDocShell and use it for media queries r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce3ad26c529
Add display mode to GeckoViewSettings r=esawin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8428fa4fe1a
Set the display mode for standalone PWA r=droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb774244a1b
Add mochitest for additional display modes r=heycam

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a7d96d920d6
https://hg.mozilla.org/mozilla-central/rev/6ce3ad26c529
https://hg.mozilla.org/mozilla-central/rev/e8428fa4fe1a
https://hg.mozilla.org/mozilla-central/rev/5bb774244a1b

Comment 34

[Jonathan Kew [:jfkthame]]

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

Comment 35

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

Whoops, we figured this out earlier -- I was leaking the PresContext in nsDocShell::SetDisplayMode(). Sorry for the false alarm!

Comment 36

[Chris Mills [:cmills]]

I've added a note to the Fx57 rel notes to cover this:

https://developer.mozilla.org/en-US/Firefox/Releases/57#CSS

I've also made the browser support situation clear on

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/display-mode
https://developer.mozilla.org/en-US/docs/Web/CSS/@media
https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries
https://developer.mozilla.org/en-US/docs/Web/Manifest#display

Let me know if that looks OK. Thanks!

Comment 37

[Luke Plant]

For the sake of anyone else tracking down why this doesn't actually work on current Firefox for Android (version 88), although it did once work, see this bug: https://github.com/mozilla-mobile/android-components/issues/8584