Closed Bug 1503420 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout: Columns, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached file body-column.html
See attached test case.
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
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.
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)

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.
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
Status: ASSIGNED → RESOLVED
Closed: 2 years 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.