Closed Bug 1382078 Opened 8 years ago Closed 7 years ago

stylo: XBL style doesn't respond to media changes

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: xidorn, Assigned: TYLin)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1382078 Part 1 - Add method to nsBindingManager to iterate all bound contents.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382078 Part 2 - Make nsBindingManager::MediumFeaturesChanged() return bool directly.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382078 Part 3 - Support media changes for XBL stylesheets.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1382078 Part 4 - Avoid unneeded restyle when XBL styleset is utilized by different PresContext.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Servo PR #18367
41 bytes, text/x-github-pull-request
Details | Review
In test layout/style/test/test_media_queries_dynamic_xbl.html, it checks that changing the viewport size which changes the media query result would lead to a computed style change, but that checks fails.
This is XBL stylesheet related. TYLin, could you have a look at this?
Flags: needinfo?(tlin)
I suspect this can actually be the same thing with bug 1375383.
Depends on: 1375383
Priority: -- → P3
Can you try doing a print preview on a document with a <marquee> to see what kind of effect it has? It might be that it just doesn't have a good, static position for the animated text. But I'm not sure. Would be good to know if the result is acceptable. The resolution one is something that would be observable, if you drag the window from a high DPI monitor to a non-high DPI one. But since the effect would just be to not update to use the correct resolution image, it might be acceptable. If you have an external monitor, can you try to see what it looks like? Handling updates to the Windows theme seems lower priority.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #3) > These are the @media rules in XBL stylesheet I'm aware of in content, which > might not need dynamic support of media query changes. We could probably > implement this post 57. > > http://searchfox.org/mozilla-central/rev/ > cd82cacec2cf734768827ff85ba2dba90a534c5e/toolkit/themes/osx/global/resizer. > css#17-22 It seems to me that we may want to handle this one. Screen dpi can change dynamically when user changes display setting or move a window from one monitor to another, which isn't that rare. But it is just a resizer, so maybe not a big deal either. > http://searchfox.org/mozilla-central/source/toolkit/themes/shared/media/ > videocontrols.css#472-477 I suppose this can change dynamically as well... but probably not very usual.
I've tried to do a print preview on Linux for the example on <marquee> MDN page. For stylo, the animated text have good static position in the preview, and it looks same to me when renedered by gecko. I've also tried to drag the resizer on <textarea> from a high DPI monitor to a low DPI monitor. Because the resizer is small, I cannot see the visual difference. If I zoom the web page in to 300%, the reszier just blurs, so I feel the resolution of the resizer image might not make much difference.
How does this work in Gecko exactly? What ends up invalidating the XBL resource stylesheets? I suppose it's mostly when MediumFeaturesChanged is called? Should we keep track on a StyleSet of all the other StyleSet's that are bound to it? Seems like this is implemented in Gecko via nsBindingManager::MediumFeaturesChanged, implementing something like that shouldn't be too hard. I may try if I don't run out of time today.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7) > Seems like this is implemented in Gecko via > nsBindingManager::MediumFeaturesChanged, implementing something like that > shouldn't be too hard. I think implementing nsBindingManager::MediumFeaturesChanged for XBL styleset should be sufficient. Once tricky thing for XBL styleset is that it could be shared by multiple PresContexts, i.e., XBL ServoStyleSet and PresContext is not 1:1 mapping, and the PresContext might be dead after the XBL styleset is computed in nsXBLPrototypeResources::ComputeServoStyleSet [1] (Bug 1388165 fixed it). I guess gecko's nsCSSRuleProcessor deals with above issue is at [2]. I copy that idea to ServoStyleSet in my Part 3. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/xbl/nsXBLPrototypeResources.cpp#168 [2] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/nsCSSRuleProcessor.cpp#3821-3830
Comment on attachment 8902562 [details] Bug 1382078 Part 1 - Add method to nsBindingManager to iterate all bound contents. https://reviewboard.mozilla.org/r/174138/#review179414 ::: dom/xbl/nsBindingManager.h:199 (Diff revision 1) > static void PostPAQEventCallback(nsITimer* aTimer, void* aClosure); > > + // Enumerate each bound content's bindings (including its base bindings) > + // in mBoundContentSet. > + template<typename Func> > + void EnumerateBoundContentBindings(Func aCallback) const { Maybe this could just be private and in the cpp file so it's easier to touch? No big deal.
Attachment #8902562 - Flags: review?(emilio) → review+
Comment on attachment 8902563 [details] Bug 1382078 Part 2 - Make nsBindingManager::MediumFeaturesChanged() return bool directly. https://reviewboard.mozilla.org/r/174140/#review179418
Attachment #8902563 - Flags: review?(emilio) → review+
Comment on attachment 8902564 [details] Bug 1382078 Part 3 - Support media changes for XBL stylesheets. https://reviewboard.mozilla.org/r/174168/#review179420 I'm very confused about the `mLastPresContext` stuff, and would want to understand when that happens, and how does gecko deal with it. Looks to me that this is wrong if it can be used from multiple prescontexts at the same time, but that's maybe (probably?) not an issue. We don't need to check for viewport units, and I think there's something very fishy in the `ComputeServoStyleSet` call. Making the update asynchronously would be _really_ nice, but not 100% required I guess. I think it may actually not be too terrible, it of course depends on the pres context stuff. ::: dom/xbl/nsBindingManager.cpp:746 (Diff revision 1) > + } else { > + // PresContext is not changed. This means aPresContext is still > + // alive since the last time it initialized this XBL styleset. > + // It's safe to check whether medium features changed. > + needToRecompute = > + styleSet->MediumFeaturesChanged(aViewportChanged) != nsRestyleHint(0); So this doesn't really need to check `aViewportChanged` at all, since we'll flag the main stylist only, and it'd handle it correctly. So I think we want a: ``` bool MediumFeaturesChangedRules(bool* aViewportUnitsUsed); ``` That basically does the style-checking part of the current `MediumFeaturesChanged`: ``` const OriginFlags rulesChanged = static_cast<OriginFlags>( Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), aViewportUnitsUsed)); if (rulesChanged != OriginFlags(0)) { MarkOriginsDirty(rulesChanged); return true; } return false; ``` WDYT? ::: dom/xbl/nsBindingManager.cpp:751 (Diff revision 1) > + styleSet->MediumFeaturesChanged(aViewportChanged) != nsRestyleHint(0); > + } > + > + if (needToRecompute) { > + // Recompute servo style set by using new PresContext. > + protoBinding->ComputeServoStyleSet(aPresContext); It feels kind of weird to do this synchronously here, but asynchronously (with `MarkOriginsDirty`) everywhere else. It depends on whether we can call `UpdateStylistIfNeeded`, and how do we deal with the multiple pres context stuff, but we may want to make `StylistState` a few bit flags, like: ``` enum class StylistState : uint8_t { NotDirty = 0, StyleSheetsDirty = 1 << 0, XBLStyleSheetsDirty = 1 << 1, }; ``` And whe the XBL flag is present, go through all the XBL stylists calling `UpdateStylistIfNeeded`. ::: dom/xbl/nsXBLPrototypeResources.cpp:173 (Diff revision 1) > nsXBLPrototypeResources::ComputeServoStyleSet(nsPresContext* aPresContext) > { > + if (!mServoStyleSet) { > - mServoStyleSet.reset(new ServoStyleSet()); > + mServoStyleSet.reset(new ServoStyleSet()); > + } > mServoStyleSet->Init(aPresContext, nullptr); I'd find very weird to call `Init` twice on the same StyleSet, seems like a footgun. Why can't we just call `UpdateStylistIfNeeded`? I guess the problem if the pres context actually changes is the servo-side reference we keep to it? How can the pres context change? Also, why not just calling `reset()` avobe anyway? Looks to me that this isn't clearing the stylesheet list in `ServoStyleSheet`, and thus each call here with a non-null `mServoStyleSet` will actually grow the stylesheet list we use, is that right? ::: layout/style/ServoStyleSet.h:568 (Diff revision 1) > + nsPresContext* MOZ_NON_OWNING_REF mPresContext = nullptr; > + > + // Because XBL style set could be used by multiple PresContext, we need to > + // store the last PresContext which initialize this style set for > + // computing medium rule changes. > + nsPresContext* MOZ_NON_OWNING_REF mLastPresContext = nullptr; Hmm... So this looks a bit hacky. I get what this is trying to solve, but I'm not sure I love it. How does Gecko deal with this? (Does it even try to deal with it?) ::: layout/style/ServoStyleSet.cpp:180 (Diff revision 1) > if (rulesChanged != OriginFlags(0)) { > MarkOriginsDirty(rulesChanged); > return eRestyle_Subtree; > } > > + if (mBindingManager && mPresContext) { With the above change, there's no need to check `mPresContext` here.
Attachment #8902564 - Flags: review?(emilio)
Comment on attachment 8902562 [details] Bug 1382078 Part 1 - Add method to nsBindingManager to iterate all bound contents. https://reviewboard.mozilla.org/r/174138/#review179414 > Maybe this could just be private and in the cpp file so it's easier to touch? No big deal. On second thought, aCallback doesn't need to be a template parameter. I'll make it a `std::function`, and move the definition to cpp.
Comment on attachment 8902564 [details] Bug 1382078 Part 3 - Support media changes for XBL stylesheets. https://reviewboard.mozilla.org/r/174168/#review179420 When `nsXBLPrototypeResources` constructs a gecko `nsCSSRuleProcessor` in GatherRuleProcessor() [1] , it doesn't need a `PresContext`, but when it creates `ServoStyleSet` in `ComputeServoStyleSet()`, `PresContext` is needed by both `SerovStyleSet` and `Stylist`. `nsXBLPrototypeResources` is loaded only once, and is shared by all the documents. The `PresContext` referenced by servo-side `Stylist` is the one which loads the resources, so we cannot just call `UpdateStylistIfNeeded` when media changes because the `PresContext` is just not the correct one. I believe gecko handles multiple PresContext by using a cache in `nsCSSRuleProcessor::GetRuleCascade` in [3], which is where my `mLastPresContext` idea came from. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/xbl/nsXBLPrototypeResources.cpp#161-164 [2] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/xbl/nsXBLPrototypeResources.cpp#171 [3] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/nsCSSRuleProcessor.cpp#3821-3830 > So this doesn't really need to check `aViewportChanged` at all, since we'll flag the main stylist only, and it'd handle it correctly. > > So I think we want a: > > ``` > bool MediumFeaturesChangedRules(bool* aViewportUnitsUsed); > ``` > > That basically does the style-checking part of the current `MediumFeaturesChanged`: > > ``` > const OriginFlags rulesChanged = static_cast<OriginFlags>( > Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), aViewportUnitsUsed)); > > if (rulesChanged != OriginFlags(0)) { > MarkOriginsDirty(rulesChanged); > return true; > } > > return false; > ``` > > WDYT? This sounds good to me. I'll update my patch accordingly. > It feels kind of weird to do this synchronously here, but asynchronously (with `MarkOriginsDirty`) everywhere else. > > It depends on whether we can call `UpdateStylistIfNeeded`, and how do we deal with the multiple pres context stuff, but we may want to make `StylistState` a few bit flags, like: > > ``` > enum class StylistState : uint8_t { > NotDirty = 0, > StyleSheetsDirty = 1 << 0, > XBLStyleSheetsDirty = 1 << 1, > }; > ``` > > And whe the XBL flag is present, go through all the XBL stylists calling `UpdateStylistIfNeeded`. OK. I'll update my patch to update XBL stylesheet asynchronously. > I'd find very weird to call `Init` twice on the same StyleSet, seems like a footgun. > > Why can't we just call `UpdateStylistIfNeeded`? I guess the problem if the pres context actually changes is the servo-side reference we keep to it? > > How can the pres context change? Also, why not just calling `reset()` avobe anyway? > > Looks to me that this isn't clearing the stylesheet list in `ServoStyleSheet`, and thus each call here with a non-null `mServoStyleSet` will actually grow the stylesheet list we use, is that right? You're right. We should just `reset()` the `ServoStyleSet`. > Hmm... So this looks a bit hacky. I get what this is trying to solve, but I'm not sure I love it. > > How does Gecko deal with this? (Does it even try to deal with it?) See my explanation above.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment on attachment 8902564 [details] Bug 1382078 Part 3 - Support media changes for XBL stylesheets. https://reviewboard.mozilla.org/r/174168/#review179894 Looks pretty good! Just some minor details that need fixing. ::: dom/xbl/nsBindingManager.cpp:767 (Diff revision 3) > + // PresContext is not changed. This means aPresContext is still > + // alive since the last time it initialized this XBL styleset. > + // It's safe to check whether medium features changed. > + styleSetChanged = > + styleSet->MediumFeaturesChangedRules(&viewportUnitsUsed); > + *aViewportUnitsUsed = *aViewportUnitsUsed || viewportUnitsUsed; No need for `aViewportUnitsUsed`, you can probably `MOZ_ASSERT(!viewportUnitsUsed, "Non main stylesets shouldn't get flagged as using viewport units")`. ::: layout/style/ServoStyleSet.h:580 (Diff revision 3) > + nsPresContext* MOZ_NON_OWNING_REF mPresContext = nullptr; > + > + // Because XBL style set could be used by multiple PresContext, we need to > + // store the last PresContext which initialize this style set for > + // computing medium rule changes. > + nsPresContext* MOZ_NON_OWNING_REF mLastPresContext = nullptr; Maybe worth storing as a `uintptr_t` so it can't be misused? Also, maybe adding a more XBL-specific name is worth? ::: layout/style/ServoStyleSet.cpp:172 (Diff revision 3) > > nsRestyleHint > ServoStyleSet::MediumFeaturesChanged(bool aViewportChanged) > { > bool viewportUnitsUsed = false; > const OriginFlags rulesChanged = static_cast<OriginFlags>( This can use `MediumFeaturesChangedRules`. ::: layout/style/ServoStyleSet.cpp:177 (Diff revision 3) > const OriginFlags rulesChanged = static_cast<OriginFlags>( > Servo_StyleSet_MediumFeaturesChanged(mRawSet.get(), &viewportUnitsUsed)); > > if (rulesChanged != OriginFlags(0)) { > MarkOriginsDirty(rulesChanged); > return eRestyle_Subtree; Also, this can't early-return, otherwise you miss updating XBL sheets if the main doc sheets actually change. Not sure how easy it is to extend the test to test this. ::: layout/style/ServoStyleSet.cpp:183 (Diff revision 3) > } > > + if (mBindingManager) { > + // We are main style set. Call binding manager to check whether medium > + // features changed for content bindings. > + bool bindingViewportUnitsUsed = false; there's no need to pass `viewportUnitsUsed` to the binding manager, it will always be false, let's remove that argument and simplify the code. ::: layout/style/ServoStyleSet.cpp:1455 (Diff revision 3) > MOZ_ASSERT(StylistNeedsUpdate()); > + > + if (mStylistState & StylistState::StyleSheetsDirty) { > - Element* root = mPresContext->Document()->GetDocumentElement(); > + Element* root = mPresContext->Document()->GetDocumentElement(); > - Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + nit: Extra whitespace. ::: layout/style/ServoStyleSet.cpp:1457 (Diff revision 3) > + if (mStylistState & StylistState::StyleSheetsDirty) { > - Element* root = mPresContext->Document()->GetDocumentElement(); > + Element* root = mPresContext->Document()->GetDocumentElement(); > - Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + > + } > + if (mStylistState & StylistState::XBLStyleSheetsDirty) { nit: Maybe worth to use `UNLIKELY` here. ::: layout/style/nsStyleSet.cpp:2687 (Diff revision 3) > bool thisChanged = processor->MediumFeaturesChanged(presContext); > stylesChanged = stylesChanged || thisChanged; > } > > if (mBindingManager) { > + bool dummy;
Attachment #8902564 - Flags: review?(emilio)
Comment on attachment 8902564 [details] Bug 1382078 Part 3 - Support media changes for XBL stylesheets. https://reviewboard.mozilla.org/r/174168/#review179894 > Maybe worth storing as a `uintptr_t` so it can't be misused? Also, maybe adding a more XBL-specific name is worth? After this and bug 1384232 land, I'd like do a simple follow up that replaces `mPresContext` with `PresContext()` which asserts if it's an XBL style set. BTW, `uintptr_t` needs cast when comparing with `mPresContex`, I feel using `void*` should prevent misuse, too.
Comment on attachment 8902564 [details] Bug 1382078 Part 3 - Support media changes for XBL stylesheets. https://reviewboard.mozilla.org/r/174168/#review180458 Perfect, thank you! ::: layout/style/ServoStyleSet.cpp:172 (Diff revision 4) > > nsRestyleHint > ServoStyleSet::MediumFeaturesChanged(bool aViewportChanged) > { > bool viewportUnitsUsed = false; > - const OriginFlags rulesChanged = static_cast<OriginFlags>( > + bool stylechanged = MediumFeaturesChangedRules(&viewportUnitsUsed); nit: I'd keep this as `rulesChanged`, seems clearer, since the viewport units change also changes style. But if you feel strongly about it, let's capitalize it properly as `styleChanged`. ::: layout/style/ServoStyleSet.cpp:1459 (Diff revision 4) > ServoStyleSet::UpdateStylist() > { > MOZ_ASSERT(StylistNeedsUpdate()); > + > + if (mStylistState & StylistState::StyleSheetsDirty) { > - Element* root = mPresContext->Document()->GetDocumentElement(); > + Element* root = mPresContext->Document()->GetDocumentElement(); This needs a rebase, but it's trivial ::: layout/style/ServoStyleSet.cpp:1463 (Diff revision 4) > + if (mStylistState & StylistState::StyleSheetsDirty) { > - Element* root = mPresContext->Document()->GetDocumentElement(); > + Element* root = mPresContext->Document()->GetDocumentElement(); > - Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root); > + } > + > + if (MOZ_UNLIKELY(mStylistState & StylistState::XBLStyleSheetsDirty)) { Let's assert here `IsMaster` when rebased.
Attachment #8902564 - Flags: review?(emilio) → review+
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/684b78bb1062 Part 1 - Add method to nsBindingManager to iterate all bound contents. r=emilio https://hg.mozilla.org/integration/autoland/rev/06fd7e672abd Part 2 - Make nsBindingManager::MediumFeaturesChanged() return bool directly. r=emilio https://hg.mozilla.org/integration/autoland/rev/b49d8d47c3f1 Part 3 - Support media changes for XBL stylesheets. r=emilio
Backed out for failing mochitest test_media_queries_dynamic.html on Windows stylo: https://hg.mozilla.org/integration/autoland/rev/886e7715645399d5225b47136b797ea54bc8dbde https://hg.mozilla.org/integration/autoland/rev/61152488edf16a7afa1fe0fb864e9f9add378bd9 https://hg.mozilla.org/integration/autoland/rev/f4f9c5a839c20749559ff28dcab207a429e733f0 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b49d8d47c3f1ab82c7c9fbf65c4741960542d7ca&filter-resultStatus=testfailed&filter-resultStatus=runnable&filter-resultStatus=busted&filter-resultStatus=usercancel&filter-resultStatus=retry&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128028841&repo=autoland 17:43:21 INFO - 618 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | full zoom is not 1.0 17:43:21 INFO - 619 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | all and (min-width: 11em) should not apply 17:43:21 INFO - 620 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | all and (min-width: 8em) should apply 17:43:21 INFO - 621 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | all and (max-width: 11em) should apply 17:43:21 INFO - 622 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | all and (max-width: 8em) should not apply 17:43:21 INFO - 623 INFO TEST-PASS | layout/style/test/test_media_queries_dynamic.html | full zoom is 1.0 17:43:21 INFO - Buffered messages finished 17:43:21 ERROR - 624 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_media_queries_dynamic.html | restyle: change width with no media queries - got true, expected false 17:43:21 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 17:43:21 INFO - flush_and_assert_change_counters@layout/style/test/test_media_queries_dynamic.html:146:6 17:43:21 INFO - run@layout/style/test/test_media_queries_dynamic.html:158:3 17:43:21 INFO - onload@layout/style/test/test_media_queries_dynamic.html:1:1
Flags: needinfo?(tlin)
test_media_queries_dynamic.html is testing again whether restyle is expected to happen. My Part 3 will make restyle happen when the PresContext is changed [1] in the binding even if there's no media features that could cause a restlye. [1] https://hg.mozilla.org/integration/autoland/rev/b49d8d47c3f1#l1.43
Flags: needinfo?(tlin)
Comment on attachment 8904226 [details] style: Expose stylist::set_device() for gecko https://reviewboard.mozilla.org/r/175980/#review181026
Attachment #8904226 - Flags: review?(emilio) → review+
Comment on attachment 8904227 [details] Bug 1382078 Part 4 - Avoid unneeded restyle when XBL styleset is utilized by different PresContext. https://reviewboard.mozilla.org/r/175982/#review181028
Attachment #8904227 - Flags: review?(emilio) → review+
Attached file Servo PR #18367 —
Attachment #8904226 - Attachment is obsolete: true
Pushed by tlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c8b4641d5f2 Part 1 - Add method to nsBindingManager to iterate all bound contents. r=emilio https://hg.mozilla.org/integration/autoland/rev/af60919d25dc Part 2 - Make nsBindingManager::MediumFeaturesChanged() return bool directly. r=emilio https://hg.mozilla.org/integration/autoland/rev/e06654eaa8c7 Part 3 - Support media changes for XBL stylesheets. r=emilio https://hg.mozilla.org/integration/autoland/rev/b58e78685334 Part 4 - Avoid unneeded restyle when XBL styleset is utilized by different PresContext. r=emilio
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: