Closed Bug 1764437 Opened 3 years ago Closed 3 years ago

Check for sibling frames when propagating CSS page-name values

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: alaskanemily, Assigned: alaskanemily)

References

Details

Attachments

(1 file)

The exact situation is where we need to propagate the CSS page-name back up the frame tree from a frame that is the first child of a parent frame that already has a page-name set. The initial implementation of CSS page-name fragmentation in Bug 1740366 does not handle this case. This doesn't seem to be specifically needed for gsuite printing support, but should be fixed to properly implement the CSS page 3 spec.

We should probably implement this properly before named pages is preffed on by default.

This case is demonstrated by the test cases layout/reftests/css-page/page-name-siblings-002.html and layout/reftests/css-page/page-name-display-none-child.html

Severity: -- → S3
Priority: -- → P2

(In reply to Emily McDonough [:alaskanemily] from comment #0)

This case is demonstrated by the test cases layout/reftests/css-page/page-name-siblings-002.html and layout/reftests/css-page/page-name-display-none-child.html

Both of the testcases mentioned here are now marked as passing (though they were annotated as failing when this bug was filed).

Do we still have a correctness issue here? (If so, do we need a new testcase to demonstrate / test for that?)

(Noticed this because https://phabricator.services.mozilla.com/D152701 is moving a code-comment that refers to this bug.)

Flags: needinfo?(emcdonough)

I believe there are still issues here. Since we link back to this bug in that code, I'll note what my concerns are:

In that code review at layout/base/nsCSSFrameConstructor.cpp line 9661, we are checking if somewhere in the frame's direct ancestors that we have the mStartPageValue value set on the nsIFrame::PageValuesProperty(). However, because this code will run before the child frames of this frame (f in this code) are created, if f has a first child with a different page-name, then when this code runs for that child it will see the mStartPageValue we set for f and not set that value.

This only affects mStartPageValue in this implementation because we unconditionally set mEndPageValue. To fix this, we should replace the null-check on mStartPageValue with a check that the previous frame we inspected (a child of frame in that loop) is frame's first child.

We don't have enough tests for complex/vertical frame trees that would catch this yet, and instead only the simplest case is working (which is those tests that are now working were for).

Flags: needinfo?(emcdonough)

After testing, I believe that this was likely fixed by moving page value propagation in Bug 1779645 (or was possibly never actually an issue). I'm repurposing this bug for the same code change that was planned, as it will be needed for lazy page-value setting on frames.

Blocks: 1782597
No longer blocks: 1723234
Severity: S3 → N/A
Type: defect → enhancement
See Also: → 1779645
Summary: Properly handle CSS page-name fragmentation for a first child with a different page-name → Check for sibling frames when propagating CSS page-name values
Assignee: nobody → emcdonough
Status: NEW → ASSIGNED
Pushed by emcdonough@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7d1db6d321f Check for prev/next sibling when propagating start/end page values for frames r=dholbert
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: