1319672 - Improve document related to writing-mode in nsFloatManager

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Per bug 1316549 comment 9, we should document aWM, mWritingMode, and perhaps CHECK_BLOCK_DIR usage in nsFloatManager.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1319672 Part 1 - Add document related to writing-mode in nsFloatManager.

Review commit: https://reviewboard.mozilla.org/r/95284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95284/

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1319672 Part 2 - Check the line direction is not changed.

Since the line direction affects the interpretation of float right or left,
we should check aWM has the same line direction as mWritingMode. And this
also guarantees that float added by AddFloat() has the same line direction
as when querying the available space via GetFlowArea().

Review commit: https://reviewboard.mozilla.org/r/95286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95286/

Comment 3

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

Comment on attachment 8813980 [details]
Bug 1319672 Part 1 - Add document related to writing-mode in nsFloatManager.

https://reviewboard.mozilla.org/r/95284/#review96236

::: layout/generic/nsFloatManager.h:48
(Diff revision 1)
>      , mHasFloats(aHasFloats) {}
>  };
>  
>  #define NS_FLOAT_MANAGER_CACHE_SIZE 4
>  
> +/**

Given that this is the comment describing the entire class, you should probably start with an additional first paragraph like:

nsFloatManager is responsible for implementing CSS's rules for positioning floats.  An nsFloatManager object is created during reflow for any block with NS_BLOCK_FLOAT_MGR.  During reflow, the float manager for the nearest such ancestor block is found in ReflowInput::mFloatManager.

::: layout/generic/nsFloatManager.h:49
(Diff revision 1)
>  };
>  
>  #define NS_FLOAT_MANAGER_CACHE_SIZE 4
>  
> +/**
> + * According to CSS Writing Modes Level 3 §7.5, line-right and line-left

Please link to https://drafts.csswg.org/css-writing-modes/#line-mappings rather than giving section numbers, since section numbers change.

::: layout/generic/nsFloatManager.h:50
(Diff revision 1)
>  
>  #define NS_FLOAT_MANAGER_CACHE_SIZE 4
>  
> +/**
> + * According to CSS Writing Modes Level 3 §7.5, line-right and line-left
> + * are calculated with respect to the writing-mode of the containing block

Throughout, this should be "writing mode" (the concept) rather than "writing-mode" (the CSS property).

::: layout/generic/nsFloatManager.h:51
(Diff revision 1)
>  #define NS_FLOAT_MANAGER_CACHE_SIZE 4
>  
> +/**
> + * According to CSS Writing Modes Level 3 §7.5, line-right and line-left
> + * are calculated with respect to the writing-mode of the containing block
> + * of the float element. All the writing-mode pass to nsFloatManager methods

"float element" -> "float"

::: layout/generic/nsFloatManager.h:51
(Diff revision 1)
>  #define NS_FLOAT_MANAGER_CACHE_SIZE 4
>  
> +/**
> + * According to CSS Writing Modes Level 3 §7.5, line-right and line-left
> + * are calculated with respect to the writing-mode of the containing block
> + * of the float element. All the writing-mode pass to nsFloatManager methods

"writing-mode pass" -> "writing modes passed"

::: layout/generic/nsFloatManager.h:54
(Diff revision 1)
> + * According to CSS Writing Modes Level 3 §7.5, line-right and line-left
> + * are calculated with respect to the writing-mode of the containing block
> + * of the float element. All the writing-mode pass to nsFloatManager methods
> + * should be the containing block's writing-mode.
> + *
> + * However, according to the abstract-to-physical mappings table in §6.4

Again, use a URL rather than a section number.

::: layout/generic/nsFloatManager.h:56
(Diff revision 1)
> + * interpretation of line-right and line-left. We actually implemented as
> + * passing in the writing-mode of the block formatting context (BFC), i.e.

"implemented as passing" -> "implement this by passing"

::: layout/generic/nsFloatManager.h:400
(Diff revision 1)
> +  // Store the WritingMode of ReflowInput from the block frame which
> +  // established the block formatting context (BFC) when nsFloatManager is
> +  // created.

I'm not sure what the ReflowInput has to do with this, so drop "of ReflowInput".  Also, "established" -> "establishes" and "when nsFloatManager" -> "when the nsFloatManager".

Comment 4

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

Comment on attachment 8813981 [details]
Bug 1319672 Part 2 - Check the line direction is not changed.

https://reviewboard.mozilla.org/r/95286/#review96240

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8813980 [details]
Bug 1319672 Part 1 - Add document related to writing-mode in nsFloatManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95284/diff/1-2/

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8813981 [details]
Bug 1319672 Part 2 - Check the line direction is not changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95286/diff/1-2/

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

All the issues in comment 3 have been addressed in patchset 2.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81eb4fbbb3ef
Part 1 - Add document related to writing-mode in nsFloatManager. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/cb5869f00c9f
Part 2 - Check the line direction is not changed. r=dbaron

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81eb4fbbb3ef
https://hg.mozilla.org/mozilla-central/rev/cb5869f00c9f