1413619 - FindFrameForContentSibling gets display: contents pseudos wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Mats noticed this while reviewing bug 979782 (see bug 979782 comment 19). I'm writing a test-case now...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

This actually fixes bug 1251799, and bug 1359656. Turns out the bug it was
hiding was this one! :)

Review commit: https://reviewboard.mozilla.org/r/195456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195456/

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195456/#review200590

r=mats

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195456/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee243422ed36
Fix insertion point computation when display: contents pseudos are involved. r=mats

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b130c004f81
Backed out changeset ee243422ed36 for test failures.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Gah, seems like a couple tests are failing, I'll look into those...

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review commit: https://reviewboard.mozilla.org/r/195616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195616/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195456/diff/2-3/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

So this was because right now nothing was wallpapering the fact that we didn't return the ::before of the parent when scanning siblings of a display: contents element.

I've reworked a bit. I've added it as a second commit on top of the first one because I think it's easier to review. Mind taking a look mats? So far I've checked that this passes css-display completely, doing a full try run now.

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195616/#review200786


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/base/nsCSSFrameConstructor.cpp:6970
(Diff revision 1)
>  
> +  return aSibling;
> +}
> +
>  nsIFrame*
> -nsCSSFrameConstructor::FindNextSibling(FlattenedChildIterator aIter,
> +nsCSSFrameConstructor::FindPreviousSibling(FlattenedChildIterator aIter,

Warning: The parameter 'aiter' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: layout/base/nsCSSFrameConstructor.cpp:6979
(Diff revision 1)
> -      return nextSibling;
> -    }
> -  }
> +}
>  
> -  return nullptr;
> +nsIFrame*
> +nsCSSFrameConstructor::FindNextSibling(FlattenedChildIterator aIter,

Warning: The parameter 'aiter' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195616/diff/1-2/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Looks reasonably green! https://treeherder.mozilla.org/#/jobs?repo=try&revision=13a6026a5ab39a56acdced90ddac7858bcd92dbe&selectedJob=141527355

Ignore the font inflation stuff, that's because of other patch that is under these. This is green except for some table caption reftests, and those are easy (since it's the aTargetContent != aIter.Get() case).

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Hm, I am lying and that last bit is wrong, so time to debug caption insertions I guess... :(

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195456/diff/3-4/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195616/diff/2-3/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

This pretty much still needs a review. I'm pretty happy with the code, I think it's easier to follow now, but happy to hear feedback too, since it makes significant changes.

And yeah, this is totally green now, the failures above were due to the ValidSibling stuff.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9acf8eb5e75d6fc6e8adb8ca58572b1cdf358f1

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195456/#review201226

Thanks for simplifying and cleaning up this mess!

::: layout/base/nsCSSFrameConstructor.h:2154
(Diff revision 4)
> +  // FIXME(emilio): If we ever kill IsValidSibling, this can become much nicer
> +  // (we can remove aTargetContent and aTargetContentDisplay, and all the
> +  // adjust() conditionals).

This comment might be better placed in the .cpp file, so that "the adjust() conditionals" makes sense.

::: layout/base/nsCSSFrameConstructor.h:2161
(Diff revision 4)
> -                            nsContainerFrame* aParentFrame);
> +    bool aDeep = false);
> +

This param needs @param documentation.
Also, bool params makes code less readable in general,
so I'd prefer an enum class.
If you keep the bool type, then at least don't use false/true
in the calls.  Use something like this instead:
const bool deep = false;
FindNextSiblingInternal(..., deep)

"deep" seems like rather non-descriptive name too, is there a better
name for what it does?  It seems to me it's true when we're recursing
into a display:contents sibling, and false otherwise.  When true, it
prevents looking at ancestors at the end of Find*SiblingInternal,
which makes sense for nested calls since we already checked those.
Perhaps "aIsNestedCall" would be more accurate?

IIUC what it does, then why can't we do the "recurse into the ancestor
for display:contents nodes" at the end of the *Internal methods in
the caller instead?  Then we wouldn't need this param right?

