1145036 - Rename nsStyleTableBorder::mBorderSpacing{X,Y} to logical style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

We currently use mBorderSpacing{X,Y}, but the writing mode spec says:

The first and second values of the border-spacing property represent spacing between columns and rows respectively, not necessarily the horizontal and vertical spacing respectively. 

Hence, intend to rename them to mBorderSpacing{Col,Row}

I guess it makes sense to also rename nsTableFrame::GetCellSpacing{X,Y}, but I'm not sure how they should be renamed. Probably GetCell{I,B}Spacing, or Get{Col,Row}Spacing?

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - rename mBorderSpacingX/Y

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - rename GetCellSpacingX/Y

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8579941 [details] [diff] [review]
patch 1 - rename mBorderSpacingX/Y

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

::: layout/style/nsRuleNode.cpp
@@ +7906,5 @@
>                NS_STYLE_BORDER_SEPARATE, 0, 0, 0, 0);
>  
>    const nsCSSValue* borderSpacingValue = aRuleData->ValueForBorderSpacing();
>    if (borderSpacingValue->GetUnit() != eCSSUnit_Null) {
> +    // border-spacing-column/row: length, inherit

This comment shouldn't change, since the property names aren't changing.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8579941 [details] [diff] [review]
> patch 1 - rename mBorderSpacingX/Y
> 
> Review of attachment 8579941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsRuleNode.cpp
> @@ +7906,5 @@
> >                NS_STYLE_BORDER_SEPARATE, 0, 0, 0, 0);
> >  
> >    const nsCSSValue* borderSpacingValue = aRuleData->ValueForBorderSpacing();
> >    if (borderSpacingValue->GetUnit() != eCSSUnit_Null) {
> > +    // border-spacing-column/row: length, inherit
> 
> This comment shouldn't change, since the property names aren't changing.

I don't think we ever have properties called border-spacing-x/y.

Comment 5

[Cameron McCormack (:heycam)]

Ah, thought that we had split out properties, but both values just get set from border-spacing.  The comment should say "border-spacing: pair(length), inherit" then, to match other comments in the file.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd5e71db6003
https://hg.mozilla.org/mozilla-central/rev/778616c8f74e