1117983 - need logical-direction equivalents of the CSS width and height properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Kew [:jfkthame]]

In addition to logical versions of the box-edge-related properties (margins, borders, padding) in bug 1083134, we'll also need logical versions of the width and height properties, and their min- and max- counterparts.

This is needed to make it possible to write stylesheets such as html.css, ua.css, forms.css, etc., in a form that works for both horizontal and vertical writing modes.

:heycam, any chance you could handle this along with the work you're already doing in bug 1083134?

Comment 1

[Cameron McCormack (:heycam)]

This is offset-{block,inline}-{start,end}?  Yes, I can take a look at that.

One issue: my patches in bug 1083134 assume that logical longhand properties all have a corresponding 4-component physical shorthand they correspond to.  For example, margin-inline-start is associated with margin.  This is used in the process that maps from logical to physical properties.  Here, there's no corresponding offset property.  We'll have to special case that at the end of EnsurePhysicalProperty.

Is there a bug for re-writing the UA style sheets in terms of logical properties?  We'll need to stick CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS on all these properties given they're living behind a pref at the moment.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, this is 'block-size' and 'inline-size' and their min-* and max-* variants.

'offset-*' correspond to 'top', 'right', 'bottom', and 'left'.

Comment 3

[Cameron McCormack (:heycam)]

Oh, indeed.  So these might need a different mechanism to convert to physical properties than I have for the other logical properties we've converted so far, as these are two-property groups rather than four-property groups.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Comment 5

[Cameron McCormack (:heycam)]

Jonathan, apart from the margin, padding, border and size properties, are there any others required for the UA style sheet conversion?  And is there a bug for that this one can block?

Comment 6

[Jonathan Kew [:jfkthame]]

I've just filed bug 1119475 on the conversion. Offhand, I'm not aware of other properties required - but we'll find out in the course of going through the stylesheet files. Time to make a start on that...

Comment 7

[Jonathan Kew [:jfkthame]]

Hmm, I suppose we might find that we need logical equivalents of top/left/bottom/right, though the usage I've noticed so far in ua.css looks like we could get away without logicalising it. Ultimately, it seems like these ought to exist, though, even if we don't need them so urgently.

Comment 8

[Jean-Yves Perrier [:teoli]]

Quick question for (future) documentation: will these properties only be available in UA stylesheets?

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Jean-Yves Perrier [:teoli] from comment #8)
> Quick question for (future) documentation: will these properties only be
> available in UA stylesheets?

They will be available for content to use (though initially behind a pref).

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Part 1: Implement the {block,inline}-size logical properties.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Part 2: Implement the max-{block,inline}-size logical properties.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Part 3: Implement the min-{block,inline}-size logical properties.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 4: Tests.

Comment 14

[Cameron McCormack (:heycam)]

Try run for my logical properties branch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9bf2ce4d8c8c

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8547365 [details] [diff] [review]
Part 1: Implement the {block,inline}-size logical properties.

I'm not crazy about shipping in this state (i.e., with the hack
in nsRuleNode.cpp here).

We should probably have a bug on (a) adding the keywords to 'height' and
(b) removing the hack in nsRuleNode.cpp.  Would you mind filing that?
It depends both on bug 1113216 and on the resulting followup bug
requested in bug 1113216 comment 9.

I'm inclined to think it should block the bug on shipping vertical
text support, at least for now.  (Maybe not block shipping, but it should
block moving people off of vertical text to other things.)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8547366 [details] [diff] [review]
Part 2: Implement the max-{block,inline}-size logical properties.

... and that followup bug should mention this nsRuleNode.cpp hack too, and the one in the next patch

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8547366 [details] [diff] [review]
Part 2: Implement the max-{block,inline}-size logical properties.

Oh, and the logical group entry in nsCSSPropList.h should be MaxSize rather than Size.  And your tests should have caught that.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8547368 [details] [diff] [review]
Part 3: Implement the min-{block,inline}-size logical properties.

