Ahmed Nefzaoui [:Nefzaoui] Reporter
	Description • 9 years ago

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

	Cameron McCormack (:heycam)
	Comment 1 • 9 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 2 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 3 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 4 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 8 • 9 years ago

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.

	Cameron McCormack (:heycam)
	Comment 9 • 9 years ago

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

	Cameron McCormack (:heycam)
	Comment 10 • 9 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 11 • 9 years ago

(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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 12 • 9 years ago

(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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 13 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 14 • 9 years ago

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

	Cameron McCormack (:heycam)
	Comment 15 • 9 years ago

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. :-)

	Cameron McCormack (:heycam)
	Comment 16 • 9 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 17 • 9 years ago

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

	fantasai
	Comment 18 • 9 years ago

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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 19 • 9 years ago

(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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 20 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 21 • 9 years ago

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).

	Jonathan Kew [:jfkthame] Assignee
	Comment 22 • 9 years ago

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...)

	Cameron McCormack (:heycam)
	Comment 23 • 9 years ago

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?

	Jonathan Kew [:jfkthame] Assignee
	Comment 24 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 25 • 9 years ago

(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.

	Jonathan Kew [:jfkthame] Assignee
	Comment 27 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 28 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 29 • 9 years ago
bugherder

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

	Jean-Yves Perrier [:teoli]
	Comment 30 • 9 years ago

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

	Jonathan Kew [:jfkthame] Assignee
	Comment 31 • 9 years ago

(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.

	Jean-Yves Perrier [:teoli]
	Comment 32 • 9 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 33 • 8 years ago

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?

	Jonathan Kew [:jfkthame] Assignee
	Comment 34 • 8 years ago

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