Add style-system support for 'display: grid' and 'display: inline-grid'

RESOLVED FIXED in mozilla30

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: SimonSapin, Assigned: SimonSapin)

Tracking

(Blocks 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 7 obsolete attachments)

8.96 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
27.73 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
This bug is about supporting in the style system the new properties as well as the new values for the 'display' property introduced by CSS Grid Layout (Bug 616605). Both are behind a new "layout.css.grid.enabled" preference.

It also includes a new GridContainerFrame frame type (generated by 'display: grid' and 'display: inline-grid') that does nothing. This is largely modeled after FlexContainerFrame.

These is incomplete for now, I only have code for two of the new properties. I’d like to get review on this much before repeating possible mistakes a dozen times. I also need some help debugging a segfault. (Three patches coming.)
Assignee

Comment 1

5 years ago
Attachment #8379876 - Flags: review?(dholbert)
Comment hidden (obsolete)
Assignee

Comment 4

5 years ago
This fixes the segfault in attachment 8379886 [details] [diff] [review]. Thanks to froydnj for the help on IRC and spotting the issue with nsStylePosition’s copy constructor.
Attachment #8379886 - Attachment is obsolete: true
Attachment #8379886 - Flags: review?(dholbert)
Attachment #8379917 - Flags: review?(dholbert)

Updated

5 years ago
See Also: → 914360
Comment on attachment 8379876 [details] [diff] [review]
Patch 1: Add a XUL prefix to existing GRID constants.

>From: Simon Sapin <simon.sapin@exyr.org>
>Add a XUL prefix to existing GRID constants.

s/existing GRID constants/existing internal GRID constants/

