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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: snorp, Assigned: snorp)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(4 files)
This is part of the web manifest spec and seems pretty useful.
Assignee | ||
Updated•6 years ago
|
Blocks: progressive-apps
Comment 1•6 years ago
|
||
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
Updated•6 years ago
|
Flags: needinfo?(snorp)
Assignee | ||
Comment 2•6 years ago
|
||
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
Updated•6 years ago
|
Keywords: dev-doc-needed
Updated•6 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-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 15•6 years ago
|
||
mozreview-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 16•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•6 years ago
|
||
mozreview-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+
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b0238e34be fix JS linting error r=me
![]() |
||
Comment 24•6 years ago
|
||
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•6 years ago
|
||
So Try shows this is indeed from this patchset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06dc0a37528eddc566bf22c4db30090d2e24922c&selectedJob=123311402
Flags: needinfo?(snorp)
Assignee | ||
Comment 26•6 years ago
|
||
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)
Assignee | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 28•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
(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)
Comment 30•6 years ago
|
||
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)
Comment 31•6 years ago
|
||
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•6 years ago
|
||
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
Comment 33•6 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 34•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73aaa0359f17bd9f1e8c0e241ee1146a7d18c9f
Assignee | ||
Comment 35•6 years ago
|
||
Whoops, we figured this out earlier -- I was leaking the PresContext in nsDocShell::SetDisplayMode(). Sorry for the false alarm!
Comment 36•6 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
Comment 37•2 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•