New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 727173 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug
Launch-Accessibility: NA
Launch-Legal: NA
Launch-Privacy: NA
Launch-Security: NA
Launch-Test: NA
Launch-UI: NA

Blocking:
issue 128227



Sign in to add a comment

"direction" css property or "dir" html attribute of table section and rows should not affect cell order

Project Member Reported by wangxianzhu@chromium.org, May 29 2017

Issue description

Support of mixed direction of table/section/row causes much complexity in our code. We also have many bugs and inconsistencies. It also causes some situations difficult in both reasonable behavior definition and
implementation, e.g. how cells in table sections in different directions share collapsed borders.

FireFox and IE don't support it. https://www.w3.org/TR/CSS2/tables.html doesn't mention anything about table row or section direction, and https://www.w3.org/TR/html401/struct/tables.html#h-11.2.1.1 says "TABLE is the only element on which dir reverses the visual order of the columns".

I would like to remove the support to eliminate inconsistency and reduce complexity.

The first step is to add use counters for the situations.

http://jsbin.com/cicuyo shows the situations.

In IE and Firefox, direction of section or row affects the inline direction of cells only, not affecting column order. 

 

Comment 1 by phistuck@gmail.com, May 29 2017

Huh. Weird feature. Can you dig into why it was added in the first place?
Labels: -OS-iOS
ios does not use Blink
Project Member

Comment 3 by bugdroid1@chromium.org, May 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70d88f21d9d5fbdd31646dbf14feed8bcae57c46

commit 70d88f21d9d5fbdd31646dbf14feed8bcae57c46
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue May 30 18:28:24 2017

Add use counter for table/section/row with different direction from table

Support of mixed direction of table/section/row causes much complexity
in our code. We also have many bugs and inconsistencies. It also causes
some situations difficult in both reasonable behavior definition and
implementation, e.g. how cells in table sections in different directions
share collapsed borders.

FireFox and IE don't support it. https://www.w3.org/TR/CSS2/tables.html
doesn't mention anything about table row or section direction, and
https://www.w3.org/TR/html401/struct/tables.html#h-11.2.1.1 says
"TABLE is the only element on which dir reverses the visual order of the
columns".

I would like to remove the support. This CL adds use counter for the
situations.

Change-Id: I92f2542184029053f93a2b460bf34f234600e91e
Bug:  727173 
Reviewed-on: https://chromium-review.googlesource.com/517668
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475605}
[modify] https://crrev.com/70d88f21d9d5fbdd31646dbf14feed8bcae57c46/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/70d88f21d9d5fbdd31646dbf14feed8bcae57c46/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/70d88f21d9d5fbdd31646dbf14feed8bcae57c46/tools/metrics/histograms/enums.xml

Labels: Merge-Request-60
I would like the use counter be launched in M60 so that we can get more data earlier.
Project Member

Comment 5 by sheriffbot@chromium.org, May 30 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: BugSource-Team PaintTeamTriaged-20170530
Project Member

Comment 7 by bugdroid1@chromium.org, May 30 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69cb7183530ed3e1b3a6b82e0b60350ddd263cd0

commit 69cb7183530ed3e1b3a6b82e0b60350ddd263cd0
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue May 30 18:58:55 2017

Add use counter for table/section/row with different direction from table

Support of mixed direction of table/section/row causes much complexity
in our code. We also have many bugs and inconsistencies. It also causes
some situations difficult in both reasonable behavior definition and
implementation, e.g. how cells in table sections in different directions
share collapsed borders.

FireFox and IE don't support it. https://www.w3.org/TR/CSS2/tables.html
doesn't mention anything about table row or section direction, and
https://www.w3.org/TR/html401/struct/tables.html#h-11.2.1.1 says
"TABLE is the only element on which dir reverses the visual order of the
columns".

I would like to remove the support. This CL adds use counter for the
situations.

TBR=wangxianzhu@chromium.org

(cherry picked from commit 70d88f21d9d5fbdd31646dbf14feed8bcae57c46)

Change-Id: I92f2542184029053f93a2b460bf34f234600e91e
Bug:  727173 
Reviewed-on: https://chromium-review.googlesource.com/517668
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#475605}
Reviewed-on: https://chromium-review.googlesource.com/517910
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#36}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/69cb7183530ed3e1b3a6b82e0b60350ddd263cd0/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/69cb7183530ed3e1b3a6b82e0b60350ddd263cd0/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/69cb7183530ed3e1b3a6b82e0b60350ddd263cd0/tools/metrics/histograms/enums.xml

https://html.spec.whatwg.org/multipage/tables.html#tables seems to have dropped "TABLE is the only element on which dir reverses the visual order of the columns". Maybe we should lobby to re-add something similar.

Only relevant mention of direction in css-tables-3 is https://drafts.csswg.org/css-tables-3/#intrinsic-percentage-width, which is not too specific, but is compatible with the proposed change.
+1 as this will remove a lot of code from https://codereview.chromium.org/2791433003 - which I'm still planning to nudge through.
Cc: atotic@chromium.org
I think Aleks had some conversations on this with Tab and fantasai. Anything we're missing here do you think?
I do not recall any discussion about removal of mixed direction. I am all for it if the use count supports it.
Labels: -Type-Bug Type-Launch-OWP
Labels: Launch-Accessibility-NA Launch-Legal-NA Launch-Privacy-NA Launch-Security-NA Launch-Status-Approval-Requested Launch-Test-NA Launch-UI-NA
Blocking: 128227
Cc: wangxianzhu@chromium.org
Labels: -Type-Launch-OWP Type-Bug
Owner: ----
Status: Available (was: Assigned)
Summary: "direction" css property or "dir" html attribute of table section and rows should not affect cell order (was: Intent to remove mixed direction of table section and rows)
Based on the information in the blink-dev thread https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K21NPMgyGS8, I think this is actually not a feature to remove, but a bug to fix. It's basically wkbug.com/96691, but we might need to change code more than the proposed patch in the webkit bug because we may have added other code to be "consistent" with the behavior. The fix might be to use table style instead of section/row style when determining cell order and cell adjacency e.g. for collapsed borders.

Note that this is not about the "mixed writing-mode/direction of cells" which is about that the different writing-mode/direction style on individual cells should be effective and is a feature we want to support.

Remaining works about this bug are:
1. Fix the bug;
2. Write web platform tests;
3. Comment on wkbug.com/96691 about the fix.

Making this bug available because I will be on vacation soon and will not work on this bug in the next 2 months.

Comment 16 by phistuck@gmail.com, Jun 22 2017

Some archeology -
Introduced by KHTML (post-WebKit-fork, but backported/copied to WebKit) -
https://websvn.kde.org/branches/make_khtml_cool/kdelibs/khtml/rendering/render_table.cpp?r1=174672&r2=174671&pathrev=174672
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Available)
I get some time to work on this.
Cc: kojii@chromium.org
 Issue 76858  has been merged into this issue.

Comment 19 by phistuck@gmail.com, Jun 22 2017

#18 - though the reproduction cases in  issue 76858  do not have any "direction" property on tr and such, only on td.
Merged  bug 76858  into bug 736072 instead which is about cell direction.

Comment 21 by kojii@chromium.org, Jun 23 2017

FYI, please refer to https://drafts.csswg.org/css-writing-modes-3/#direction for the spec if you need.

There are some tests under wpt/css/css-writing-modes-3. Results of our last run is here.
http://kojiishi.github.io/generate-w3c-implementation-report/?q=table
These tests are imported in external/wpt/css/css-writing-modes-3.

Latest implementation report here:
https://drafts.csswg.org/css-writing-modes-3/implementation-report.html
Project Member

Comment 22 by bugdroid1@chromium.org, Jun 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2173679a4a69e4352debd9fee819281592b31240