the logical group should be MinSize rather than Size, and you should fix the tests to catch that.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8547370 [details] [diff] [review]
Part 4: Tests.

>+  gAxisPropertyGroups = [];
>+  ["", "max-", "min-"].forEach(function(aPrefix) {
>+    gAxisPropertyGroups.push({
>+      horizontal: "width", vertical: "height",

I think you need to add aPrefix here.

>+      inline: `${aPrefix}inline-size`, block: `${aPrefix}block-size`,
>+      type: "length",
>+      prerequisites:
>+        make_declaration(gCSSProperties[`${aPrefix}height`].prerequisites)
>+    });
>+  });

(Please test that that makes things fail without the above changes, but pass with them!)


r=dbaron with that

Comment 20

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #15)
> I'm not crazy about shipping in this state (i.e., with the hack
> in nsRuleNode.cpp here).

Shipping with vertical text support enabled and with the hack present, or even with vertical text support disabled but the hack present?

> We should probably have a bug on (a) adding the keywords to 'height' and
> (b) removing the hack in nsRuleNode.cpp.  Would you mind filing that?
> It depends both on bug 1113216 and on the resulting followup bug
> requested in bug 1113216 comment 9.

Filed bug 1122253.

> I'm inclined to think it should block the bug on shipping vertical
> text support, at least for now.  (Maybe not block shipping, but it should
> block moving people off of vertical text to other things.)

I couldn't find a bug for shipping vertical text; I made it block bug 45503.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #20)
> (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from
> comment #15)
> > I'm not crazy about shipping in this state (i.e., with the hack
> > in nsRuleNode.cpp here).
> 
> Shipping with vertical text support enabled and with the hack present, or
> even with vertical text support disabled but the hack present?

I meant enabling the vertical text pref with the hack present.

> I couldn't find a bug for shipping vertical text; I made it block bug 45503.

Probably bug 1099032.

Comment 22

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #19)
> (Please test that that makes things fail without the above changes, but pass
> with them!)

Confirmed.

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6fd5b35b4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd33a6241a12
https://hg.mozilla.org/integration/mozilla-inbound/rev/844036800fe9
https://hg.mozilla.org/integration/mozilla-inbound/rev/63587fc445c2

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6a6fd5b35b4b
https://hg.mozilla.org/mozilla-central/rev/fd33a6241a12
https://hg.mozilla.org/mozilla-central/rev/844036800fe9
https://hg.mozilla.org/mozilla-central/rev/63587fc445c2

Comment 26

[Jean-Yves Perrier [:teoli]]

Hi!
In order to assess the documentation need here. Can you confirm/infirm if I understand well this bug? 

It creates: inline-size, block-size, max-inline-size, max-block-size, min-inline-size, and max-block-size.

They are behind a flag, layout.css.vertical-text.enabled (off by default for the moment), but available to any web content.

They aren't part of a Web standard.

Am I correct?

Comment 27

[Jean-Yves Perrier [:teoli]]

I just found this: http://dev.w3.org/csswg/css-logical-props/ My last assertion was incorrect.

Comment 28

[Cameron McCormack (:heycam)]

(In reply to Jean-Yves Perrier [:teoli] from comment #26)
> It creates: inline-size, block-size, max-inline-size, max-block-size,
> min-inline-size, and max-block-size.
> 
> They are behind a flag, layout.css.vertical-text.enabled (off by default for
> the moment), but available to any web content.

Yes that's right.

Comment 29

[Sebastian Zartner [:sebo]]

Described the six new properties:
https://developer.mozilla.org/en-US/docs/Web/CSS/block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/inline-size
https://developer.mozilla.org/en-US/docs/Web/CSS/min-block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/min-inline-size
https://developer.mozilla.org/en-US/docs/Web/CSS/max-block-size
https://developer.mozilla.org/en-US/docs/Web/CSS/max-inline-size

Sebastian

Comment 30

[Jean-Yves Perrier [:teoli]]

Also added a mention in https://developer.mozilla.org/en-US/Firefox/Releases/38#CSS