Closed Bug 1239496 Opened 4 years ago Closed 4 years ago

grid-container-overflow-001.html is going to permafail when Gecko 45 merges to Beta


(Core :: Layout, defect, critical)

Not set



Tracking Status
firefox45 + fixed
firefox46 + fixed


(Reporter: RyanVM, Assigned: mats)



(1 file)

[Tracking Requested - why for this release]: Merge day bustage
Flags: needinfo?(mats)
I can reproduce this on Aurora with the following change to modules/libpref/init/all.js:
 pref("layout.css.grid.enabled", false);
-pref("layout.css.grid.enabled", true);

which puzzles me since the manifest file explicitly enables grid so the above
change should have no effect:

What am I missing?
Flags: needinfo?(mats) → needinfo?(ryanvm)
No clue. Maybe Cameron knows?
Flags: needinfo?(ryanvm) → needinfo?(cam)
We probably depend on grid being enabled *at startup-time* in order for the Grid-related inheritance-into-the-scrolled-frame rules in ua.css to be parsed correctly.

IIRC someone (cameron?) added a fix for this at some point, making us reparse ua.css and other stylesheets after certain prefs were flipped. Maybe we need to opt-in the grid pref to that mechanism..
Yeah, my testing supports that thesis.
When I start with the pref disabled, load the test (fails), then enable the pref in about:config
and shift+Reload the page it still fails (and looks like the rendering in the reftest-analyzer
for the job in comment 0).  If I now restart the browser (with the pref still enabled in
about:config) I get the expected rendering.
It looks like nsLayoutStylesheetCache::EnsureGlobal is missing a whole bunch of CSS features.
Shouldn't we add all of these for example:
We'll need to revert bug 1202940 part 1 (the mUASheet part at least) for this to work, too:

Otherwise the callback for the pref-change doesn't do anything.

(In reply to Mats Palmgren (:mats) from comment #6)
> It looks like nsLayoutStylesheetCache::EnsureGlobal is missing a whole bunch
> of CSS features.
> Shouldn't we add all of these for example:

Only if any of those features are needed for correct parsing of a UA stylesheet.
Also, do we have a race condition here?  EnsureGlobal() must be notified about the pref
changes after all the others are notified on order for it to work, right?
I'm surprised that only one of the grid reftests failed, can anyone explain that?
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Only if any of those features are needed for correct parsing of a UA
> stylesheet.

Hmm, but display:grid/inline-grid isn't used anywhere inside a UA stylesheet as far as I know.
So this failure shouldn't have happened if that were true?

I think we need Cameron to explain this.
Flags: needinfo?(cam)
No, but we do have a bunch of pref-controlled grid-property inheritance rules in a UA stylesheet:
154 *|*::-moz-scrolled-content, *|*::-moz-scrolled-canvas,
155 *|*::-moz-scrolled-page-sequence {
177   /* Grid container */
178   grid-auto-columns: inherit;
179   grid-auto-rows: inherit;
180   grid-auto-flow: inherit;
181   grid-column-gap: inherit;
182   grid-row-gap: inherit;
183   grid-template-areas: inherit;
184   grid-template-columns: inherit;
185   grid-template-rows: inherit;
MXR link:

I'm assuming the problem is that those ^ declarations are getting skipped (as unrecognized/invalid) at startup time, and then if we enable the grid pref later on without reparsing that stylesheet, we don't inherit any of those properties through to the inner grid frame inside of "overflow:hidden" scrollframes correctly.
Ah, right, I forgot about those.  And that explains why that particular test fails -
it's specifically testing grid layout with overflow:hidden.
Yup, exactly. (I think that addresses comment 9's needinfo --> clearing)
Flags: needinfo?(cam)
We should probably fix this so that enabling this pref works without restart.

We already have a callback here:
I'd prefer to add
there but that method is private:

(or we could add a new InvalidateAllTheSheetsYeahIMeanIt() method)

Adding separate pref callbacks in nsLayoutStylesheetCache for various prefs seems
messy to me since they need to run in a specific order.

n-i Cameron what solution he prefers.
Flags: needinfo?(cam)
(I think you'll want to do what he did in bug 1096808 for the ruby pref.  We could use the existing nsLayoutUtils callback if nsLayoutStylesheetCache was more public, as you noted, but it seems simpler & better separation-of-concerns to have each file care about pref-changes independently.)
Attached patch fixSplinter Review
Let me know if I missed something.

The css-grid reftests pass locally both with the pref enabled/disabled
by default.  Here's an Aurora Try push with the pref disabled by default
(thus triggering a dynamic update from the manifest file):
Assignee: nobody → mats
Attachment #8707745 - Flags: review?(cam)
It seems a bit fragile to have to change code in multiple places
to add a pref dependency to this code, fwiw.
Comment on attachment 8707745 [details] [diff] [review]

FYI, this patch is for the Aurora branch.  It looks like it applies to
trunk with some fuzz.
Comment on attachment 8707745 [details] [diff] [review]

Review of attachment 8707745 [details] [diff] [review]:

Thanks.  Yeah I agree it's not easy to notice that nsLayoutStylesheetCache needs to be updated for these prefs (which is why we missed doing it!) but I'm not sure if there's a better way.  We might be able to do something like have nsCSSParser assert that, when parsing a nsLayoutStylesheetCache-owned sheet encountering a pref-controlled property, its pref is handled by nsLayoutStylesheetCache somehow.
Attachment #8707745 - Flags: review?(cam) → review+
Comment on attachment 8707745 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: bug 1227162 I guess
[User impact if declined]: turning CSS Grid support on/off (using a pref in about:config) doesn't work without restarting the browser.  That's not terribly important for users but we need it for running reftests.
[Describe test coverage new/current, TreeHerder]: I think this code is well covered in general.  Reloading the style sheet when the pref changes has isn't tested though but it will be on the next merge to beta (and then it will run for every reftest job)
[Risks and why]: low, a similar patch have been used before so the general approach have been tested
[String/UUID change made/needed]: none
Flags: needinfo?(cam)
Attachment #8707745 - Flags: approval-mozilla-aurora?
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Tracking for 45+ since we don't want the tests to fail on merge day in a week.
Comment on attachment 8707745 [details] [diff] [review]

Looks good on Try too.
Attachment #8707745 - Flags: feedback+
Comment on attachment 8707745 [details] [diff] [review]

Helps the testsuite, taking it.
Attachment #8707745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.