Bug 245936 - Subgrid is not aligned correctly with miscalculated gap and page crashes
Summary: Subgrid is not aligned correctly with miscalculated gap and page crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 16
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Woodrow
URL: https://codepen.io/chriskirknielsen/p...
Keywords: BrowserCompat, InRadar
Depends on:
Blocks: 202115
  Show dependency treegraph
 
Reported: 2022-10-01 16:20 PDT by Christopher Kirk-Nielsen
Modified: 2022-11-14 16:39 PST (History)
7 users (show)

See Also:


Attachments
Firefox 105 OK, Safari 16 FAIL. Resizes causes the page to crash and reload. (211.53 KB, image/png)
2022-10-01 16:20 PDT, Christopher Kirk-Nielsen
no flags Details
rendering in safari, firefox, chrome (314.45 KB, image/png)
2022-10-03 01:31 PDT, Karl Dubost
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Kirk-Nielsen 2022-10-01 16:20:40 PDT
Created attachment 462754 [details]
Firefox 105 OK, Safari 16 FAIL. Resizes causes the page to crash and reload.

I am working on a page which leverages subgrid to align some various items. I noticed that the content was offset by about the same size as 2x the column-gap value I was using. The grid is modified using a custom property replacing a single column with a `repeat` that divides the space up into 5 columns within that central "content area".

Additionally, the page crashes when I resize the window (see the bottom of the attached screenshot), I suspect the complexity of all the custom properties determining the size as well as the subgrid being calculated on the fly, all wrapped in a media query, is too much to handle at once. While  my original page crashes pretty consistently, the demo seems to be quite slower but not always crashing. However, my page has many more elements placed more specifically along the grid, whereas the demo is simpler so the issue might not be as severe

I have reproduced the issue on a small sample, DEMO: https://codepen.io/chriskirknielsen/pen/dyeeRYY

Tested:
Firefox 105: PASS
Safari 16: FAIL
Safari TP 154: FAIL
Comment 1 Karl Dubost 2022-10-03 01:31:51 PDT
Created attachment 462768 [details]
rendering in safari, firefox, chrome

Tested on macOS 13.0
---
Safari Technology Preview  16.0          18615.1.6.2 (STP 154)
Firefox Nightly            107.0a1       10722.9.30
Google Chrome Canary       108.0.5336.0  5336.0


3 different renderings.
Comment 2 Tim Nguyen (:ntim) 2022-10-03 11:13:20 PDT
(In reply to Karl Dubost from comment #1)
> Google Chrome Canary       108.0.5336.0  5336.0


Is that with subgrid support enabled?
Comment 3 Karl Dubost 2022-10-03 15:48:52 PDT
(In reply to Tim Nguyen (:ntim) from comment #2)
> Is that with subgrid support enabled?

In chrome://flags/ there is nothing to enable subgrid.
(Version 108.0.5338.0 (Official Build) canary (arm64))
Comment 4 Sam Sneddon [:gsnedders] 2022-10-05 09:03:13 PDT
(In reply to Karl Dubost from comment #3)
> (In reply to Tim Nguyen (:ntim) from comment #2)
> > Is that with subgrid support enabled?
> 
> In chrome://flags/ there is nothing to enable subgrid.
> (Version 108.0.5338.0 (Official Build) canary (arm64))

Pretty sure you need --enable-blink-feature=LayoutNGSubgrid as a command line argument still.
Comment 5 Radar WebKit Bug Importer 2022-10-08 16:21:17 PDT
<rdar://problem/100939450>
Comment 6 Matt Woodrow 2022-11-06 19:43:40 PST
I can't reproduce any crashes here, but I do see a bug with gap.

It looks like there's not great interop here (I see differences in handling with Chrome too, even with subgrid support enabled). I think Firefox is right for your test case though.

Will try fix and add a few new WPT tests for this.
Comment 7 Matt Woodrow 2022-11-06 20:07:56 PST
Pull request: https://github.com/WebKit/WebKit/pull/6193
Comment 8 EWS 2022-11-13 14:57:15 PST
Committed 256621@main (87d76d7dac68): <https://commits.webkit.org/256621@main>

Reviewed commits have been landed. Closing PR #6193 and removing active labels.