Column-span is not working if the multi-column container is <body> element

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

8 months ago
Posted file body-column.html
See attached test case.
Assignee

Updated

5 months ago
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Assignee

Comment 1

5 months ago
To reproduce, execute
"GECKO_FRAMECTOR_DEBUG_FLAGS=really-noisy-content-updates ./mach run".

List()'s second argument should be a const char*, not an integer. We can
fix the bug by omitting it because the default value of the argument is
an empty string.
Assignee

Comment 2

5 months ago
The elements under <body> are treat as content append, so their frames
will be construct by nsCSSFrameConstructor::ContentAppended.

This patch fixed only the simple "append" case which is appending to the
last continuation of ::moz-column-content. For other more completx
appending or inserting cases, we might need to reframe (bug 1504053).

Depends on D16075

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #1)

List()'s second argument should be a const char*, not an integer. We can
fix the bug by omitting it because the default value of the argument is
an empty string.

Where are you seeing the second arg being a const char*?

This is being called on aFirstNewContent->GetParent() which has type nsIContent*, and I'm seeing this in the nsIContent class definition:

  virtual void List(FILE* out = stdout, int32_t aIndent = 0) const = 0;

https://searchfox.org/mozilla-central/source/dom/base/nsIContent.h#848-852

It's fine to omit the aIndent since it defaults to 0, but it also shouldn't make a functional difference...

Flags: needinfo?(aethanyc)

Ah, I do see the nsIFrame list method takes a char* as its second arg. But your patch is touching calls on nsIContent as well on nsIFrame (the first call, at least). So, those changes need to be omitted from the patch.

Flags: needinfo?(aethanyc)
Assignee

Comment 5

5 months ago

Ah, I didn't aware some of the callers are nsIContent*. The buggy callers are nsContainerFrame::List() with definition:

  void List(FILE* out = stderr, const char* aPrefix = "",
            uint32_t aFlags = 0)

https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/layout/generic/nsContainerFrame.h#73

I should either rewrite the comment or fix only those buggy ones.

Yeah, it'd probably be fine to simplify the non-buggy nsIContent calls as well. But (per nits on phabricator) that should happen separately, to avoid grouping kinda-unrelated functional changes & non-functional changes together.

Attachment #9035397 - Attachment description: Bug 1503420 Part 1 - Fix crash when using GECKO_FRAMECTOR_DEBUG_FLAGS. → Bug 1503420 Part 1 - Adjust some callers of nsIFrame::List() to stop passing 0 as a const char* argument.

Comment 7

5 months ago
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/918042a8eb34
Part 1 - Adjust some callers of nsIFrame::List() to stop passing 0 as a const char* argument. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3eafb595be4f
Part 2 - Fix appending a subtree with column-span descendants under the ::moz-column-content. r=emilio

Comment 8

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14906 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.