Closed Bug 1155772 Opened 10 years ago Closed 5 years ago

Convert flex item margin/border/padding computations to use logical axes

Categories

(Core :: Layout: Flexbox, defect)

Product:
Core
Component:
Layout: Flexbox
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Tracking Flags:
Tracking Status
firefox40 --- wontfix
firefox74 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(16 files)

Bug 1155772 Part 1 - Move delete methods to public section for various flex classes. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 2 - Add "Physical" to main/cross axis accessor and variable names in FlexboxAxisTracker (idempotent patch). r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 3 - Add logical main/cross axis accessors and variables to FlexboxAxisTracker. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 4 - Add "Physical" to the axis accessor and variable names in PositionTracker (idempotent patch). r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 5 - Cache flex container's logical axis and writing-mode in PositionTracker. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 7 - Convert FlexItem::mBorderPadding from nsMargin to LogicalMargin. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 8 - Rename GetMargin() to GetPhysicalMargin() in FlexItem (idempotent patch). r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 9 - Convert FlexItem::mMargin to LogicalMargin. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 10 - Remove unused AxisOrientationType argument. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 11 - Convert FlexItem::GetNumAutoMarginsInAxis() to use logical axis. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 12 - Use LogicalMargin for EnterMargin / ExitMargin in PositionTracker. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 13 - Add start side and end side accessors in FlexboxAxisTracker, and use them to replace AxisOrientationType usages. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 14 - Remove physical axis usage from GetBaselineOffsetFromOuterCrossEdge(). r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 15 - Remove mPhysicalAxis from PositionTracker. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1155772 Part 16 - Remove mPhysicalMainAxis and mPhysicalAxis from FlexboxAxisTracker, and all the other helper enums and functions. r=mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1152913 left behind physical axes in otherwise-logical structs FlexboxAxisTracker & PositionTracker, for use in some margin/border/padding computations. I'm filing this bug on converting these computations to use logical axes & LogicalMargin, so that we don't need to know the physical axes in these structs anymore.

I look at this a bit, I think we'll need to

  1. Change FlexItem::{mBorderPadding,mMargin} to store in LogicalMargin. I'm not sure which writing-mode we should use to store the LogicalMargin . It's straitforward to use flex item's writing-mode because it is from ReflowInput, but flex algorithm seems need to use flex container's writing-mode most of the cases.
  2. Eliminate AxisOrientationType and FlexboxAxisTracker::{GetMainAxis(),GetCrossAxis()}, and use FlexboxAxisTracker::{IsRowOriented(),IsColumnOriented} instead.
  3. Search for XXXdholbert [BEGIN DEPRECATED], and delete those code.

Did I miss anything?

Flags: needinfo?(dholbert)

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) (Traveling Nov 23 - Dec 15) from comment #1)

I look at this a bit, I think we'll need to

  1. Change FlexItem::{mBorderPadding,mMargin} to store in LogicalMargin. I'm not sure which writing-mode we should use to store the LogicalMargin . It's straitforward to use flex item's writing-mode because it is from ReflowInput, but flex algorithm seems need to use flex container's writing-mode most of the cases.

