1122918 - implement logical keywords (inline-start, inline-end) for 'float' and 'clear' properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ahmed Nefzaoui [:Nefzaoui]]

I work on adding RTL support for Firefox OS, the usual way for this is overriding original properties' values, and even though we have -moz-margin-start|end, -moz-padding-start|end, text-align: start|end, we still have to write lots of manual code to override LTR-specific properties.
The aim of this bug is to gather feedback on the idea of implementing start|end values for the float property making gecko a little smarter about LTR/RTL, and piloting their use in B2G.

http://dev.w3.org/csswg/css-box/#the-float-property

Comment 1

[Cameron McCormack (:heycam)]

I think http://dev.w3.org/csswg/css-logical-props/#float-clear is newer than css-box, so we should maybe follow the naming of the new values in there -- inline-start and inline-end.

Comment 2

[Jonathan Kew [:jfkthame]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

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

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for inline-start/end values of the 'float' property

Here's a basic reftest (that currently fails, obviously) for float:inline-start and inline-end.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Part 1 - Add parsing for logical inline-start/end keywords to the 'float' and 'clear' properties

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Part 2 - Provide accessors that return logical 'float' and 'clear' values resolved to their physical equivalents

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Part 3 - Use the resolved values of 'float' and 'clear' properties during layout

Comment 8

[Jonathan Kew [:jfkthame]]

Along with 'float' we should also handle 'clear'; the patches above aim to do both of these. The intention is to fold them together for landing, but I figured they'd make sense to review as separate stages of implementation.

Comment 9

[Cameron McCormack (:heycam)]

Does |float: inline-{start,end}| work for vertical text?

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8661405 [details] [diff] [review]
Part 2 - Provide accessors that return logical 'float' and 'clear' values resolved to their physical equivalents

Review of attachment 8661405 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.h
@@ +2319,5 @@
> +
> +  // Return the 'float' and 'clear' properties, with inline-{start,end} values
> +  // resolved to {left,right} according to the given writing mode. These are
> +  // defined in WritingModes.h.
> +  inline uint8_t ResolvedFloats(mozilla::WritingMode aWM) const;

Since we have nsStyleTableBorder::LogicalCaptionSide, would it make sense to call these two methods PhysicalFloats and PhysicalBreakType?  Either way.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Does |float: inline-{start,end}| work for vertical text?

It should, though I admit I didn't test it yet.....

(In reply to Cameron McCormack (:heycam) from comment #10)

> Since we have nsStyleTableBorder::LogicalCaptionSide, would it make sense to
> call these two methods PhysicalFloats and PhysicalBreakType?  Either way.

Yes, I think I like that better. I wasn't very keen on the Resolved... names anyway.

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> (In reply to Cameron McCormack (:heycam) from comment #9)
> > Does |float: inline-{start,end}| work for vertical text?
> 
> It should, though I admit I didn't test it yet.....

Yes, it seems to behave as expected. I'll add a vertical version of the reftest for completeness.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for inline-start/end values of the 'float' property

Added a vertical writing-mode version of the reftest; carrying over r=heycam.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Add reftests using clear:inline-{start,end} values

We should also have a test that uses the inline-{start,end} values for the 'clear' property; something like this.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8661406 [details] [diff] [review]
Part 3 - Use the resolved values of 'float' and 'clear' properties during layout

Review of attachment 8661406 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with "Resolved" changed to "Physical".

::: layout/generic/nsFrame.cpp
@@ +4158,5 @@
> +        }
> +      }
> +
> +      uint8_t floatStyle = floatDisp->ResolvedFloats(wm);
> +      nscoord &floats_cur = floatStyle == NS_STYLE_FLOAT_LEFT

Feel free to move that ampersand across to the type while touching this line. :-)

Comment 16

[Cameron McCormack (:heycam)]

Before we land this, are we sure these keywords are stable enough?  The spec has an issue about whether they should be inline-start/inline-end or just start/end.  I think they'll want to be inline-start/inline-end, given float will probably have other values for floating to the top/bottom of pages etc., but it might be good to confirm with fantasai.

Comment 17

[Jonathan Kew [:jfkthame]]

Elika, what's your take on comment 16 ... are the inline-{start,end} values stable enough for us to proceed here?

Comment 18

[fantasai]

My main concern is the issue marked in the spec -- 1D properties like 'text-align' use the unprefixed keywords 'start' and 'end'.

A related thing is that 'float' needs clarification on whether it uses its writing mode or its containing block's writing mode to map the directions. Writing Modes specifies using the containing block's writing mode (which is more correct, generally), though that does create an inconsistency with the way margin-* is handled. (margin-* being limited by not complicating the cascading mechanisms)

I'm afraid I don't have a solid answer for you here. Your thoughts on the matter are welcome. I think I'd have to go through the whole spec and see the whole picture before having an informed opinion.

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to fantasai from comment #18)
> My main concern is the issue marked in the spec -- 1D properties like
> 'text-align' use the unprefixed keywords 'start' and 'end'.

True; but ISTM that 'float' could reasonably be extended in the future to become two-dimensional, whereas 'text-align' is meaningful only along the inline-progression axis of a single line of text, so it is inherently one-dimensional. (I suppose 'text-align' *could* have been two-dimensional, but the other axis is handled by 'vertical-align' instead.)

> A related thing is that 'float' needs clarification on whether it uses its
> writing mode or its containing block's writing mode to map the directions.
> Writing Modes specifies using the containing block's writing mode (which is
> more correct, generally), though that does create an inconsistency with the
> way margin-* is handled. (margin-* being limited by not complicating the
> cascading mechanisms)

I was assuming it would use the containing block's writing mode (and that's what the patches here implement). IMO this is by far the more reasonable/expected behavior. The 'float' value is about where the object is placed in relation to its container, so logical directions are those of the container; changing 'writing-mode' or 'direction' of the float should affect the layout of its contents, but not its placement within its container.

I'll post something to this effect on www-style to see if anyone else wants to offer an opinion.

Comment 20

[Jonathan Kew [:jfkthame]]

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

Comment 21

[Jonathan Kew [:jfkthame]]

Just a status update: I'm holding off on landing these patches, pending a decision by the CSS WG on the names of the logical values here. This is due to be discussed at TPAC (last week of Oct), following which I hope to land these patches (with any necessary adjustment to the names).

Comment 22

[Jonathan Kew [:jfkthame]]

How about if we land the patches here with the new keywords behind an

  #if defined(NIGHTLY_BUILD) || defined(MOZ_B2G)

condition, so that they're not available in release builds (except in FxOS, so that Gaia can start to use them)? That would limit the exposure of these not-yet-stable names to general Web content.

(I was going to suggest we prefix them, but that doesn't really help because the uncertainty relates to the names of the eventual unprefixed values. So I don't know what to prefix...)

Comment 23

[Cameron McCormack (:heycam)]

Would defined(MOZ_B2G) make these values available to content in the B2G Browser app?  Is there a way we can limit these values just to B2G apps?

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) from comment #23)
> Would defined(MOZ_B2G) make these values available to content in the B2G
> Browser app?

Yes, AFAIK they'd be exposed to everything on B2G. Which is not ideal, but I figure that as long as it's only the B2G browser (not Firefox on desktop or android, nor any other browser) there's not much temptation for authors to start using them in general web content.

>  Is there a way we can limit these values just to B2G apps?

I don't know any reasonable way to do that, without adding some ugly conditional code at the CSS parsing stage.

Comment 25

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

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> How about if we land the patches here with the new keywords behind an
> 
>   #if defined(NIGHTLY_BUILD) || defined(MOZ_B2G)
> 
> condition, so that they're not available in release builds (except in FxOS,
> so that Gaia can start to use them)? That would limit the exposure of these
> not-yet-stable names to general Web content.

Sounds fine to me.

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: Put the logical values for 'float' and 'clear' behind a pref, and enable them only on nightly builds and for B2G

Comment 27

[Jonathan Kew [:jfkthame]]

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

Comment 28

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/271a3adf1445a5b15ee730640c245b3bd025ec80
Bug 1122918 - Reftest for inline-start/end values of the 'float' property. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/30cd0e3e48db3eebc4fdff55326e30f0a807f871
Bug 1122918 - Part 1 - Add parsing for logical inline-start/end keywords to the 'float' and 'clear' properties. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/89caf161d603834336f497497b86a8fc336b3283
Bug 1122918 - Part 2 - Provide accessors that return logical 'float' and 'clear' values resolved to their physical equivalents. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab34b9a34e4b9bd6811c5685b9fc7fc1f417c03
Bug 1122918 - Part 3 - Use the resolved physical values of 'float' and 'clear' properties during layout. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1fc3b32d69f4affc07b81c29af6b9cb19d44f06
Bug 1122918 - Add reftests using clear:inline-{start,end} values. r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/66862e989b01a262f43f7ea84c8b18503a09dc55
Bug 1122918 - Put the logical values for 'float' and 'clear' behind a pref, and enable them only on nightly builds and for B2G. r=heycam

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/271a3adf1445
https://hg.mozilla.org/mozilla-central/rev/30cd0e3e48db
https://hg.mozilla.org/mozilla-central/rev/89caf161d603
https://hg.mozilla.org/mozilla-central/rev/6ab34b9a34e4
https://hg.mozilla.org/mozilla-central/rev/a1fc3b32d69f
https://hg.mozilla.org/mozilla-central/rev/66862e989b01

Comment 30

[Jean-Yves Perrier [:teoli]]

Docs updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/float
https://developer.mozilla.org/en-US/docs/Web/CSS/clear
and
https://developer.mozilla.org/en-US/Firefox/Releases/45#CSS

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to Jean-Yves Perrier [:teoli] from comment #30)
> Docs updated:
> https://developer.mozilla.org/en-US/docs/Web/CSS/float
> https://developer.mozilla.org/en-US/docs/Web/CSS/clear
> and
> https://developer.mozilla.org/en-US/Firefox/Releases/45#CSS

The docs should probably indicate somehow that the new keywords are behind a pref "layout.css.float-logical-values.enabled", and currently (for FF45) they'll be enabled by default only for Nightly/Aurora builds, not for Beta/Release (except on B2G, where they'll be enabled in all builds). See comment 22 and following.

Comment 32

[Jean-Yves Perrier [:teoli]]

Argh, I missed it. Good catch. I added a comment in the compat table of these pages. Thx.

Comment 33

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

Is there a bug on enabling these?  If not, could you file one and make it depend on this one, and on anything else that needs to happen before enabling?

Comment 34

[Jonathan Kew [:jfkthame]]

The only impediment (afaik) to enabling these for release channels is the question of whether the spec is sufficiently stable. I filed bug 1253919.