-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collapsed table first row width quirk #6230
Comments
My understanding is that fixed tables work that way so that you can add rows without having to recompute the column widths. Checking the second row would defeat that purpose. |
I believe that original intent was that you were able to determine column widths just by traversing the first row. This optimization is no longer possible, and might have never been possilble.
Also, computing table's ink overflow requires traversing the entire table. Today, this behavior is just a historical quirk that makes life more complex for web developers. |
Ok, so if I understood correctly, what you are saying that with the current complexity level of the web platform, this optimization is no longer useful, and that it's a constraint we should relax to make developers' life easier. Is that correct? If so, I am not against that. Regarding web compatibility, could you put a use counter so we can take a look at a few sites and check if that improves or cause issues for them? |
Filed a chrome issue asking for a use counter: https://bugs.chromium.org/p/chromium/issues/detail?id=1238243 |
Wonderful, thanks @atotic! |
Bug: 1238243 Change-Id: Ie23e33c79c6ab4c3df6aafc9fb49801d8b035a9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3101511 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Aleks Totic <atotic@chromium.org> Reviewed-by: Aleks Totic <atotic@chromium.org> Cr-Commit-Position: refs/heads/master@{#912897}
I've added in the use-counter. My expectation is that there won't be a many (if any) sites relying on this quirk. We should have some initial data in a couple of months. As Aleks says implementations already have to calculate the other value to correctly determine visual-overflow so there really isn't any "performance benefit". (There /potentially/ was in old implementations which has much simpler paint sub-systems, e.g. they didn't care about visual overflow as much, but this isn't true for modern implementations). |
I finally got around to performing some analysis on our UseCounter data. TL;DR I think this change is going to be fine. The UseCounter is "high" at ~0.55% of pageloads. However I analyzed a lot of examples URLs - all the changes are all minimal. The changes broadly fall into ~4-5 categories. Craigslist - e.g. https://sfbay.craigslist.org/
However this doesn't match the Discourse - e.g. https://discourse.wicg.io/categories Open table reservation widget - e.g. https://www.charcoalgrillbar.com/ Various calendar widgets on blogs etc. In my opinion if we'd like to do this change - it'll has a high chance of being fine, and given the amount of confusion it has generated over the years (e.g. we still get bug reports about this behaviour) likely a positive for the web platform. Personally my only real concern is the discourse example. Ian |
Sounds like good news to me! |
Yeah I think so - its fairly straightforward for us. I'll let you adgenda+ it so that you can be on the call. |
This week is APAC call. So yeah not joining, will Agenda+ for the next one. |
FWIW the current spec:
And the older spec:
All engines seem to do this latter thing, but rounding issues cause this to be un-noticable in Firefox in the initial test case. Here is a more clear test case: |
The CSS Working Group just discussed
The full IRC log of that discussion<TabAtkins> Topic: collapsed table row quirk<TabAtkins> github: https://github.com//issues/6230 <TabAtkins> fremy: while reimplementing tables in chrome, the dev found a lot of bugs that werne't bugs per spec, but were unexpected by authors <TabAtkins> fremy: We realized the new version of the spec was causing the issues <TabAtkins> fremy: But behavior that is currently implemented was mentioned in an old version of the spec <TabAtkins> fremy: So chrome dev decided to implement the old behavior, since we didn't ahve resolution for the spec change, but they'd like to change to the new beahvior because the old didn't make much sense <TabAtkins> fremy: They feared there would be some breakage with new behavior, so they added a UseCounter and studied websites <TabAtkins> fremy: Found that on all websites, the changes to layout would be very small, if any, and often slightly better. Found one case where it was slightly worse, but in no case was it worse than about 3px. <TabAtkins> fremy: So they want to update their impl to the new spec, if the WG approves it <TabAtkins> fremy: So that's context. Here's the behavior. <iank_> https://www.w3.org/TR/CSS2/tables.html#:~:text=17.6.2-,The%20collapsing%20border%20model,-In%20the%20collapsing <TabAtkins> fremy: When you have a table in collapsed border mode <bradk> 3px narrower or 3px wider? <TabAtkins> fremy: Two cells next to each other, the border needs to be "harmonized" - pick one of the two borders and draw. <TabAtkins> fremy: Must also harmonize border between cells and the table itself. <TabAtkins> fremy: In the new spec you harmonize all the cells on the table edge, take the biggest one. Do that for each edge. <TabAtkins> fremy: The older spec for some reason only harmonized the first cell of the first row. <astearns> https://www.w3.org/TR/CSS2/tables.html#collapsing-borders is a more interoperable version of iank_'s URL <TabAtkins> fremy: If you have a table you don't draw borders around the header row, but you use borders for the rest, <TabAtkins> fremy: According to the old spec, the borders will then be drawn outside the table <TabAtkins> fremy: so it often just causes an overflow of like 1px, not noticeable <TabAtkins> fremy: But scrollbars can pop <TabAtkins> fremy: And devs can't figure out why it's happening <bradk> That answers my question <TabAtkins> fremy: Bug is marked as Doesn'tReproduce in Firefox, technically true, because they do something differently but still wrong, it's based on rounding. <TabAtkins> fremy: So proposal is to accept the new spec text: when you decide the border of a table, you look at *all* the cells against each border edge, not just the first <TabAtkins> fremy: Should be more in line with dev expectations, but it's currently a breaking change so we wanted a resolution. <TabAtkins> iank_: Ironically this is one of the better specified areas in 2.1 regarding tables, which is why everyone has the same (bad) behavior. <astearns> ack fantasai <TabAtkins> fantasai: I worked on this issue in 2.1. I was really unhappy with the old resolution. <TabAtkins> fantasai: Happy to move away from that. <TabAtkins> fantasai: There was a proposal to have the borders not belonging to the table overlfow; another was to count thema s part of the border with. I think there was a third to allow it to overlap with the margin, but take up space if there wasn't enough amrgin. <TabAtkins> fantasai: Say you have a table with anrrow borders, and you used a thick border to highlight an interesting cell. <TabAtkins> fantasai: If you want the table to stay centered when the highlighted cell happens to be on the edge, we can't include the difference in the border width, it'll shift the table slighitly <TabAtkins> fantasai: But that is a more complicated behavior, but I want to put it out for consideration. <TabAtkins> fantasai: That effect is why, I suspect, why we had not taken the max border width, so those borders could overlap into the margin. <TabAtkins> iank_: I don't believe we've found anyone using it that way. Might exist, but broadly the usage fell into a few buckets. <TabAtkins> iank_: Primary is calendar widgets - craigslist was the big one. Had some dead code to add a transparent border to fix the behavior. <TabAtkins> astearns: So the current spec describes what you prefer? <TabAtkins> iank_: I think the max-border beahvior is defined in Tables 3. <TabAtkins> iank_: caveated on us not breaking too much <TabAtkins> iank_: Use counter is high-ish but layout changes are very minor. <TabAtkins> fremy: Current Tables spec does have symmetrical behavior <TabAtkins> astearns: francois, do you ahve opinions on the behavior fantasai described? <TabAtkins> fremy: I see it's useful behavior <TabAtkins> fremy: But think you should probably be using outline to add this kind of call-out border <TabAtkins> fremy: So I think the simple behavior is fine. Wouldn't be mad either way. <TabAtkins> fantasai: Think it's probably fine. <TabAtkins> astearns: So proposed resolution is no change to the Tables spec, just confirming that the current spec is intended. <fantasai> The thing I *wish* we could fix in table borders model is to invert the priority so that table wins over rowgroup over row over cell. <fantasai> The current version is just so impossible to deal with if you want different colored but same-thickness borders for rows vs rowgroups, etc. <TabAtkins> iank_: Think the spec fixes some unintuitive behavior for webdevs. Will report if there's problems. <TabAtkins> RESOLVED: Accept current Tables 3 spec text regarding calculation of shared borders in collapsed-border mode between table and cells. |
\o/ |
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504}
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504}
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504}
…testonly Automatic update from web-platform-tests [wpt] Fix test for crbug.com/1238243 Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504} -- wpt-commits: cc0620f158fa70053f2fa72aa9c2e77329338dc9 wpt-pr: 34883
…testonly Automatic update from web-platform-tests [wpt] Fix test for crbug.com/1238243 Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504} -- wpt-commits: cc0620f158fa70053f2fa72aa9c2e77329338dc9 wpt-pr: 34883
Bug: 1238243 Change-Id: Ie23e33c79c6ab4c3df6aafc9fb49801d8b035a9c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3101511 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Commit-Queue: Aleks Totic <atotic@chromium.org> Reviewed-by: Aleks Totic <atotic@chromium.org> Cr-Commit-Position: refs/heads/master@{#912897} NOKEYCHECK=True GitOrigin-RevId: 978844e5601ea402e1c49c0791b0de9041e68b4d
This disables the logic for the table border quirk. See: w3c/csswg-drafts#6230 For more information. This doesn't remove the logic - however the primary function which uses this: NGTableBorders::GetCollapsedBorderVisualSizeDiff will return an empty NGBoxStrut now. If this lands successfully we'll remove the remaining logic in: https://chromium-review.googlesource.com/c/chromium/src/+/3484918/1 Bug: 1238243 Change-Id: Ic5c8a369b09cc693e4a4b20f269ccd5a159c3e98 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3485478 Reviewed-by: Aleks Totic <atotic@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#974738} NOKEYCHECK=True GitOrigin-RevId: 0da4bca813208acafeb242b108b1f1791a960c8f
Updates test as per: w3c/csswg-drafts#6230 Fixed: 1238243 Change-Id: I75ef894dabf63daf7c49a0a728668aac44e8ce7d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3770091 Auto-Submit: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Commit-Queue: Ian Kilpatrick <ikilpatrick@chromium.org> Cr-Commit-Position: refs/heads/main@{#1025504} NOKEYCHECK=True GitOrigin-RevId: 974b94f6ca920f75b2fb3759d388666abb78971e
Chrome gets bug reports about incorrect table widths for tables with collapsed borders.
Examples: https://crbug.com/1200116 https://crbug.com/711360
The issue happens because per spec, width of the table in collapsed borders mode is determined by borders in the 1st row.
I could not find definition of this behavior in the latest spec, but here it is in the old spec:
https://www.w3.org/TR/2011/REC-CSS2-20110607/tables.html#collapsing-borders
"As must compute an initial left and right border width for the table by examining the first and last cells in the first row of the table.'
If the first row has no borders, this results in a behavior developers consider a bug.
What do you think about using all the rows to compute table size? I am not sure if this is web compatible, but it would increase developer happiness.
The text was updated successfully, but these errors were encountered: