Implement support for CSS display-mode media feature

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: marcosc, Assigned: bdahl)

Tracking

(Depends on 2 bugs, Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla47
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

()

Attachments

(2 attachments, 7 obsolete attachments)

Implement the display-mode media feature specified in the Web Manifest spec.
Might be able to reuse some of https://bugzilla.mozilla.org/show_bug.cgi?id=678173
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!
Attachment #8563894 - Flags: feedback?(dbaron)
(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 on attachment 8563894 [details] [diff] [review]
0001-Began-work-on-display-mode.patch

see above comment (I was thinking this was a needinfo?)
Attachment #8563894 - Flags: feedback?(dbaron)
(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.
Flags: needinfo?(dbaron)
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.
Actually, nevermind. That won't be it.

I'll take another look tomorrow.
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.
Assignee: nobody → tclancy
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.
Flags: needinfo?(dbaron)
@tedders1, got it working locally (Markus worked it out). Will upload patch tomorrow and we can go through it together.
(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?
Flags: needinfo?(mcaceres)
(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.
Flags: needinfo?(mcaceres)
Posted patch displaymode.patch (obsolete) — Splinter Review
Attached as far as I got... not sure if it will apply cleanly anymore, as I did it a very long time ago.
Attachment #8563894 - Attachment is obsolete: true
Assignee: ted.clancy → nobody
@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.
Attachment #8699320 - Attachment is obsolete: true
Attachment #8706526 - Flags: review?(cam)
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.
Attachment #8706526 - Flags: review?(cam)
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
Attachment #8706526 - Attachment is obsolete: true
Attachment #8708009 - Flags: review?(cam)
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.
Attachment #8708009 - Flags: review?(cam) → review+
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.
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.
Attachment #8708009 - Attachment is obsolete: true
Attachment #8712929 - Flags: review?(cam)
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?
Attachment #8712929 - Flags: review?(cam) → review+
> ::: 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.
Attachment #8712929 - Attachment is obsolete: true
Attachment #8715368 - Flags: review+
Keywords: checkin-needed
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
Flags: needinfo?(bdahl)
Keywords: checkin-needed
Fixed merge from "async" being added everywhere in ipdl's.
Attachment #8715368 - Attachment is obsolete: true
Flags: needinfo?(bdahl)
Attachment #8716013 - Flags: review+
Keywords: checkin-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
Flags: needinfo?(bdahl)
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
Depends on: 1133615
Attachment #8722776 - Flags: review?(cam) → review+
Flags: needinfo?(bdahl)
Keywords: checkin-needed
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.
Attachment #8722777 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9ecc9682c3a3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1255669
Assignee: nobody → bdahl
Depends on: 1256084
Depends on: 1259641
I think this needs to be backed out before it ships if bug 1259641 isn't fixed in time.
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);
>+  }
>+}
Depends on: 1323777
You need to log in before you can comment on or make changes to this bug.