1104916 - Implement support for CSS display-mode media feature

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marcos Caceres [:marcosc]]

Implement the display-mode media feature specified in the Web Manifest spec.

Comment 1

[Marcos Caceres [:marcosc]]

Might be able to reuse some of https://bugzilla.mozilla.org/show_bug.cgi?id=678173

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: 0001-Began-work-on-display-mode.patch

Hi David, 
I'm wondering if you can help me out? I'm having trouble getting    'PostMediaFeatureValuesChangedEvent()' in nsBaseWidget.cpp to reach "GetDisplayMode()" in nsMediaFeature.cpp 

I can see when I run the code that "Posting media feature change" is reached, but then nsMediaFeature never seems to pick it up :( 

(please note that the inclusion of <iostream> throughout is obviously just for my own testing). 

also, I can see in the browser that, at least initially, this works:

```
window.matchMedia("(display-mode:browser)").matches; //returns true

```

But then switching to fullscreen doesn't seem to work (.matches is false and cout never prints anything). 

Any guidance would be greatly appreciated!

Comment 3

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

(In reply to Marcos Caceres [:marcosc] from comment #2)
> I'm wondering if you can help me out? I'm having trouble getting   
> 'PostMediaFeatureValuesChangedEvent()' in nsBaseWidget.cpp to reach
> "GetDisplayMode()" in nsMediaFeature.cpp 
> 
> I can see when I run the code that "Posting media feature change" is
> reached, but then nsMediaFeature never seems to pick it up :( 

By fails to pick it up, do you mean that CSS style sheets don't dynamically change to reflect the feature (but matchMedia does)?

So PostMediaFeatureValuesChangedEvent should really go away; you should just call MediaFeatureValuesChanged synchronously.  (We currently use it only for size changes, and there are bugs resulting from that.)  It's possible you're hitting one of those bugs.

If not, what should happen for CSS sheets is that nsPresContext::MediaFeatureValuesChanged should lead to nsStyleSet::MediumFeaturesChanged, which should (for the nsCSSRuleProcessor for author-level sheets) return true if there's a change that needs to be processed.

What should happen for matchMedia results is that nsPresContext::MediaFeatureValuesChanged should call MediaQueryList::MediumFeaturesChanged, which should both build the notification list and change the state of the MediaQueryList.

Comment 4

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

Comment on attachment 8563894 [details] [diff] [review]
0001-Began-work-on-display-mode.patch

see above comment (I was thinking this was a needinfo?)

Comment 5

[Marcos Caceres [:marcosc]]

(In reply to David Baron [:dbaron] (UTC+13) (vacation, returning March 2) from comment #3)
> (In reply to Marcos Caceres [:marcosc] from comment #2)
> > I'm wondering if you can help me out? I'm having trouble getting   
> > 'PostMediaFeatureValuesChangedEvent()' in nsBaseWidget.cpp to reach
> > "GetDisplayMode()" in nsMediaFeature.cpp 
> > 
> > I can see when I run the code that "Posting media feature change" is
> > reached, but then nsMediaFeature never seems to pick it up :( 
> 
> By fails to pick it up, do you mean that CSS style sheets don't dynamically
> change to reflect the feature (but matchMedia does)?

Sorry for not being clear. Right now, the default "display-mode" media feature is applied to a document, which is `(display-mode: browser)` - I've verified this with both CSS and `matchMedia()`. However, Putting the browser window into fullscreen: 

1. does not cause the style to change (as per CSS below - where green is expected).
2. does not cause `matchMedia(display-mode:fullscreen)` to match or fire an event.  

In 1, the test I use to verify this is (I see yellow, where I want to see green):

```
body {
  background-color: red;
}
@media(display-mode: browser) {
  body {
    background-color: yellow;
  }
}
@media(display-mode: fullscreen) {
  body {
    background-color: green;
  }
}

```

> So PostMediaFeatureValuesChangedEvent should really go away; you should just
> call MediaFeatureValuesChanged synchronously.  (We currently use it only for
> size changes, and there are bugs resulting from that.)  It's possible you're
> hitting one of those bugs.

I've updated my code to use MediaFeatureValuesChanged synchronously, as you suggest... there I call:

```
presShell->GetPresContext()->MediaFeatureValuesChanged(nsRestyleHint(0));
```
 
> If not, what should happen for CSS sheets is that
> nsPresContext::MediaFeatureValuesChanged should lead to
> nsStyleSet::MediumFeaturesChanged, which should (for the nsCSSRuleProcessor
> for author-level sheets) return true if there's a change that needs to be
> processed.

Correct. However, I'm a bit lost here: I can see the code enter nsCSSRuleProcessor::MediumFeaturesChanged() at the appropriate time (when the window goes fullscreen), but it doesn't match any cached CSS rule. Hence, it doesn't perform the update and simply returns.
   
> What should happen for matchMedia results is that
> nsPresContext::MediaFeatureValuesChanged should call
> MediaQueryList::MediumFeaturesChanged, which should both build the
> notification list and change the state of the MediaQueryList.

As above: registering a listener does nothing:

```
var mql = matchMedia("(display-mode: fiullscreen)"); 
//never fires
mql.addListener(function(e){console.log(e)});

```

It seems I've missed something that would cause `nsMediaFeature::GetDisplayMode()` to be reached, but I can't figure out where :(

I've tried finding how orientation media feature is implemented (as, I imagine, it must be pretty much the same code as I need), but I couldn't find it. Any additional guidance would be greatly appreciated.

Comment 6

[Ted Clancy [:tedders1]]

Hey Marcos.

See this line: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#3421

It says:
>    if (groupRule->UseForPresentation(data->mPresContext, data->mCacheKey))


Could you delete that line and see if that fixes your problem? I have a theory that it's the problem.

I'd try it myself, but I need to get to bed now.

Comment 7

[Ted Clancy [:tedders1]]

Actually, nevermind. That won't be it.

I'll take another look tomorrow.

Comment 8

[Ted Clancy [:tedders1]]

Okay, I can kinda see what's happening a bit, but I don't know why.

When GetDisplayMode() (in nsMediaFeatures.cpp) queries the nsIWidget's size mode, the size mode is always 0. This is why the colour doesn't change from yellow to green in Marcos's test in Comment 5.

That happens even though nsCocoaWindow.mm calls nsBaseWidget::SetSizeMode() to set the size mode to 3.

Why? Because the nsIWidget object referenced by GetDisplayMode() is not the same nsIWidget object referenced by nsCocoaWindow.mm. The latter is of type nsCocoaWindow. The former is of type PuppetWidget.

This is the first I've heard of PuppetWidget, and I'm still investigating what's happening.

The problem seems to be specific to Mac. It works fine on Linux.

Comment 9

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

Back from vacation now.

(In reply to Marcos Caceres [:marcosc] from comment #5)
> It seems I've missed something that would cause
> `nsMediaFeature::GetDisplayMode()` to be reached, but I can't figure out
> where :(

Comment 8 suggests this isn't actually the issue, so it sounds like the problem here is related to comment 8 instead.

But let me know if there's still something I can help with.

Comment 10

[Marcos Caceres [:marcosc]]

@tedders1, got it working locally (Markus worked it out). Will upload patch tomorrow and we can go through it together.

Comment 11

[Brendan Dahl [:bdahl]]

(In reply to Marcos Caceres [:marcosc] from comment #10)
> @tedders1, got it working locally (Markus worked it out). Will upload patch
> tomorrow and we can go through it together.

I may be working on this. Do you have this updated patch?

Comment 12

[Marcos Caceres [:marcosc]]

(In reply to Brendan Dahl [:bdahl] from comment #11)
> (In reply to Marcos Caceres [:marcosc] from comment #10)
> > @tedders1, got it working locally (Markus worked it out). Will upload patch
> > tomorrow and we can go through it together.
> 
> I may be working on this. Do you have this updated patch?

Turns out what we did was kinda wrong... but will see if I can find the patch.

Comment 13

[Marcos Caceres [:marcosc]]

Attachment: displaymode.patch

Attached as far as I got... not sure if it will apply cleanly anymore, as I did it a very long time ago.

Comment 14

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

@heycam dbaron is not accepting reviews, not sure if you have time or know of someone better suited to review this.

Things are working now with the updated patch and tests. Some notes:

There was an issue with the original patch that caused the page to not repaint with the correct CSS after the page transitioned into fullscreen. There seems to be two issues. 1) During the fullscreen transition I see GetDisplayMode() called, but at that time the SizeMode of the main widget is still set to nsSizeMode_Normal. SizeMode is later updated during the EnteredFullScreen event with the correct value. 2) During SizeModeChanged() we call MediaFeatureValuesChanged(), but that was only calling NotifyMediaFeatureValuesChanged on one document, so all the sub documents never were updated.

For #1, we may just have to live with that since the SizeModeChanged happens later. We could potentially use nsGlobalWindow's FullScreen() since it is the correct value when GetDisplayMode is called, but then we'd also need to move the call to NotifyMediaFeatureValuesChanged() out of SizeModeChanged.

For #2, I took Marcos' updated patch that iterates over all the subdocuments and things seem to be working correctly now.

Comment 15

[Brendan Dahl [:bdahl]]

Comment on attachment 8706526 [details] [diff] [review]
0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

Cancelling review for now, I fixed the linux test but now the windows 7+ test are failing.

Comment 16

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

Comments above still apply. To get tests stable on linux/windows I changed around the test to assert on mozfullscreenelement instead of the sizemode. This test is pretty close to a mochitest plain, but plain's iframe doesn't allow fullscreen.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7385fe80638&selectedJob=15436604

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8708009 [details] [diff] [review]
0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

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

r=me with the following.

Generally tests named bug_NNNNNNN.html are for testing specific regressions or issues, rather than testing new features.  Probably better to name the file test_display_mode.html or similar.

Is the intention to match display-mode:fullscreen when the browser has been put into full screen with F11?  If so, can you add a test for that too?

::: layout/base/nsPresContext.cpp
@@ +2088,4 @@
>    RestyleManager()->PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
>  }
>  
> +struct MediaFeatureHints {

{ on new line.

@@ +2088,5 @@
>    RestyleManager()->PostRebuildAllStyleDataEvent(aExtraHint, aRestyleHint);
>  }
>  
> +struct MediaFeatureHints {
> +   nsRestyleHint restyleHint;

Two space indent.

@@ +2095,5 @@
> +
> +static bool
> +MediaFeatureValuesChangedAllDocumentsCallback(nsIDocument* aDocument, void* aHints)
> +{
> +  MediaFeatureHints *hints = static_cast<MediaFeatureHints*>(aHints);

* next to type.

::: layout/base/nsPresContext.h
@@ +276,4 @@
>     */
>    void MediaFeatureValuesChanged(nsRestyleHint aRestyleHint,
>                                   nsChangeHint aChangeHint = nsChangeHint(0));
> +  void MediaFeatureValuesChangedAllDocuments(nsRestyleHint aRestyleHint,

Can you please add a comment above this to say that it posts a restyle for this pres context's document and all descendant subdocuments that have a pres context, and to say what kinds of media features you would call this method for (giving display-mode as an example).

::: layout/style/nsMediaFeatures.cpp
@@ +323,5 @@
> +  nsCOMPtr<nsIWidget> mainWidget;
> +  baseWindow->GetMainWidget(getter_AddRefs(mainWidget));
> +  int32_t mode = mainWidget ? mainWidget->SizeMode() : nsSizeMode_Normal;
> +
> +  PRInt32 displayMode;

int32_t

::: layout/style/test/test_media_queries.html
@@ +258,5 @@
> +  mediatypes.forEach(function(type) {
> +    expression_should_be_parseable("display-mode: " + type);
> +  });
> +
> +  expression_should_not_be_parseable("display-mode: invalid;")

The semicolon would make us not be parseable even when the display-mode media feature isn't supported, so I think it should be removed.

Comment 18

[Brendan Dahl [:bdahl]]

I've hit a bit of snag. After the above changes I was trying out an old manual test that I had and it wasn't working anymore. Previously, I had always had lldb attached when running in and without it attached it no longer works, so there appears to be a timing issue on OSX only. Looking into that now.

Comment 19

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

The issue turned out to be with e10s and the size mode not always being properly propagated to the remote child. There were two issues: 1) Size mode was sent across every time TabParent::UpdateDimensions was called, but sometimes this is called before the size mode has actually changed. 2) nsPresContext::MediaFeatureValuesChanged would never get called in the child process so we never did a repaint.

I've changed it so now size mode is no longer part of UpdateDimensions, but is instead explicitly updated when nsWebShellWindow's size mode is updated. nsPresContext::SizeModeChanged now also takes care of calling MediaFeatureValuesChangedAllDocuments and calling SizeModeChanged on all the remote children.

Somewhat as aside but you can test that #1 is broken in e10s Nightly on OSX by going into fullscreen and seeing window.fullScreen is false in the console. It is fixed after this patch.

Comment 20

[Brendan Dahl [:bdahl]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d3471f50ba&selectedJob=16007866

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8712929 [details] [diff] [review]
0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

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

::: dom/ipc/TabChild.cpp
@@ +1633,4 @@
>  }
>  
>  bool
> +TabChild::RecvSizeModeChanged(const nsSizeMode& sizeMode)

aSizeMode

@@ +1634,5 @@
>  
>  bool
> +TabChild::RecvSizeModeChanged(const nsSizeMode& sizeMode)
> +{
> +    mPuppetWidget->SetSizeMode(sizeMode);

Use two space indent.  (I see the file is inconsistent, but let's move closer to what we want.)

::: layout/style/nsMediaFeatures.cpp
@@ +313,5 @@
>  static nsresult
> +GetDisplayMode(nsPresContext* aPresContext, const nsMediaFeature*,
> +               nsCSSValue& aResult)
> +{
> +  nsCOMPtr<nsISupports> container = aPresContext->GetRootPresContext()->Document()->GetContainer();

Keep to 80 columns.

::: widget/cocoa/nsCocoaWindow.mm
@@ +1940,4 @@
>      return;
>    }
>  
> +  nsBaseWidget::SetSizeMode(newMode);

Was this just an existing bug?

Comment 22

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

> ::: widget/cocoa/nsCocoaWindow.mm
> @@ +1940,4 @@
> >      return;
> >    }
> >  
> > +  nsBaseWidget::SetSizeMode(newMode);
> 
> Was this just an existing bug?

This was just to make things more consistent (windows and linux call this) while I was testing across platforms. I've removed the change.

Carrying the r+ forward with review comments addressed.

Comment 23

[Carsten Book [:Tomcat]]

failed to apply:

renamed 1104916 -> 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch
applying 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch
patching file dom/ipc/PBrowser.ipdl
Hunk #1 FAILED at 561
Hunk #2 FAILED at 569
2 out of 2 hunks FAILED -- saving rejects to file dom/ipc/PBrowser.ipdl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

Comment 24

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

Fixed merge from "async" being added everywhere in ipdl's.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa1296bd773

Comment 26

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

I had to back this out because it apparently turned Win Debug m(oth) tests permafail like https://treeherder.mozilla.org/logviewer.html#?job_id=21159739&repo=mozilla-inbound

This failure showed up in the try push linked in comment 20.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e36505acb6f3

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

this patch also has a win8 tpaint regression of 2%.  When it lands again we will have a perf regression, so please address this prior to landing, you can push to try with:
try: -b o -p win64 -u none -t other --rebuild 5

Comment 28

[Brendan Dahl [:bdahl]]

Attachment: 0001-Attempting-to-address-talos-tpaint-regression.patch

Fixes the tpaint regression.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=789a12291942&newProject=try&newRevision=38dee20e3ba9&framework=1&showOnlyImportant=0

Comment 29

[Brendan Dahl [:bdahl]]

Attachment: 0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

Squashed patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e3f9f005a9

Comment 30

[Brendan Dahl [:bdahl]]

Comment on attachment 8722777 [details] [diff] [review]
0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

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

Carrying r+ forward. This is the patch that needs to be checked in.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ecc9682c3a3

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/9ecc9682c3a3

Comment 33

[Chris Mills [:cmills]]

I have documented this media feature:

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

I've also added a note into the relevant Firefox release notes:

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

Let me know if this looks ok!

Comment 34

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

I think this needs to be backed out before it ships if bug 1259641 isn't fixed in time.

Comment 35

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

Comment on attachment 8722777 [details] [diff] [review]
0001-Bug-1104916-Implement-CSS-media-query-display-mode.patch

> bool
>+TabChild::RecvSizeModeChanged(const nsSizeMode& aSizeMode)
>+{
>+  mPuppetWidget->SetSizeMode(aSizeMode);
>+  nsCOMPtr<nsIDocument> document(GetDocument());
>+  nsCOMPtr<nsIPresShell> presShell = document->GetShell();
>+  if (presShell) {
>+    nsPresContext* presContext = presShell->GetPresContext();
>+    if (presContext) {
>+      presContext->MediaFeatureValuesChangedAllDocuments(eRestyle_Subtree,
>+                                                         NS_STYLE_HINT_REFLOW);
>+    }
>+  }
>+  return true;

It feels weird to me that this does something other than what the parent did (i.e., calling presContext->SizeModeChanged()).

Are we assuming that we don't need to care about the possibility of having nested process boundaries?

It also seems odd in terms of the code duplication, since it means that "what to do for a size mode change" has the same (incorrect, see bug 1259641 comment 14) code in 2 different places.

>+void
>+nsPresContext::SizeModeChanged(nsSizeMode aSizeMode)
>+{
>+  if (HasCachedStyleData()) {
>+    nsContentUtils::CallOnAllRemoteChildren(mDocument->GetWindow(),
>+                                            NotifyTabSizeModeChanged,
>+                                            &aSizeMode);
>+    MediaFeatureValuesChangedAllDocuments(eRestyle_Subtree,
>+                                          NS_STYLE_HINT_REFLOW);
>+  }
>+}

Comment 36

[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

Also, :cmills - is it worth correcting https://developer.mozilla.org/en-US/docs/Web/CSS/@media/display-mode - its claim about Firefox for Android is rather unhelpful as support here is almost entirely busted - only display-mode: browser actually works currently