Closed Bug 1369815 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Bug 1369815 - Add display mode to nsIDocShell and use it for media queries
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1369815 - Add display mode to GeckoViewSettings
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1369815 - Set the display mode for standalone PWA
59 bytes, text/x-review-board-request
droeh
: review+
Details
Bug 1369815 - Add mochitest for additional display modes
59 bytes, text/x-review-board-request
heycam
: review+
Details 
This is part of the web manifest spec and seems pretty useful.
In particular, this is the 'display-mode' media feature.

Bug 1104916 says we already did this.  Is that not the case?
Summary: Implement PWA media queries → Implement 'display-mode' media feature for media queries
Flags: needinfo?(snorp)
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.
Flags: needinfo?(snorp)
Summary: Implement 'display-mode' media feature for media queries → Implement 'minimal-ui' and 'standalone' matching for display-mode media queries
Priority: -- → P3
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.
Attachment #8895918 - Flags: review?(esawin) → review-
Comment on attachment 8895919 [details]
Bug 1369815 - Set the display mode for standalone PWA

https://reviewboard.mozilla.org/r/167198/#review172496
Attachment #8895919 - Flags: review?(droeh) → review+
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.
Attachment #8895918 - Flags: review?(esawin) → review+
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.
Attachment #8895917 - Flags: review?(cam) → review-
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?
Attachment #8895976 - Flags: review?(cam) → review+
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?
Attachment #8895917 - Flags: review?(cam) → review+
Pushed by jwillcox@mozilla.com:
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
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
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?
Assignee: nobody → snorp
Flags: needinfo?(snorp) → needinfo?(cam)
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 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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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) (jwillcox@mozilla.com) 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.)
Flags: needinfo?(cam) → needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
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.
Pushed by jwillcox@mozilla.com:
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
Whoops, we figured this out earlier -- I was leaking the PresContext in nsDocShell::SetDisplayMode(). Sorry for the false alarm!

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

Blocks: 1819494
You need to log in before you can comment on or make changes to this bug.