::: layout/base/nsCSSFrameConstructor.cpp:6881
(Diff revision 4)
> -                                                  nsContainerFrame* aParentFrame,
> +  bool aDeep)
> -                                                  bool aPrevSibling)
>  {
> -  nsIFrame* sibling = aContent->GetPrimaryFrame();
> -  if (!sibling && GetDisplayContentsStyleFor(aContent)) {
> -    // A display:contents node - check if it has a ::before / ::after frame...
> +  auto adjust = [&](nsIFrame* aPotentialSiblingFrame) -> nsIFrame* {
> +    return AdjustSiblingFrame(
> +      aPotentialSiblingFrame, aTargetContent, aTargetContentDisplay, false);

Probably worth replacing |false| with:
const bool isPreviousSibling = false;
and pass that for clarity.

::: layout/base/nsCSSFrameConstructor.cpp:6885
(Diff revision 4)
> -      // continuation fixups.
> -      if (sibling) {
> +    if (nsIFrame* frame = sibling->GetPrimaryFrame()) {
> +      if (frame->GetContent() == sibling) {

I think we should retain the original comment:
    // XXX the GetContent() != aContent check is needed due to bug 135040.
    // Remove it once that's fixed.
here, with s/aContent/sibling/

::: layout/base/nsCSSFrameConstructor.cpp:6895
(Diff revision 4)
> +        if (nsIFrame* sibling = adjust(frame)) {
> +          return frame;

This should be "return sibling" right?

::: layout/base/nsCSSFrameConstructor.cpp:6937
(Diff revision 4)
> -    MOZ_ASSERT(sibling != aTargetContent);
> -    nsIFrame* prevSibling =
> +    if (nsIFrame* frame = sibling->GetPrimaryFrame()) {
> +      if (frame->GetContent() == sibling) {

ditto about the XXX comment

::: layout/base/nsCSSFrameConstructor.cpp:6947
(Diff revision 4)
> +        if (nsIFrame* sibling = adjust(frame)) {
> +          return frame;

ditto about "return sibling"

::: layout/base/nsCSSFrameConstructor.cpp:6970
(Diff revision 4)
>  
> +  FlattenedChildIterator iter(aIter.Parent()->GetFlattenedTreeParent());
> +  iter.Seek(aIter.Parent());
> +  return FindPreviousSiblingInternal(iter, aTargetContent, aTargetContentDisplay);

I'd like to retain the essence of the original comment:
        // Our siblings (if any) does not have a frame to guide us.
        // The frame for aChild should be inserted whereever a frame for
        // the container would be inserted.  This is needed when inserting
        // into nested display:contents nodes.
before this code block.  (ditto in FindNext*, so probably better to add
it in the next patch where you merge them)

::: layout/base/nsCSSFrameConstructor.cpp:6998
(Diff revision 4)
> +    aSibling = aSibling->GetTailContinuation();
> +  }

GetTailContinuation isn't obvious what it does so I'd like to keep a comment, e.g.:
// Use the last non-overflow-container continuation as our previous sibling.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195616/#review201236

I'll wait with reviewing this part until the next version.
Sharing the code using a template seems like a good idea though.
(Oh, I see you fixed the "return frame" bug here :-))

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924240 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195456/#review201226

> This param needs @param documentation.
> Also, bool params makes code less readable in general,
> so I'd prefer an enum class.
> If you keep the bool type, then at least don't use false/true
> in the calls.  Use something like this instead:
> const bool deep = false;
> FindNextSiblingInternal(..., deep)
> 
> "deep" seems like rather non-descriptive name too, is there a better
> name for what it does?  It seems to me it's true when we're recursing
> into a display:contents sibling, and false otherwise.  When true, it
> prevents looking at ancestors at the end of Find*SiblingInternal,
> which makes sense for nested calls since we already checked those.
> Perhaps "aIsNestedCall" would be more accurate?
> 
> IIUC what it does, then why can't we do the "recurse into the ancestor
> for display:contents nodes" at the end of the *Internal methods in
> the caller instead?  Then we wouldn't need this param right?

Yup, I agree. Since the amount of changes is somewhat nontrivial, some of the bugs you spotted here are fixed in the next patch, and some of the changes are easier when the functions aren't duplicated, I'll just go ahead and squash both.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195616/diff/3-4/

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195616/#review201554

In general this looks great, though I'd like Mats to have a a look too.

::: layout/base/nsCSSFrameConstructor.h:2131
(Diff revision 4)
> -                                       mozilla::StyleDisplay& aTargetContentDisplay,
> -                                       nsContainerFrame* aParentFrame,
> -                                       bool aPrevSibling);
>  
>    /**
> -   * Find the frame for the content immediately preceding the one aIter
> +   * Find the frame for the content immediately after the one aIter points to,

"after" should be something depending on the SiblingDirection, right?

::: layout/base/nsCSSFrameConstructor.h:2161
(Diff revision 4)
> -   * points to, following continuations if necessary.  aIter is passed by value
> -   * on purpose, so as not to modify the caller's iterator.
> -   *
> -   * @param aIter should be positioned such that aIter.GetNextChild()
> -   *          is the first content to search for frames
> -   * @param aTargetContent the content we're finding a sibling frame for
> +                            mozilla::StyleDisplay& aTargetContentDisplay);
> +  // An alias of FindSibling<SiblingDirection::Backwards>.
> +  nsIFrame* FindPreviousSibling(const mozilla::dom::FlattenedChildIterator& aIter,
> +                                mozilla::StyleDisplay& aTargetContentDisplay);
> +
> +  nsIFrame* AdjustSiblingFrame(nsIFrame* aSibling,

This needs to be documented.

::: layout/base/nsCSSFrameConstructor.cpp:6893
(Diff revision 4)
> +  auto nextDomSibling = [](FlattenedChildIterator& aIter) -> nsIContent* {
> +    return aDirection == SiblingDirection::Forward
> +      ? aIter.GetNextChild() : aIter.GetPreviousChild();
> +  };
> +
> +  auto getClosestPseudo = [](const nsIContent* aContent) -> nsIFrame* {

"closer", not "closest", right?

::: layout/base/nsCSSFrameConstructor.cpp:7000
(Diff revision 4)
> -    nsIFrame* nextSibling =
> -      FindFrameForContentSibling(sibling, aTargetContent, aTargetContentDisplay,
> -                                 aParentFrame, false);
> +    FindSiblingInternal<aDirection>(aIter, targetContent, aTargetContentDisplay);
> +  if (sibling) {
> +    return sibling;
> +  }
>  
> -    if (nextSibling) {
> +  // Our siblings (if any) does not have a frame to guide us.

"do not have any frames to guide us"

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

https://reviewboard.mozilla.org/r/195616/#review201550

r=mats with the comments addressed

::: layout/base/nsCSSFrameConstructor.h:2126
(Diff revision 4)
> -   * will look for last continuations, etc, as necessary.  This calls
> -   * IsValidSibling as needed; if that returns false it returns null.
> +    Forward,
> +    Backwards,

nit: drop the 's' in Backwards

::: layout/base/nsCSSFrameConstructor.cpp:6899
(Diff revision 4)
> +    return aDirection == SiblingDirection::Forward
> +      ? nsLayoutUtils::GetBeforeFrame(aContent)
> +      : nsLayoutUtils::GetAfterFrame(aContent);
> +  };
> +
> +  auto getFurtherPseudo = [](const nsIContent* aContent) -> nsIFrame* {

nit: the "closest/further" naming doesn't feel right (it should be "furthest" right?), also isn't "near" the word to use as opposite of "far".
Maybe getNearPseudo/getFarPseudo? or getSameSidePseudo/getOppositeSidePseudo?

::: layout/base/nsCSSFrameConstructor.cpp:6909
(Diff revision 4)
> +      if (frame->GetContent() == sibling) {
> +        if (nsIFrame* sibling = adjust(frame)) {
> +          return sibling;
> +        }
> +      }
> +    }
> +
> +    if (GetDisplayContentsStyleFor(sibling)) {
> +      if (nsIFrame* frame = adjust(getClosestPseudo(sibling))) {

It's easy to misread this code since the inner 'sibling' declaration
shadows the outer 'sibling'.  Please rename it.  I suggest you rename
"frame = sibling->GetPrimaryFrame()" too (as 'primaryFrame') and then
use 'frame' consistently for the result from adjust().  I.e.
  if (nsIFrame* frame = adjust(primaryFrame)) {
    return frame;

::: layout/base/nsCSSFrameConstructor.cpp:6931
(Diff revision 4)
> -      return nullptr;
> +  if (nsIFrame* after = adjust(getFurtherPseudo(aIter.Parent()))) {
> +    return after;

nit: 'after' may actually be the ::before frame here...
I suggest you name it 'frame' instead for consistency with the other adjust() calls.

::: layout/base/nsCSSFrameConstructor.cpp:7000
(Diff revision 4)
> -    if (nextSibling) {
> -      // We found a next sibling, we're done!
> -      return nextSibling;
> +  // Our siblings (if any) does not have a frame to guide us.
> +  //
> +  // The frame for the target content should be inserted whereever a frame for
> +  // the container would be inserted.
> +  //
> +  // This is needed when inserting into display: contents nodes.

nit: no need to add the comment lines with no text here IMO, it can be one paragraph

::: layout/base/nsCSSFrameConstructor.cpp:7013
(Diff revision 4)
> +    const nsIContent* parent = current->GetFlattenedTreeParent();
> +    MOZ_ASSERT(parent, "No display: contents on the root");
> +
> +    FlattenedChildIterator iter(parent);
> +    iter.Seek(current);
> +    nsIFrame* sibling = FindSiblingInternal<aDirection>(

This declaration shadows the one outside the loop.
I think we should use an assignment here instead.

::: layout/base/nsCSSFrameConstructor.cpp:7084
(Diff revision 4)
>    nsIFrame* prevSibling =
> -    FindPreviousSibling(iter, iter.Get(), childDisplay, aInsertion->mParentFrame);
> +    FindPreviousSibling(iter, childDisplay);

fits on one line now?

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8924365 [details]
Bug 1413619: Fix insertion point computation when display: contents pseudos are involved.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195616/diff/4-5/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be1d69e97c5a
Fix insertion point computation when display: contents pseudos are involved. r=mats,bz

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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