1202993 - caption-side (logical) and vertical writing-modes

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Gérard Talbot]

Vendor-prefixed tests
---------------------

caption-side: top
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s74-caption-side-vrl-002.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s74-caption-side-vlr-003.xht

caption-side: bottom
http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s74-caption-side-vrl-004.xht

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/s74-caption-side-vlr-005.xht


Expected results
----------------

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/ref-filled-green-100px-square.xht


Notes
-----
- This bug report spun from comment 7 of bug 1193145
- More caption-side tests (left and right) are available
- Firefox 43.0a1 buildID=20150908030203 fails these 4 tests
- Chrome 47.0.2503.0 passes these 4 tests; IE11 passes the 2 'caption-side: top' tests and fails the 'caption-side: bottom' tests
- I use Linux 3.13.0-63-generic x86_64, Qt: 4.8.6, KDE 4.13.3; Kubuntu (trusty) 14.04.3 LTS

Comment 1

[Jonathan Kew [:jfkthame]]

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

Comment 2

[Jonathan Kew [:jfkthame]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

When we make this change, we'll need to update the doc at https://developer.mozilla.org/en-US/docs/Web/CSS/caption-side: the logical values will be removed, and we should add a note clarifying that the "physical" values (top/bottom/etc) are actually treated logically, according to the table's writing-mode.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8659171 [details] [diff] [review]
Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides

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

::: layout/generic/WritingModes.h
@@ +1136,5 @@
>    }
>  
> +  nscoord LineLeft(WritingMode aWritingMode) const
> +  {
> +    return aWritingMode.IsBidiLTR()

Please add documentation saying what these new functions are meant to do, and how they differ from the similarly-named Left() / Right() functions.  Their behavior is a bit confusing, particularly if our writing-mode is vertical.  (It's non-obvious what "LineLeft"/"LineRight" intrinsically mean in a vertical writing-mode, particularly in the presence of "direction: rtl". So much physical/logical language mixing. :-/  Ah, legacy CSS stuff.)

Also: It looks like these functions are missing CHECK_WRITING_MODE(...) assertions which are present as the first line of all the other accessors.  Please add those (or if we don't want them here, add a comment explaining why these functions want to allow mismatched WritingModes).

::: layout/reftests/writing-mode/tables/table-caption-left-1.html
@@ +62,5 @@
> +
> +<table class="vlr rtl">
> +  <caption>table caption</caption>
> +  <tr><td></td></tr>
> +</table>

Nit: Consider inserting this first new chunk up a tad higher, in your tweaks to these tests.

Right now, with your patch, this test (and table-caption-right-1.htm) ends up with the following order ('+' indicating the new parts added here):

  h ltr    _
  h rtl    -`same 'writing-mode', varying 'direction'
  vlr ltr  _
  vrl ltr  -`same 'direction', varying 'writing-mode'
+ vlr rtl  _
+ vrl rtl  -`same 'direction', varying 'writing-mode'

The inconsistency in which property we hold constant at which point in time seems a bit counterintuitive -- 
the test would be easier to follow (and easier to sanity-check that we're exercising all the possibilities) if we're consistent about how we work through the combinations.

So I'd suggest we instead use this ordering (just inserting your new "vlr rtl" table a bit higher up):
  h ltr    _
  h rtl    -`same 'writing-mode', varying 'direction'
  vlr ltr  _
+ vlr rtl  -`same 'writing-mode', varying 'direction'
  vrl ltr  _
+ vrl rtl  -`same 'writing-mode', varying 'direction'

::: layout/style/nsStyleConsts.h
@@ +908,5 @@
>  #define NS_STYLE_TABLE_EMPTY_CELLS_SHOW            1
>  
> +// Constants for the caption-side property. Note that despite having "physical"
> +// names, these are actually interpreted as logical sides according to the
> +// table's writing-mode, so "TOP" means block-start, etc.

Expanding on the "etc." here -- it's not clear what "LEFT" means.   Just looking at this comment, I'd assume it means "inline-start".  But I think instead it really means "either left or top, depending on whether our inline axis is horizontal or vertical".

That's probably worth calling out (or at least hinting at) here, because otherwise "etc" seems to imply that LEFT means inline-start.

@@ +912,5 @@
> +// table's writing-mode, so "TOP" means block-start, etc.
> +#define NS_STYLE_CAPTION_SIDE_TOP               0
> +#define NS_STYLE_CAPTION_SIDE_BOTTOM            1
> +#define NS_STYLE_CAPTION_SIDE_LEFT              2
> +#define NS_STYLE_CAPTION_SIDE_RIGHT             3

Nit: For consistency, it's probably best to leave these lines (and numerical values) with the standard CSS ordering of "top, right, bottom, left".  That's how they're ordered before this change, and it's also how they were ordered before we introduced logical keywords, as shown here:
 http://hg.mozilla.org/mozilla-central/annotate/8422e49e2ab3/layout/style/nsStyleConsts.h#l909

(and it's how most other constants with these names are ordered as well. So, best to continue matching that order here, unless we somehow depend on e.g. left & right being adjacent numerically. [I don't think we do? Maybe it makes the 'switch' tables simpler for the compiler to optimize? not sure.])

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

> Please add documentation saying what these new functions are meant to do,
> and how they differ from the similarly-named Left() / Right() functions. 
> Their behavior is a bit confusing, particularly if our writing-mode is
> vertical.  (It's non-obvious what "LineLeft"/"LineRight" intrinsically mean
> in a vertical writing-mode, particularly in the presence of "direction:
> rtl". So much physical/logical language mixing. :-/  Ah, legacy CSS stuff.)

line-left and line-right are defined by CSS Writing Modes, http://www.w3.org/TR/css-writing-modes-3/#line-directions. E.g. LineLeft is the side where bidi-LTR text would start; it will be top in vertical writing modes (regardless of the actual bidi dir in effect), except in sideways-left mode where it will be bottom. I've added a comment.

> Also: It looks like these functions are missing CHECK_WRITING_MODE(...)
> assertions

They call through to IStart/IEnd accessors that will do the check.


> ::: layout/style/nsStyleConsts.h
> @@ +908,5 @@
> >  #define NS_STYLE_TABLE_EMPTY_CELLS_SHOW            1
> >  
> > +// Constants for the caption-side property. Note that despite having "physical"
> > +// names, these are actually interpreted as logical sides according to the
> > +// table's writing-mode, so "TOP" means block-start, etc.
> 
> Expanding on the "etc." here -- it's not clear what "LEFT" means.   Just
> looking at this comment, I'd assume it means "inline-start".  But I think
> instead it really means "either left or top, depending on whether our inline
> axis is horizontal or vertical".

It means line-left, which will currently be "either left or top, depending on whether our inline axis is horizontal or vertical", but can also become bottom once we add sideways-left support. (This is by analogy with how float:left / float:right behave in vertical writing modes.) Comment added.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c32abb6c9e65860e4f12e5dbc4668b36779ee8fd
Bug 1202993 - Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides. r=dholbert

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c32abb6c9e65

Comment 10

[Jean-Yves Perrier [:teoli]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> When we make this change, we'll need to update the doc at
> https://developer.mozilla.org/en-US/docs/Web/CSS/caption-side: the logical
> values will be removed, and we should add a note clarifying that the
> "physical" values (top/bottom/etc) are actually treated logically, according
> to the table's writing-mode.

Am I correct to assume that the CSS Logical Properties spec has not yet been updated to reflect this? https://drafts.csswg.org/css-logical-props/#caption-side

(Or am I missing something here?)

Comment 11

[Jonathan Kew [:jfkthame]]

Yes, it looks like that hasn't been updated yet. The CSS WG just made this decision last month[1], so I expect the editors will be updating the spec in the fairly near future.

[1] https://lists.w3.org/Archives/Public/www-style/2015Sep/0113.html

Comment 12

[Jean-Yves Perrier [:teoli]]

Ok, let's see if I've understood here. (Sorry for my slow brain)

The CSS 2.1 defines the following value for caption-side:
* top
* bottom
Gecko implemented for ages (if not ever)

The CSS 2 spec, but not CSS 2.1, also defined:
* left
* right
Gecko implemented this (unprefixed) for ages (if not ever). (They are technically non-standard, but were on the standard track when implemented)

Gecko also implements these non-standard values:
* top-outside
* bottom-outside
They are CSS 2 variants of the CSS 2.1 top and bottom (different algorithm)

In Firefox 42 (bug 1177076, currently in Beta), Gecko added the logical counter-parts of all these:
* block-end
* block-end-outside
* block-start
* block-start-outside
* inline-end
* inline-start
Gecko also changed caption-side's initial value from top to block-start

In this bug (that will be in Firefox 43) we revert part of bug 1177076: block-(start|end)[-outside], inline-(start|end) as well as the caption-side's initial value (back to top).
(We also made top and bottom relative to the writing-mode of the table).

Is this correct?

A last question: does this mean that the experimental behavior removed in this bug will be in Firefox 42 Release? Or will the patches be uplifted?

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Jean-Yves Perrier [:teoli] from comment #12)
> Ok, let's see if I've understood here. (Sorry for my slow brain)
> 
> The CSS 2.1 defines the following value for caption-side:
> * top
> * bottom
> Gecko implemented for ages (if not ever)
> 
> The CSS 2 spec, but not CSS 2.1, also defined:
> * left
> * right
> Gecko implemented this (unprefixed) for ages (if not ever). (They are
> technically non-standard, but were on the standard track when implemented)
> 
> Gecko also implements these non-standard values:
> * top-outside
> * bottom-outside
> They are CSS 2 variants of the CSS 2.1 top and bottom (different algorithm)
> 
> In Firefox 42 (bug 1177076, currently in Beta), Gecko added the logical
> counter-parts of all these:
> * block-end
> * block-end-outside
> * block-start
> * block-start-outside
> * inline-end
> * inline-start
> Gecko also changed caption-side's initial value from top to block-start

Correct.

> In this bug (that will be in Firefox 43) we revert part of bug 1177076:
> block-(start|end)[-outside], inline-(start|end) as well as the
> caption-side's initial value (back to top).
> (We also made top and bottom relative to the writing-mode of the table).

In this bug, we removed all 6 of the logical values; reverted the initial value; and made all 6 of the old caption-side values -- including the *-outside versions of top/bottom, and the "side caption" values left/right -- be interpreted relative to the writing-mode of the table, so top/bottom are mapped to block-start/block-end, and left/right are mapped to line-left/line-right.

> A last question: does this mean that the experimental behavior removed in
> this bug will be in Firefox 42 Release? Or will the patches be uplifted?

I wasn't planning to uplift, so we'll have one release (42) where the logical keywords are supported, before switching to the new behavior in 43. I don't think that matters much; AFAIK we were the only browser to get around to implementing the logical keywords before the CSS WG changed its mind, so it's unlikely there will be any significant content that tries to use them.

Do you think it'd be better to uplift the patch here, so that the logical values don't get shipped on the release channel at all? I'm not opposed to requesting uplift -- I think the patch is pretty low-risk -- if that seems worthwhile to you.

Comment 14

[Jean-Yves Perrier [:teoli]]

Yes: from a documentation perspective, if a specific, experimental and removed behaviour never reach a released version, we can simply remove all mention to it. It is has been released, we have to keep at least a note about it in the compat data info and in the notes for this version.

So an uplift to Fx 42 would remove the specific behaviour for this version, and simplify documentation. The abandoned keywords will just disappear from MDN.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8659171 [details] [diff] [review]
Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides

Approval Request Comment
[Feature/regressing bug #]: In bug 1177076 (landed for gecko42), we made changes to CSS 'caption-side' that were not strictly a regression, but since we implemented that, the Working Group resolved to change the model for 'caption-side' values.

[User impact if declined]: Firefox 42 will implement a model for CSS 'caption-side' using new logical-direction properties that has since been abandoned and will be withdrawn in Firefox 43; this complicates documentation and risks confusion for authors.

[Describe test coverage new/current, TreeHerder]: This feature is covered by existing reftests, which are updated in the patch to match the changed set of CSS values.

[Risks and why]: Although the patch looks big, a good deal of that is updating the related reftests; and the code changes do not involve new/untested behavior, they are basically rearranging the mapping of CSS values to the various behavior options already present in the layout code. Hence, I think the risk here is pretty low, and should be safe to take early in Beta.

[String/UUID change made/needed]: n/a

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8659171 [details] [diff] [review]
Remove the logical-direction-based keywords for caption-side, and instead interpret the old physical keywords as logical sides

OK. Make sense, we don't want 42 to be a special version for webdev.
Should be in 42 beta 2

Comment 17

[Jean-Yves Perrier [:teoli]]

Many thanks!

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/caption-side
and
https://developer.mozilla.org/en-US/Firefox/Releases/42

Comment 18

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8839d7e9632

Comment 19

[Gérard Talbot]

I have updated

http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/CaptionSide-writing-modes-dhtml.html

to reflect all 50 possibilities of positioning a table caption under the current specifications.

Comment 20

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 43.0a1 (Build ID: 20150908030203) on 
windows 8.1 64-bit with the instructions from comment 0.

Verified as fixed with Firefox Aurora 43.0b7 (Build ID: 20151126120800)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0