Closed
Bug 1104916
Opened 10 years ago
Closed 9 years ago
Implement support for CSS display-mode media feature
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: marcosc, Assigned: bdahl)
References
(Depends on 2 open bugs, Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 7 obsolete files)
1.17 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
23.13 KB,
patch
|
bdahl
:
review+
|
Details | Diff | Splinter Review |
Implement the display-mode media feature specified in the Web Manifest spec.
Reporter | ||
Updated•10 years ago
|
Blocks: webmanifest
Reporter | ||
Comment 1•10 years ago
|
||
Might be able to reuse some of https://bugzilla.mozilla.org/show_bug.cgi?id=678173
Reporter | ||
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
(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)
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 6•10 years ago
|
||
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•10 years ago
|
||
Actually, nevermind. That won't be it. I'll take another look tomorrow.
Comment 8•10 years ago
|
||
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.
Updated•10 years ago
|
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)
Reporter | ||
Comment 10•10 years ago
|
||
@tedders1, got it working locally (Markus worked it out). Will upload patch tomorrow and we can go through it together.
Assignee | ||
Comment 11•9 years ago
|
||
(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)
Reporter | ||
Comment 12•9 years ago
|
||
(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)
Reporter | ||
Comment 13•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Assignee: ted.clancy → nobody
Assignee | ||
Comment 14•9 years ago
|
||
@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)
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Comment 19•9 years ago
|
||
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)
Assignee | ||
Comment 20•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d3471f50ba&selectedJob=16007866
Comment 21•9 years ago
|
||
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+
Assignee | ||
Comment 22•9 years ago
|
||
> ::: 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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
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
Assignee | ||
Comment 24•9 years ago
|
||
Fixed merge from "async" being added everywhere in ipdl's.
Attachment #8715368 -
Attachment is obsolete: true
Flags: needinfo?(bdahl)
Attachment #8716013 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa1296bd773
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)
Comment 27•9 years ago
|
||
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
Assignee | ||
Comment 28•9 years ago
|
||
Fixes the tpaint regression. https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=789a12291942&newProject=try&newRevision=38dee20e3ba9&framework=1&showOnlyImportant=0
Attachment #8722776 -
Flags: review?(cam)
Assignee | ||
Comment 29•9 years ago
|
||
Squashed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e3f9f005a9
Attachment #8716013 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8722776 -
Flags: review?(cam) → review+
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bdahl)
Keywords: checkin-needed
Assignee | ||
Comment 30•9 years ago
|
||
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+
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ecc9682c3a3
Keywords: checkin-needed
Comment 32•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ecc9682c3a3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 33•9 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
Updated•9 years ago
|
Assignee: nobody → bdahl
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: 1266660
Comment 36•3 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
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
You need to log in
before you can comment on or make changes to this bug.
Description
•