798592 - Error console has warning "Unknown property 'align-self'. Declaration dropped." (& same for 'order' property)

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Run nightly, opening just e.g. about:blank

EXPECTED RESULTS: No warnings in error console

ACTUAL RESULT: One warning:
Warning: Unknown property '-moz-align-self'.  Declaration dropped.
Source File: resource://gre-resources/ua.css
Line: 44

This is expected, because this is a flexbox property, and the flexbox pref is default-disabled for now.  Once we default-enable it, it won't be a problem anymore, but in the meantime, it'd be nice to avoid spamming the error console.

We could probably fix this by shifting that declaration into an @supports rule: 
 http://mcc.id.au/blog/2012/08/supports

Comment 1

[Brian Carr]

This will be my first patch but I'd be happy to take a crack at it if no one else has volunteered yet.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Go for it. Below are some tips for patches you submit.

https://developer.mozilla.org/en-US/docs/Participating_in_the_Mozilla_project
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 3

[Daniel Holbert [:dholbert]]

Thanks Brian! You'll need to check out and build the source, too. This page can help with that:
  https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

The file you'll want to edit is "layout/style/ua.css" in the source code, here:
  https://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css

You can also ask for help in #introduction or #developers on irc.mozilla.org -- more info here, if you need help connecting:
  https://wiki.mozilla.org/IRC

(hopefully we're not overwhelming you too much with links :))

Comment 4

[Brian Carr]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Thanks Brian! You'll need to check out and build the source, too. This page
> can help with that:
>   https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> 
> The file you'll want to edit is "layout/style/ua.css" in the source code,
> here:
>   https://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css
> 
> You can also ask for help in #introduction or #developers on irc.mozilla.org
> -- more info here, if you need help connecting:
>   https://wiki.mozilla.org/IRC
> 
> (hopefully we're not overwhelming you too much with links :))

Ok The source code is built.  I'm going to go try and figure this out.  I'll let you know if I have any questions.

Comment 5

[Brian Carr]

Attachment: proposed patch: in progress

Ok, this is what I came up with.  I read all the documentation I could find on @supports and I think what I wrote makes sense, but I've never used @supports before and I haven't gotten a chance to test the fix yet (see next paragraph) so I could be completely off.  If so let me know.

Also is there a good way to recompile just files that I've changed for testing purposes?  I'm sure the must be a way to add in changes without rebuilding the whole application; I just haven't found it yet.  Additionally, how would you recommend I go about testing this?  I did a debug build but I haven't had much of a chance to play with it yet so any advice would be appreciated.

One more question: what's your policy on comments?  I added one next to my fix but I don't know if it's in the correct format or if I'm even supposed to put one there.  I'll remove or modify it if necessary.

Sorry if it seems like I'm asking a thousand questions, I'm just trying to get an idea of how everything works.  I really appreciate any advice I get because I want to do this right.

Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

I'm not sure, but I *think* the "@supports" needs to happen at the highest level -- so you'd need to split it out into a separate "*|*::-moz-table-outer {...} block, with just the align-self line, inside of an @supports section.

> Also is there a good way to recompile just files that I've changed for
> testing purposes?  I'm sure the must be a way to add in changes without
> rebuilding the whole application; I just haven't found it yet.

Generally, "yes", but I don't know how for ua.css.  A full recompile (make -f client.mk build) should hopefully only take a few minutes, though.

> Additionally, how would you recommend I go about testing this?  I did a
> debug build but I haven't had much of a chance to play with it yet so any
> advice would be appreciated.

(1) Follow steps in comment 0 -- see if you get that spam in error console anymore.  That will tell you if it's getting ignored when the pref is off (as we'd like it to)

RE whether it's honored when the pref is on -- that's a little trickier; I think we have a test for that, but I'll have to check and get back to you.

> One more question: what's your policy on comments?  I added one next to my
> fix but I don't know if it's in the correct format or if I'm even supposed
> to put one there.  I'll remove or modify it if necessary.

just match local style, usually. The one you added is fine, except it makes the line a little long. (generally we try to linebreak at ~80 characters)

> Sorry if it seems like I'm asking a thousand questions, I'm just trying to
> get an idea of how everything works.  I really appreciate any advice I get
> because I want to do this right.

No worries! thank you very much for the contribution.

One other important thing -- you should attach a patch, rather than just a copy of the edited file.  You can generate a patch easily using "hg diff > my_patch.txt", or even better, follow the directions in Ryan's second link from comment 2 to generate a patch with correct header information (with annotations for you as the author, a commit message included, etc.)

Comment 7

[Brian Carr]

Attachment: Proposed Patch: 10/9/12

Hey, sorry I dropped off the face of the Earth for a couple of days.  I had a big midterm but now I'm back.

Anyways, you are indeed correct that the @supports has to be at the highest level.  I've made changes to correct that.  The patch has gotten rid of the error on my machine so I believe that aspect works.  I think you mentioned that there may be another test we need to perform to ensure that the @supports doesn't interfere when flexbox is active though.  What does that involve?

Also I attempted to use Mq so hopefully this patch is formatted better than the first.  Let me know about any feedback you may have.

Thanks!

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 669690 [details] [diff] [review]
Proposed Patch: 10/9/12

>+/* Bug 798592 fix: unnecessary once flexbox is default enabled */
>+@supports (display : flexbox) { 
>+  *|*::-moz-table-outer {
>+    -moz-align-self: inherit; /* needed for "align-self: auto" to work on table */
>+  }
>+}
>+

No worries -- sorry about not following up on how to tell if the rule is still being honored. :)  I meant to do that sooner, but didn't get to it until this morning, and I discovered that it's _supposed_ to be tested, but there's another bug that means it's not at the moment (oops!).   I'll file that separately.

Anyway -- this looks good, with one exception: replace "display : flexbox" with "display: -moz-flex".  (The flexbox display keyword indeed used to be "flexbox", but it was shortened to "flex" at some point -- see [1] for reference. And our implementation is prefixed for the time being, so you need the "-moz-" as well.)

So -- just change that, add a commit message by running this while the patch is applied in your MQ:
  hg qref -m "Bug 798592: [description of the change]"
and reattach the patch-file, request review from me[2], and you'll be good!

[1]  http://www.w3.org/TR/css3-flexbox/#flex-containers
[2] (When you attach the patch, toggle the "review" box to "?", and type ":dholbert" into the box that appears.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> I discovered that it's _supposed_ to be tested, but
> there's another bug that means it's not at the moment (oops!).   I'll file
> that separately.

(for reference, I filed bug 799647.  With that bug fixed and the flexbox pref toggled on, we depend on the ua.css snippet here ( *|*::-moz-table-outer { -moz-align-self: inherit ]) in order to pass the unit test "flexbox-align-self-baseline-horiz-1.xhtml".  I'll be including more thorough tests for this on that bug, as well.)

Comment 10

[Daniel Holbert [:dholbert]]

I added one more flexbox-specific property ("-moz-order") to ua.css, here:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f858236f4e62#l4.12
(That's on mozilla-inbound, and it should make it over to mozilla-central in a day or so)

This bug's patch should hoist that line into the new @supports chunk, too.

Comment 11

[Daniel Holbert [:dholbert]]

Just a heads-up: the code that it touches changed bit more, in bug 801098.  These are now just "align-self" and "order". (no -moz prefix)  And the "@supports" rule should check for "display: flex" (not -moz-flex)

I think that's the last thing that should be changing things out from under you, though. :)

Comment 12

[Daniel Holbert [:dholbert]]

Brian: Still working on this?  Patch just needs a few tweaks, and then it'll be landable -- see comment 11.  Thanks!

Comment 13

[Brian Carr]

Oh I am so, so, so sorry about that.  I somehow got the impression that it had been fixed by one of you but upon re-reading those posts it appears that the bug was only moved.  I thought it was complete so I stopped worrying about it.  That's completely my bad.  I'll get on it as soon as I finish coding my current project for class, so it should be done either tonight or tomorrow.

Comment 14

[Daniel Holbert [:dholbert]]

No problem! No huge rush (otherwise I would have posted sooner), & thanks in advance for the new patch! :)

Comment 15

[Brian Carr]

Attachment: new proposed patch

Once again, sorry about not checking back for a couple weeks.  My fault.

Anyways, here's a new proposed patch.  I was wondering if the "*|*::-moz-table-outer {" is even necessary.  I had it in my previous patches but no I'm not really sure.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 676919 [details] [diff] [review]
new proposed patch

(In reply to Brian Carr from comment #15)
> Once again, sorry about not checking back for a couple weeks.  My fault.

It's ok, don't worry about it. :)

> Anyways, here's a new proposed patch.

Looks good to me!

> I was wondering if the
> "*|*::-moz-table-outer {" is even necessary.

Yes, it is necessary.  It's a selector for "moz-table-outer pseudo-elements", which we generate as wrappers around <table> elements for some arcane reason to get tables to work correctly. (I don't recall the details at the moment) :)  The important thing that makes this whole chunk of code necessary is that these pseudo-elements are *children of the table* in the style tree, but they're the *parent of the table* in the content tree. (This is very rare... might be the only case where this happens?)  So when we have a table inside a flex container, these pseudo-elements are what the container interacts with, in the content tree -- not the table itself.  Since the flex container inspects its children's "align-self" and "order" properties, we need the hacky style rules in this patch's contextual code in order for the container to see the values that the author has specified on the table.  The CSS makes the author-specified property-values "inherit upwards" from the table to its table-outer pseudo-element, so that the flex container can see them.