Yeah, we should pick whichever is more convenient in terms of usage. I suspect the flex container's WM is the more commonly-needed one. This will involve minor tweaks/rewriting of a bunch of small APIs, I think, but hopefully they're all pretty straightforward.

  1. Eliminate AxisOrientationType and FlexboxAxisTracker::{GetMainAxis(),GetCrossAxis()}, and use `FlexboxAxisTracker::{IsRowOriented(),IsColumnOriented} instead.

(Also the kAxisOrientationToSidesMap struct, which is indexed in part by AxisOrientationType and is currently only used for physical-margin/border-side related stuff, I believe. Probably all/most of its usages will be removed in (1).)

  1. Search for XXXdholbert [BEGIN DEPRECATED], and delete those code.

Yeah -- I think that should cover it for this bug! Thanks for taking a look.

Flags: needinfo?(dholbert)
Depends on: 1601298

Fix "warning: deleted member function should be public", which can be
reproduced by
./mach static-analysis check layout/generic/nsFlexContainerFrame.cpp

If a private delete method is called, the compiler shows two error
messages: the method is deleted, and a private methods is called. By
declaring the method as public, only the former message is shown, which
should be more readable.

Assignee: nobody → aethanyc
Status: NEW → ASSIGNED

I'm going to add logical accessors and variables in the next patch, so
it's better the rename the old ones first.

Depends on D59043

This includes renaming aAxis to aPhysicalAxis in PositionTracker's
constructor.

Depends on D59045

They are going to be used in the later parts.

Depends on D59046

These new added member variables are going to be used in the following
parts that convert border/padding/margin to use logical layout, so they
are added to the place prior to mBorderPadding and mMargin.

Also, move flex item's own writing-mode mWM for readability.

For "strut" constructor, we can use the default values for mMainAxis,
mCrossAxis, and mIsInlineAxisMainAxis initialized in the class
declaration.

Depends on D59047

Also delete unused GetBorderPadding().

Depends on D59048

Also, use GetPhysicalMargin() instead of mMargin in
FlexItem::GetBaselineOffsetFromOuterCrossEdge() to ease the transition
when converting FlexItem::mMargin to LogicalMargin in the later parts.

Depends on D59049

Note that I return LogicalMargin by value for GetMargin(). It is
considered a suboptimal practice to return a constant reference to a
member variable. We won't gain performance by returning a reference
because the compiler can optimize the return value. If it were assigned
to a new variable, the copy/move constructor is called anyway.

It can also break encapsulation if the caller removes the const
deliberately by something like

    const_cast<LogicalMargin&>(item->GetMargin()) = LogicalMargin(...);

As noted in FIXME(TYLin), the AxisOrientationType argument in
GetMarginBorderPaddingSizeInMainAxis() and many other methods can be
removed. However, to make the patch size manageable, I'll remove them in
a later part.

Depends on D59050

Also, remove unused FlexboxAxisTracker argument from
ResolveStretchedCrossSize().

Depends on D59052

The original physical axis AxisOrientationType contains direction
information, which can be replaced by physical start side
straightforwardly.

Depends on D59054

Attachment #9119260 - Attachment description: Bug 1155772 Part 6 - Cache flex container's writing-mode, main / cross axis in FlexItem. r=mats → Bug 1155772 Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats
Blocks: 1610057
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7677362c44b3 Part 1 - Move delete methods to public section for various flex classes. r=dholbert https://hg.mozilla.org/integration/autoland/rev/11f020664664 Part 2 - Add "Physical" to main/cross axis accessor and variable names in FlexboxAxisTracker (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/ce108df4d23c Part 3 - Add logical main/cross axis accessors and variables to FlexboxAxisTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/abc7f4471cbf Part 4 - Add "Physical" to the axis accessor and variable names in PositionTracker (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/b3b3dfa8ac4e Part 5 - Cache flex container's logical axis and writing-mode in PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/32f209c8856c Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats https://hg.mozilla.org/integration/autoland/rev/83da4fc71357 Part 7 - Convert FlexItem::mBorderPadding from nsMargin to LogicalMargin. r=mats https://hg.mozilla.org/integration/autoland/rev/7c92f5b6d190 Part 8 - Rename GetMargin() to GetPhysicalMargin() in FlexItem (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/981e290de2af Part 9 - Convert FlexItem::mMargin to LogicalMargin. r=mats https://hg.mozilla.org/integration/autoland/rev/6cc7c04bf567 Part 10 - Remove unused AxisOrientationType argument. r=mats https://hg.mozilla.org/integration/autoland/rev/a5cb718ed15d Part 11 - Convert FlexItem::GetNumAutoMarginsInAxis() to use logical axis. r=mats https://hg.mozilla.org/integration/autoland/rev/ad2edd6baa2f Part 12 - Use LogicalMargin for EnterMargin / ExitMargin in PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/3cd8558625a2 Part 13 - Add start side and end side accessors in FlexboxAxisTracker, and use them to replace AxisOrientationType usages. r=mats https://hg.mozilla.org/integration/autoland/rev/1e9c57134aae Part 14 - Remove physical axis usage from GetBaselineOffsetFromOuterCrossEdge(). r=mats https://hg.mozilla.org/integration/autoland/rev/2a2e5970cf88 Part 15 - Remove mPhysicalAxis from PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/21ab70ca4496 Part 16 - Remove mPhysicalMainAxis and mPhysicalAxis from FlexboxAxisTracker, and all the other helper enums and functions. r=mats

Backed out for bustages on WritingModes.h

backout: https://hg.mozilla.org/integration/autoland/rev/5914beb11d5c24fc83bc1e2bfb28281b42facf9f

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=285822980&revision=21ab70ca44968f47755aa1bcf6d309d927c43376&searchStr=build&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285822980&repo=autoland&lineNumber=43427

[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - In file included from /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:59:0,
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - from /builds/worker/workspace/build/src/layout/generic/ScrollbarActivity.cpp:11,
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - from Unified_cpp_layout_generic1.cpp:11:
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h: In member function 'nscoord& mozilla::LogicalMargin::Side(mozilla::LogicalSide, mozilla::WritingMode)':
[task 2020-01-21T20:15:07.759Z] 20:15:07 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h:1253:3: error: control reaches end of non-void function [-Werror=return-type]
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - }
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - ^
[task 2020-01-21T20:15:07.759Z] 20:15:07 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h: In member function 'nscoord mozilla::LogicalMargin::Side(mozilla::LogicalSide, mozilla::WritingMode) const':
[task 2020-01-21T20:15:07.759Z] 20:15:07 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/WritingModes.h:1241:3: error: control reaches end of non-void function [-Werror=return-type]

Flags: needinfo?(aethanyc)

OK. This is a build failure for warnings on only g++. For a code snippet like the following, clang is smart enough to know that we have handled all the cases in LogicalSide enum in the switch-statement, so the code flow never reaches after the switch, but g++ does not aware of this.

  nscoord Side(LogicalSide aSide, WritingMode aWM) const {
    switch (aSide) {
      case eLogicalSideBStart:
        return BStart(aWM);
      case eLogicalSideBEnd:
        return BEnd(aWM);
      case eLogicalSideIStart:
        return IStart(aWM);
      case eLogicalSideIEnd:
        return IEnd(aWM);
    }

Both Part 9 & Part 13 contain such switch statements that can trigger this kind of warnings.

I'll add an assertion and dumb return value at the end of the function.

    MOZ_ASSERT_UNREACHABLE("We should handle all sides!");
    return BStart(aWM);
Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8503dcdda2c4 Part 1 - Move delete methods to public section for various flex classes. r=dholbert https://hg.mozilla.org/integration/autoland/rev/c887483fa25b Part 2 - Add "Physical" to main/cross axis accessor and variable names in FlexboxAxisTracker (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/075afaa25ee6 Part 3 - Add logical main/cross axis accessors and variables to FlexboxAxisTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/e42d916c6011 Part 4 - Add "Physical" to the axis accessor and variable names in PositionTracker (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/fd73bbfddf8c Part 5 - Cache flex container's logical axis and writing-mode in PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/2babcfb213c5 Part 6 - Cache flex container's writing-mode and main axis in FlexItem. r=mats https://hg.mozilla.org/integration/autoland/rev/bfd7c1c2accc Part 7 - Convert FlexItem::mBorderPadding from nsMargin to LogicalMargin. r=mats https://hg.mozilla.org/integration/autoland/rev/ea81b8962a19 Part 8 - Rename GetMargin() to GetPhysicalMargin() in FlexItem (idempotent patch). r=mats https://hg.mozilla.org/integration/autoland/rev/d03d935a66df Part 9 - Convert FlexItem::mMargin to LogicalMargin. r=mats https://hg.mozilla.org/integration/autoland/rev/d4e98d652bc3 Part 10 - Remove unused AxisOrientationType argument. r=mats https://hg.mozilla.org/integration/autoland/rev/54e1f82768fd Part 11 - Convert FlexItem::GetNumAutoMarginsInAxis() to use logical axis. r=mats https://hg.mozilla.org/integration/autoland/rev/ba772f194d56 Part 12 - Use LogicalMargin for EnterMargin / ExitMargin in PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/0e8361f9069e Part 13 - Add start side and end side accessors in FlexboxAxisTracker, and use them to replace AxisOrientationType usages. r=mats https://hg.mozilla.org/integration/autoland/rev/615c3a987954 Part 14 - Remove physical axis usage from GetBaselineOffsetFromOuterCrossEdge(). r=mats https://hg.mozilla.org/integration/autoland/rev/38150b46f038 Part 15 - Remove mPhysicalAxis from PositionTracker. r=mats https://hg.mozilla.org/integration/autoland/rev/d3fd7085836b Part 16 - Remove mPhysicalMainAxis and mPhysicalAxis from FlexboxAxisTracker, and all the other helper enums and functions. r=mats
Regressions: 1613380
Component: Layout → Layout: Flexbox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: