Bug 219291

Summary:

REGRESSION (?): Increasing column-count above 2 at runtime has no effect

Product:

WebKit

Reporter:

Peter Dekkers <peter>

Component:

Layout and Rendering

Assignee:

zalan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, changseok, esprehn+autocc, ews-watchlist, glenn, koivisto, kondapallykalyan, niedziolek, pdr, peter, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

Safari 14

Hardware:

All

OS:

Unspecified

Attachments:

Description	Flags
Short video illustrating the buggy behaviour	none
Minimal reproduction	none
Screen-recoding with local fix	none
Patch	none
[fast-cq]Patch	none

Description Peter Dekkers 2020-11-25 20:03:54 PST

Created attachment 414874 [details]
Short video illustrating the buggy behaviour

When updating CSS column-count to values above 2 at runtime (i.e. with JavaScript) doesn't seem to take effect. The underlying CSS updates but the browser doesn't display more than two columns.

A basic reproduction:
https://codepen.io/editkid/pen/WNGNNew

I am under the impression that this behaviour only started recently, possibly in Safari 14.

Comment 1 Peter Dekkers 2020-11-25 22:18:38 PST

Created attachment 414881 [details]
Minimal reproduction

A minimal reproduction of the bug in this simple HTML file with inline JavaScript. To replace the codepen.io link from my previous post.

Comment 2 Radar WebKit Bug Importer 2020-11-30 09:34:36 PST

<rdar://problem/71808738>

Comment 3 Michal Niedziolka 2023-02-23 03:22:24 PST

It's not only about increasing column-count, but simply changing it.
Take a look at https://codepen.io/NiedziolkaMichal/pen/MWqeJKb
After activating multi-column layout, it's not possible to change the number of columns by changing the value of `columns` or `column-count` property.

Comment 4 zalan 2023-02-23 08:27:09 PST

Thank you for adding a minimal repro case, it's really great.
and yeah, I am able to reproduce it too. I went back to a relatively old build (from spring 2021) and was still able to repro it.
It looks like we don't trigger layout when the property value changes. Forcing layout recovers it (e.g. window resize).

Comment 5 Peter Dekkers 2023-02-23 16:02:02 PST

Thank you.

I was hopeful that the insight regarding a forced layout change would enable a workaround for this bug. After trying to trigger a layout change using JavaScript, this doesn't seem possible. Safari blocks most layout changes originating from JavaScript (and, rightfully-so).

Comment 6 zalan 2023-02-23 17:07:47 PST

(In reply to Peter Dekkers from comment #5)
> Thank you.
> 
> I was hopeful that the insight regarding a forced layout change would enable
> a workaround for this bug. After trying to trigger a layout change using
> JavaScript, this doesn't seem possible. Safari blocks most layout changes
> originating from JavaScript (and, rightfully-so).
Oh I just realized this was reported back in 2020 (no wonder the build from 2021 reprod it). :| (apparently it showed up in my mailbox because Michal left a comment). Alight, let me grab this bug.

Comment 7 zalan 2023-02-23 17:09:10 PST

> Alight, let me grab this bug.
"Alight is a leading cloud-based human capital technology and services provider that powers confident health, wealth and wellbeing decisions for 36 million people and dependents"
Not bad, but let's just go with "alright".

Comment 8 Peter Dekkers 2023-02-23 17:10:09 PST

> "Alight is a leading cloud-based human capital technology and services provider

Learning every day :) Thank you.

Comment 9 zalan 2023-02-24 22:12:49 PST

Created attachment 465167 [details]
Screen-recoding with local fix

with proper dirty bit propagation, this starts working as expected. will be posting my patch soon.

Comment 10 zalan 2023-02-25 14:55:13 PST

Created attachment 465173 [details]
Patch

Comment 11 Michal Niedziolka 2023-02-25 15:07:21 PST

Thank you for your very quick response!
I have noticed that changes to the `column-width` are also not triggering reflow of the multi-column layout. This fix will affect `column-width` too?

If you or somebody else wants to make sure that this patch works well, the following 3 interactive examples should now work well on MDN:
https://developer.mozilla.org/en-US/docs/Web/CSS/column-count
https://developer.mozilla.org/en-US/docs/Web/CSS/column-width
https://developer.mozilla.org/en-US/docs/Web/CSS/columns

Comment 12 Michal Niedziolka 2023-02-25 15:14:45 PST

I think you have a typo in the file dynamic-column-count-change.html at line 11:
"document.body.offsetHeigh;"
probably should be:
"document.body.offsetHeight;"

Comment 13 zalan 2023-02-25 15:34:48 PST

(In reply to Michal Niedziolka from comment #12)
> I think you have a typo in the file dynamic-column-count-change.html at line
> 11:
> "document.body.offsetHeigh;"
> probably should be:
> "document.body.offsetHeight;"
good catch, let me fix that.

Comment 14 zalan 2023-02-25 15:51:31 PST

Created attachment 465176 [details]
[fast-cq]Patch

Comment 15 EWS 2023-02-26 08:12:14 PST

Committed 260849@main (e0b27b7e1c9b): <https://commits.webkit.org/260849@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465176 [details].

Comment 16 Peter Dekkers 2023-02-26 12:56:08 PST

Amazing, cheers @zalan! Columns for everyone.

Comment 17 zalan 2023-02-26 19:48:21 PST

(In reply to Peter Dekkers from comment #16)
> Amazing, cheers @zalan! Columns for everyone.
Yay! [insert the obligatory Oprah meme]