Closed Bug 1356241 Opened 3 years ago Closed 2 years ago

The computed value of column-gap when specifying 'normal' should be 'normal' keyword.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mantaroh, Assigned: hiro)

Details

Attachments

(3 files)

The getComputedStyle value of column-gap is length when specifying the 'normal' keyword. However, specification explain that computed value is 'absolute length or normal'[1]. So we will need to return 'normal' keyword when specifying the 'normal'.

[1] https://drafts.csswg.org/css-multicol-1/#column-gap

When I try the example code, Fireforx return '16px' on my OSX environment, Chrome/Safari return 'normal' keyword. [2]

[2] http://jsbin.com/nihupof
Priority: -- → P3
I was wondering some test cases [1] in web platform test fail on stylo either. I thought it's an animation problem, but it's just a problem in nsComputedDOMStyle.cpp.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e16328e84c8446b41d81d61434767a4c37c841d8

[1] https://hg.mozilla.org/mozilla-central/file/4e93516e92e5/testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property.html.ini#l4
Comment on attachment 8897675 [details]
Bug 1356241 - Set the pref for overflow-clip-box true since it's not contained in prefs_general.js.

https://reviewboard.mozilla.org/r/168956/#review174298

::: testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini:1
(Diff revision 1)
> +prefs: [layout.css.contain.enabled:true, layout.css.initial-letter.enabled:true, layout.css.overflow-clip-box.enabled:true, layout.css.shape-outside.enabled:true]

Actually I haven't investigated why accumulation-per-property.html works fine without these prefs unlike others.  Daisuke, if you want to know the reason, I will file a new bug for it and drop this patch in this bug.
Comment on attachment 8897675 [details]
Bug 1356241 - Set the pref for overflow-clip-box true since it's not contained in prefs_general.js.

https://reviewboard.mozilla.org/r/168956/#review174314

::: testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property.html.ini:1
(Diff revision 1)
> +prefs: [layout.css.contain.enabled:true, layout.css.initial-letter.enabled:true, layout.css.overflow-clip-box.enabled:true, layout.css.shape-outside.enabled:true]

Thanks, Hiro.
I tested other addition-per-property.html and interpolation-per-property.html without those prefs in local.
The both tests worked fine as well except overflow-clip-box (this was skipped).
I checked the prefs in about:config on the testing window, they were modified to be true except layout.css.overflow-clip-box.enabled.
We do not undestand the cause yet, so I think that it would be better to leave it as it is or set layout.css.overflow-clip-box.enabled at least. Because overflow-clip-box would be skipped in "accumulation-per-property.html" as well.

Then let me investigate in another bug.
Comment on attachment 8897674 [details]
Bug 1356241 - Return specified 'normal' value for column-gap computed value.

https://reviewboard.mozilla.org/r/168954/#review174648

::: layout/style/test/property_database.js:1539
(Diff revision 1)
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
> -    initial_values: [ "normal", "1em", "calc(-2em + 3em)" ],
> +    initial_values: [ "normal" ],
>      other_values: [ "2px", "4em",
>        "calc(2px)",
>        "calc(-2px)",

Could you add "1em" to the other_values list (maybe just before the "4em" entry), so that both of your removed initial_values are being preserved somewhere?

This will help validate that we don't accidentally serialize an author-specified "1em" as "normal", for example. (even though they mean the same thing under the hood for layout purposes)
Attachment #8897674 - Flags: review?(dholbert) → review+
Comment on attachment 8897675 [details]
Bug 1356241 - Set the pref for overflow-clip-box true since it's not contained in prefs_general.js.

https://reviewboard.mozilla.org/r/168956/#review174650

It looks like right now you're deleting this .ini file entirely (in part 1), and then recreating it in part 2 (to set some prefs).

Seems like it would be better to avoid the delete-and-recreate dance -- if we need these prefs, perhaps you could make that change *first* (in a nonfunctional "part 0" patch), and then have the main patch just edit the .ini file so that that's all that remains?
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8897674 [details]
> Bug 1356241 - Return specified 'normal' value for column-gap computed value.
> 
> https://reviewboard.mozilla.org/r/168954/#review174648
> 
> ::: layout/style/test/property_database.js:1539
> (Diff revision 1)
> >      inherited: false,
> >      type: CSS_TYPE_LONGHAND,
> > -    initial_values: [ "normal", "1em", "calc(-2em + 3em)" ],
> > +    initial_values: [ "normal" ],
> >      other_values: [ "2px", "4em",
> >        "calc(2px)",
> >        "calc(-2px)",
> 
> Could you add "1em" to the other_values list (maybe just before the "4em"
> entry), so that both of your removed initial_values are being preserved
> somewhere?
> 
> This will help validate that we don't accidentally serialize an
> author-specified "1em" as "normal", for example. (even though they mean the
> same thing under the hood for layout purposes)

OK, fair enough. I will do it. Thanks!
Comment on attachment 8897675 [details]
Bug 1356241 - Set the pref for overflow-clip-box true since it's not contained in prefs_general.js.

https://reviewboard.mozilla.org/r/168956/#review174314

> Thanks, Hiro.
> I tested other addition-per-property.html and interpolation-per-property.html without those prefs in local.
> The both tests worked fine as well except overflow-clip-box (this was skipped).
> I checked the prefs in about:config on the testing window, they were modified to be true except layout.css.overflow-clip-box.enabled.
> We do not undestand the cause yet, so I think that it would be better to leave it as it is or set layout.css.overflow-clip-box.enabled at least. Because overflow-clip-box would be skipped in "accumulation-per-property.html" as well.
> 
> Then let me investigate in another bug.

> I checked the prefs in about:config on the testing window, they were modified to be true except layout.css.overflow-clip-box.enabled.
> We do not undestand the cause yet

The reason you're seeing them enabled is: they're listed in this "prefs_general.js" file, which gets used in the browser profile that we use to run tests:
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

(They're enabled there so that our property_database.js-based tests will be able to test their corresponding properties and catch regressions.  This is of debatable value, since it means we're testing a default-configuration that's different from what we ship to our users -- but that's a different conversation.)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Comment on attachment 8897675 [details]
> Bug 1356241 - Set preference values explicitly for
> accumulation-per-property.html.
> 
> https://reviewboard.mozilla.org/r/168956/#review174650
> 
> It looks like right now you're deleting this .ini file entirely (in part 1),
> and then recreating it in part 2 (to set some prefs).
> 
> Seems like it would be better to avoid the delete-and-recreate dance -- if
> we need these prefs, perhaps you could make that change *first* (in a
> nonfunctional "part 0" patch), and then have the main patch just edit the
> .ini file so that that's all that remains?

Yeah, but it was not clear to me that we need the preference values for the test file. Given that comment 5 and your comment 9 (that's really good to know! I did not know that at all), we still need (some) pref values there.  I will put the patch as part 0 here.  Thank you!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8897675 [details]
Bug 1356241 - Set the pref for overflow-clip-box true since it's not contained in prefs_general.js.

https://reviewboard.mozilla.org/r/168956/#review174716

Thank you very much, Daniel, Hiro!
Attachment #8897675 - Flags: review?(dakatsuka) → review+
Comment on attachment 8898056 [details]
Bug 1356241 - Drop preference values that are already set in prefs_general.js.

https://reviewboard.mozilla.org/r/169358/#review174722
Attachment #8898056 - Flags: review?(dakatsuka) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1cbd18a18186
Set the pref for overflow-clip-box true since it's not contained in prefs_general.js. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/a369a533833a
Return specified 'normal' value for column-gap computed value. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f34888c63675
Drop preference values that are already set in prefs_general.js. r=daisuke
https://hg.mozilla.org/mozilla-central/rev/1cbd18a18186
https://hg.mozilla.org/mozilla-central/rev/a369a533833a
https://hg.mozilla.org/mozilla-central/rev/f34888c63675
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.