(That "internal" hopefully makes it clearer that this doesn't affect behavior -- i.e. we're not renaming "display: -moz-grid" to "display: -moz-xul-grid". (which is what I initially guessed this patch might be doing, based on its attachment-title)

>+++ b/layout/style/nsStyleConsts.h
>@@ -389,20 +389,20 @@ static inline mozilla::css::Side operator++(mozilla::css::Side& side, int) {
> #define NS_STYLE_DISPLAY_INLINE_BOX             19
> #ifdef MOZ_XUL
>-#define NS_STYLE_DISPLAY_GRID                   20
>-#define NS_STYLE_DISPLAY_INLINE_GRID            21
>-#define NS_STYLE_DISPLAY_GRID_GROUP             22
>-#define NS_STYLE_DISPLAY_GRID_LINE              23
>+#define NS_STYLE_DISPLAY_XUL_GRID                   20
>+#define NS_STYLE_DISPLAY_INLINE_XUL_GRID            21
>+#define NS_STYLE_DISPLAY_XUL_GRID_GROUP             22
>+#define NS_STYLE_DISPLAY_XUL_GRID_LINE              23

Please adjust the whitespace there to keep the numeric values aligned with the ones above/below them.

r=me with that, assuming this compiles.
Attachment #8379876 - Flags: review?(dholbert) → review+
Comment on attachment 8379878 [details] [diff] [review]
Patch 2: Add display:{inline-,}grid behind a pref, and a no-op grid container frame.

I'll sanity-check this in more detail on Monday, but here are a few things I noticed so far:

>+++ b/layout/base/nsCSSFrameConstructor.cpp

In this file, you'll need to add a chunk to nsCSSFrameConstructor::ConstructDocElementFrame(), basically identical to the "flex" chunk that I added last week in bug 969460.

Without that, <html style="display:grid"> won't work. (it'll end up producing a block, and it'll assert fatally in debug builds)

Also, without that change, I expect you'll trigger a fatal assertion-failure in the mochitest that I added last week for that bug (test_root_node_display.html) -- at least, you *should* fail a fatal assertion. If not, please let me know, as that'd be a bug in the test. (It's supposed to look for this exact issue.)

(I suspect you haven't run up against this yet just because I added the test so recently.)

>+++ b/layout/generic/nsGridContainerFrame.cpp
>+void
>+nsGridContainerFrame::DestroyFrom(nsIFrame* aDestructRoot)
>+{
>+  DestroyAbsoluteFrames(aDestructRoot);
>+  nsGridContainerFrameSuper::DestroyFrom(aDestructRoot);
>+}

I don't think you need this.

nsContainerFrame::DestroyFrom already calls DestroyAbsoluteFrames, so you don't need an additional call to DestroyAbsoluteFrames here. Hence, you should be fine relying entirely on the inherited version, and getting rid of this method entirely.

Reference:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=f6d37fdcc976&mark=240-240#233

>diff --git a/layout/generic/nsGridContainerFrame.h b/layout/generic/nsGridContainerFrame.h
>new file mode 100644
>index 0000000..c7a1e91
>--- /dev/null
>+++ b/layout/generic/nsGridContainerFrame.h
>+#include "nsContainerFrame.h"
>+#include "mozilla/Types.h"

Do we actually need this Types.h include here? I don't think any other frame classes bother with that in their .h files, so I suspect it's unnecessary.

(If your build breaks when you remove it, maybe a forward decl of something would suffice?)

>+public:
>+  // nsIFrame overrides
>+  virtual nsIAtom* GetType() const MOZ_OVERRIDE;

Please also add a GetFrameName impl, too, so that this frame will be reported correctly in frametree dumps.  (This is usually declared right after GetType, and should be defined/declared inside #ifdef DEBUG_FRAME_DUMP.)

>+  // Protected nsIFrame overrides:
>+  virtual void DestroyFrom(nsIFrame* aDestructRoot);

If you actually need this, then it should be declared as MOZ_OVERRIDE -- BUT -- I don't think you need it. (see comments above, for this method in the .cpp file)

>+};
>+
>+
>+#endif /* nsGridContainerFrame_h___ */

Drop one of those two blank lines after "};"

>+++ b/modules/libpref/src/init/all.js
>+// Is support for CSS Grid enabled?
>+pref("layout.css.grid.enabled", false);
>+

I think we should avoid putting this pref in all.js, to prevent it from being discoverable in about:config, *until* we actually have a usable frame class. Otherwise, users might stumble across it and turn it on, expecting it to do something, when really it doesn't do anything yet. (and might even crash in some cases? hopefully not)

Any users who *really* want to just test the current style-system stuff (& can handle the fact that the layout is still missing) can take the extra step to add the about:config pref themselves.)

Once we have some grid layout functionality implemented, it'll be more useful to add the pref to all.js (and make it discoverable to power-users). 

>diff --git a/testing/profiles/prefs_general.js b/testing/profiles/prefs_general.js
>+// Enable CSS Grid for testing
>+user_pref("layout.css.grid.enabled", true);

I'm mixed on the cost/benefit of adding prefs to be globally enabled for all mochitests...  But in this case, I think this probably makes sense, since it will get us testing in all of the property_database.js-based tests, and shouldn't really affect anything else at this point.
(In reply to Daniel Holbert [:dholbert] (out of office until 2/24) from comment #6)
> >+++ b/layout/base/nsCSSFrameConstructor.cpp
> 
> In this file, you'll need to add a chunk to
> nsCSSFrameConstructor::ConstructDocElementFrame(), basically identical to
> the "flex" chunk that I added last week in bug 969460.

That chunk being:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=969a7e228596&mark=2418-2423#2418

You can copypaste that, with s/flex/grid/.
Assignee

Comment 8

5 years ago
Addressed review comments.
Attachment #8379876 - Attachment is obsolete: true
Attachment #8380374 - Flags: review+
Comment on attachment 8380374 [details] [diff] [review]
Patch 1 v2: Add a XUL prefix to existing internal GRID constants. r=dholbert

>From: Simon Sapin <simon.sapin@exyr.org>
>Add a XUL prefix to existing internal GRID constants. r=dholbert

(Commit message should start out with "Bug 123: " or mention the bug number somewhere, too; not sure if you're planning on adding that later)

Updated

5 years ago
Assignee: nobody → simon.sapin
Severity: normal → enhancement
Assignee

Comment 10

5 years ago
Bug number in the commit message.
Attachment #8380374 - Attachment is obsolete: true
Attachment #8380517 - Flags: review+
Assignee

Comment 11

5 years ago
> Also, without that change, I expect you'll trigger a fatal assertion-failure
> in the mochitest that I added last week for that bug
> (test_root_node_display.html) -- at least, you *should* fail a fatal
> assertion. If not, please let me know, as that'd be a bug in the test. (It's
> supposed to look for this exact issue.)
> 
> (I suspect you haven't run up against this yet just because I added the test
> so recently.)

Yes, rebasing onto a more recent mozilla-central got me that test. It failed with the previous version of this patch, although only in a debug build.
Attachment #8379878 - Attachment is obsolete: true
Attachment #8379878 - Flags: review?(dholbert)
Attachment #8380546 - Flags: review?(dholbert)

Comment 12

5 years ago
Simon, would it be possible to fix Bug 914360 alongside this bug? Having XUL-Grid exposed to content may lead to problems (at least misunderstandings) when both XUL- and CSS-Grid are avialable, e.g. while testing.
(In reply to Florian Bender from comment #12)
> Simon, would it be possible to fix Bug 914360 alongside this bug?

Assuming we want to fix Bug 914360, we should perhaps do it before we *turn on the pref* for CSS grid; however, I don't think it needs to block this bug here.

Per comment 0, I think there's a lot more to come here; let's not increase the scope of this bug.
Comment on attachment 8380546 [details] [diff] [review]
Patch 2 v2: Add display:{inline-,}grid behind a pref, and a no-op grid container frame.

>From: Simon Sapin <simon.sapin@exyr.org>
>Bug 975501: Add display:{inline-,}grid behind a pref, and a no-op grid container frame.

Maybe s/no-op/stub/?  I think of "no-op" as describing functions, not objects.

>+++ b/layout/generic/nsGridContainerFrame.cpp
>+#ifdef DEBUG_FRAME_DUMP
>+nsresult
>+nsFlexContainerFrame::GetFrameName(nsAString& aResult) const
>+{
>+  return MakeFrameName(NS_LITERAL_STRING("GridContainer"), aResult);
>+}
>+#endif

s/Flex/Grid/ in the function name here ;)

>+++ b/layout/generic/nsGridContainerFrame.h
>+#include "nsContainerFrame.h"
>+#include "mozilla/Types.h"

As noted in comment 6, I'm pretty sure you can drop the Types.h include.

(Looks like I had this #include in my original nsFlexContainerFrame.h patch, which is probably why you have it here; I'm not sure why I had it, but I'm betting it was unnecessary.)

r=me with the above
Attachment #8380546 - Flags: review?(dholbert) → review+
Assignee

Comment 15

5 years ago
Florian, I’m happy to look at bug 914360 when there’s a decision on what to do, but it won’t go in this set of patches. (Is that what you mean by "alongside"?)
Assignee

Comment 16

5 years ago
I’m not very satisfied with the changes to nsCSSValue.cpp in Patch 3 v2. Would it make sense to add new variants to nsCSSUnit and nsCSSValue.mValue instead?

https://bugzilla.mozilla.org/attachment.cgi?id=8379917&action=diff#a/layout/style/nsCSSValue.cpp_sec2
Assignee

Comment 18

5 years ago
Comment on attachment 8379917 [details] [diff] [review]
Patch 3 v2: Add the grid-template-{columns,rows} properties the style system.

Moved to bug 975501.
Attachment #8379917 - Attachment is obsolete: true
Attachment #8379917 - Flags: review?(dholbert)
Assignee

Updated

5 years ago
Summary: Style system for CSS Grid Layout → CSS Grid Layout style system, part 1
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 19

5 years ago
Comment on attachment 8379917 [details] [diff] [review]
Patch 3 v2: Add the grid-template-{columns,rows} properties the style system.

Correction of previous comment: moved to bug 976787.
Assignee

Updated

5 years ago
Summary: CSS Grid Layout style system, part 1 → Add style-system support for 'display: grid' and 'display: inline-grid'
This probably needs a Try run before it lands. Simon, has a recent-enough version of these patches already gotten a Try run, & if not, could you trigger one?

[temporarily removing "checkin-needed" so that this doesn't land in the meantime]
Flags: needinfo?(simon.sapin)
Keywords: checkin-needed
Assignee

Comment 21

5 years ago
Compared to attachment 8380657 [details] [diff] [review], this only adds "grid" and "inline-grid" to the expected return values of utils.getCSSValuesForProperty() in layout/inspector/tests/test_bug877690.html

I assume this change is small enough that the r+ on the earlier patch still applies.

In the Try results here: https://tbpl.mozilla.org/?tree=Try&rev=3af52df55a4f , most of the orange is from test_bug877690.html and should be fixed by this change. I’m told the rest are known intermittent.
Attachment #8380657 - Attachment is obsolete: true
Attachment #8382638 - Flags: review+
Flags: needinfo?(simon.sapin)
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3afc42ad2185
https://hg.mozilla.org/mozilla-central/rev/3038a5a34a75
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.