1764437 - Check for sibling frames when propagating CSS page-name values

Status: RESOLVED FIXED

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

Description

[Emily McDonough [:alaskanemily]]

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

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[Emily McDonough [:alaskanemily]]

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).

Comment 3

[Emily McDonough [:alaskanemily]]

Attachment: Bug 1764437 - Check for prev/next sibling when propagating start/end page values for frames

Comment 4

[Emily McDonough [:alaskanemily]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7d1db6d321f
Check for prev/next sibling when propagating start/end page values for frames r=dholbert

Comment 6

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c7d1db6d321f