Closed Bug 1117983 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jfkthame, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 1 obsolete file)

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?
Version: unspecified → Trunk
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
No, this is 'block-size' and 'inline-size' and their min-* and max-* variants.

'offset-*' correspond to 'top', 'right', 'bottom', and 'left'.
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.
Attached patch WIP v0.1 (obsolete) — Splinter Review
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?
Blocks: 1119475
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...
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.
Quick question for (future) documentation: will these properties only be available in UA stylesheets?
Keywords: dev-doc-needed
(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).
Depends on: 1120284
Attachment #8545169 - Attachment is obsolete: true
Attachment #8547365 - Flags: review?(dbaron)
Attached patch Part 4: Tests.Splinter Review
Attachment #8547370 - Flags: review?(dbaron)
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.)
Attachment #8547365 - Flags: review?(dbaron) → review+
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
Attachment #8547366 - Flags: review?(dbaron) → review+
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 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.
Attachment #8547368 - Flags: review?(dbaron) → review+
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
Attachment #8547370 - Flags: review?(dbaron) → review+
(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.
(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.
(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.
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?
Flags: needinfo?(cam)
I just found this: http://dev.w3.org/csswg/css-logical-props/ My last assertion was incorrect.
(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.
Flags: needinfo?(cam)
Depends on: 1131994
See Also: → 1132859
You need to log in before you can comment on or make changes to this bug.