Hopefully that made some sense, but it doesn't matter too much -- it's mostly beyond the scope of this patch. As far as this bug is concerned: these property-settings *are* specific to the moz-table-outer selector, but we only need these flexbox-related properties to be set if flexbox is preffed on.  And your patch does appear to fix that (from reading it, at least). Thanks!

I'll test this locally as a sanity-check, and land it assuming all goes well.

Comment 17

[Daniel Holbert [:dholbert]]

Unfortunate news: The patch doesn't seem to do what I was hoping -- that is, the warnings remain in error console.  But it's not your fault -- it may be because I filed this bug on a faulty premise.  I thought @supports would prevent us from parsing (or at least complaining about parse errors in) these properties, but it doesn't seem to behave that way.

I filed bug 807336 on this, on the off chance that my expectation here was correct and our @supports implementation just needs fixing.

Very sorry for possibly sending you on a wild goose chase -- I'll report back when I hear more from the other bug.

Comment 18

[Daniel Holbert [:dholbert]]

Woot, looks like my expectations weren't off-base after all.  Cameron (the implementor of @supports) agrees with me, over in bug 807336 comment 2.

So: up until bug 807336 is fixed, this bug's patch won't have any effect (and we can't technically verify that it works) -- so I'm not landing it yet.  But when that bug *is* fixed, I'll land this patch.

Comment 19

[Cameron McCormack (:heycam)]

What happens if the @supports pref is off?  I don't think parsing of @supports rules in chrome style sheets is force enabled.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Cameron McCormack (:heycam) from comment #19)
> What happens if the @supports pref is off?

Ah, good point.  Assuming that means the whole "@supports (...) {...} block gets skipped.  Nothing too bad will happen from that -- for people who've selectively toggled the flexbox pref *on* and the @supports pref *off* (probably nobody), two flexbox-specific properties won't be honored on table flex items (tables that are children of a "display:flex" element).  See the second half of comment 16 for details.

So -- as long as CSS-flexbox is default-disabled, I don't think that's something we should worry about, since the set of people who've toggled that pref *and* turned off @supports is likely zero.  (And even if it's nonzero, the failure is kind of edge-casey and isn't too horrible, so I don't think we need to worry about adding special cases to support those users for the time being.)

However -- once CSS flexbox is default-enabled, then all of the people who've toggled the @supports pref to off will potentially be impacted.  That is, they'll have CSS flexbox mostly working, except for table flex items.  At that point, we should probably remove the @supports check and restore the current code.

Comment 21

[Cameron McCormack (:heycam)]

OK, sounds good to me.

I wonder if we are moving now towards features behind prefs if we are likely to run into this situation again (spec hasn't reached CR yet, so we haven't removed the pref, but we use the feature internally), and weather there should be a way for chrome style sheets to be able to use these features regardless of the pref.

Comment 22

[Daniel Holbert [:dholbert]]

Given that @supports is now disabled-by-default in release builds[1], and given that flexbox should soon be enabled-by-default[2], I don't think it's worth trying to fix this bug at this point.  The fix necessarily requires @supports to be functional, and we don't want to gate flexbox support on that unnecessarily.  (And we don't want flexbox to arbitrarily break in builds where people have manually disabled the @supports pref).

So: I'm morphing the summary slightly to make this just track the error-console warning, and I'm making this depend on bug 841876 (which will make the flexbox pref default-enabled everywhere,  fixing the warning.)  I think the right way to "fix" this is just to have bug 841876 land.

I'm also removing the mentor whiteboard-status, and clearing the assignee field. (Brian: sorry for leading you astray on this bug! When I filed it, I wasn't aware that @supports was experimental and pref-controlled -- my bad on that.  I'd be help you with another good-first-bug at some point -- hopefully the lack of movement on this bug hasn't crushed your spirits too much. :))

[1] https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js?rev=1fc2be2d8cb7#1690
[2] When bug 841876's dependency list is settled & all of its dependencies are resolved

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> I'd be help you with another good-first-bug at some point
(er "I'd be happy to help you with another good-first-bug" :) )

Comment 24

[Masataka Yakura [:myakura]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> Given that @supports is now disabled-by-default in release builds[1], and
> given that flexbox should soon be enabled-by-default[2], I don't think it's
> worth trying to fix this bug at this point.  The fix necessarily requires
> @supports to be functional, and we don't want to gate flexbox support on
> that unnecessarily.  (And we don't want flexbox to arbitrarily break in
> builds where people have manually disabled the @supports pref).
> 
> So: I'm morphing the summary slightly to make this just track the
> error-console warning, and I'm making this depend on bug 841876 (which will
> make the flexbox pref default-enabled everywhere,  fixing the warning.)  I
> think the right way to "fix" this is just to have bug 841876 land.

Since now @supports is enabled by default, and will come together with Flexbox in Firefox 22, what should we do?

[1] http://mcc.id.au/blog/2013/04/supports-enabled-by-default

Comment 25

[Cameron McCormack (:heycam)]

Well, it depends on if we want to still support the pref being flipped off manually and that not breaking flexbox.  If we do, then we can't land the patch yet.  I filed bug 864005 for removing the @supports pref.

Incidentally, I feel like CSS errors in UA style sheets should be reported to the console in debug builds, rather than the error console in all builds.

Comment 26

[sjw]

It seems fixed, right?

Comment 27

[Daniel Holbert [:dholbert]]

Technically, this should still be broken for anyone with the pref layout.css.flexbox.enabled turned off.

However, layout.css.flexbox.enabled is on by default and has been for some time now (and the pref will probably be removed before too long), so no one should really be hitting this bug.

So: given that you have to manually toggle a to-be-removed pref to hit this (& given that this doesn't cause any real problems even if you do toggle that pref), I think this is WONTFIX at this point.

Comment 28

[Daniel Holbert [:dholbert]]

(I just filed bug 936100 on removing the pref, BTW.)