Closed Bug 1122918 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: nefzaoui, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 1 obsolete file)

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
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.
Summary: Consider implementing float: start|end for a better RTL support → implement logical keywords (inline-start, inline-end) for float property
Here's a basic reftest (that currently fails, obviously) for float:inline-start and inline-end.
Attachment #8661403 - Flags: review?(cam)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Summary: implement logical keywords (inline-start, inline-end) for float property → implement logical keywords (inline-start, inline-end) for 'float' and 'clear' properties
Does |float: inline-{start,end}| work for vertical text?
Attachment #8661403 - Flags: review?(cam) → review+
Attachment #8661404 - Flags: review?(cam) → review+
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.
Attachment #8661405 - Flags: review?(cam) → review+
(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.
(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.
Added a vertical writing-mode version of the reftest; carrying over r=heycam.
Attachment #8661403 - Attachment is obsolete: true
We should also have a test that uses the inline-{start,end} values for the 'clear' property; something like this.
Attachment #8661708 - Flags: review?(cam)
Attachment #8661708 - Flags: review?(cam) → review+
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. :-)
Attachment #8661406 - Flags: review?(cam) → review+
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.
Elika, what's your take on comment 16 ... are the inline-{start,end} values stable enough for us to proceed here?
Flags: needinfo?(fantasai.bugs)
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.
Flags: needinfo?(fantasai.bugs)
(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.
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).
Blocks: 1213774
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...)
Flags: needinfo?(cam)
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?
Flags: needinfo?(cam) → needinfo?(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.
Flags: needinfo?(jfkthame)
(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.
Attachment #8684859 - Flags: review?(cam) → review+
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
(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.
Flags: needinfo?(jypenator)
Argh, I missed it. Good catch. I added a comment in the compat table of these pages. Thx.
Flags: needinfo?(jypenator)
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?
Flags: needinfo?(jfkthame)
Blocks: 1253919
The only impediment (afaik) to enabling these for release channels is the question of whether the spec is sufficiently stable. I filed bug 1253919.
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.