commit 2173679a4a69e4352debd9fee819281592b31240
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Jun 23 16:01:07 2017

Add tests for table/row/row-group directions

BUG= 727173 

Change-Id: Idb20e6d81192865d125c802e40ef58ab1960ffbd
Reviewed-on: https://chromium-review.googlesource.com/545136
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481909}
[modify] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-direction-ref.html
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-direction.html
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-direction-ref.html
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-direction.html
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-group-direction-ref.html
[add] https://crrev.com/2173679a4a69e4352debd9fee819281592b31240/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-group-direction.html

I started CQ before seeing kojii@'s comment, and failed to stop it before its landing.

Checking if there are already similar tests under css-writing-mode-3.
 Issue 736111  has been merged into this issue.
Project Member

Comment 25 by bugdroid1@chromium.org, Jun 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56ab4c1ae8f30911e2c7e11fa73a088f49bfb1ad

commit 56ab4c1ae8f30911e2c7e11fa73a088f49bfb1ad
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Mon Jun 26 20:59:00 2017

Revert "Add tests for table/row/row-group directions"

This reverts commit 2173679a4a69e4352debd9fee819281592b31240.

Reason for revert: We already have test coverage under
external/wpt/css/css-writing-mode-3.

Original change's description:
> Add tests for table/row/row-group directions
> 
> BUG= 727173 
> 
> Change-Id: Idb20e6d81192865d125c802e40ef58ab1960ffbd
> Reviewed-on: https://chromium-review.googlesource.com/545136
> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> Reviewed-by: David Grogan <dgrogan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481909}

TBR=wangxianzhu@chromium.org,dgrogan@chromium.org,mkwst@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  727173 
Change-Id: I011fdc370f549f5337894dae1a5bbed44a88181f
Reviewed-on: https://chromium-review.googlesource.com/549058
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482393}
[modify] https://crrev.com/56ab4c1ae8f30911e2c7e11fa73a088f49bfb1ad/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-direction-ref.html
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-direction.html
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-direction-ref.html
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-direction.html
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-group-direction-ref.html
[delete] https://crrev.com/66d7c69f7d8c8d63138066ad74a89524f3a4e250/third_party/WebKit/LayoutTests/external/wpt/html/rendering/non-replaced-elements/tables/table-row-group-direction.html

Project Member

Comment 26 by bugdroid1@chromium.org, Jun 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e84e7643da78ca93373487c5e8dfec261933fbb

commit 6e84e7643da78ca93373487c5e8dfec261933fbb
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Jun 27 01:29:27 2017

Use table's style for cell ordering

In discussion in  crbug.com/727173  and
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K21NPMgyGS8,
we decided to not allow table section/row's direction affect cell
ordering.

Now use the table's style for cell ordering.

Make it explicit that the logical directions of collapsed borders are
in the table's direction.

BUG= 727173 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ia7a4bc3eca9b7537a78850bce53b8cbbbb2df813
Reviewed-on: https://chromium-review.googlesource.com/547740
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482495}
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/LayoutTests/fast/table/border-collapsing/first-cell-left-border-hidden-table-ltr-section-rtl-expected.html
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/LayoutTests/fast/table/border-collapsing/first-cell-left-border-hidden-table-ltr-section-rtl.html
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/LayoutTests/fast/table/border-collapsing/last-cell-left-border-hidden-table-ltr-section-rtl-expected.html
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/LayoutTests/fast/table/border-collapsing/last-cell-left-border-hidden-table-ltr-section-rtl.html
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTable.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableCell.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableCellTest.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableCol.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableRow.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/layout/LayoutTableSection.h
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/style/ComputedStyle.cpp
[modify] https://crrev.com/6e84e7643da78ca93373487c5e8dfec261933fbb/third_party/WebKit/Source/core/style/ComputedStyle.h

Status: Fixed (was: Assigned)

Sign in to add a comment