Closed Bug 1413619 Opened 2 years ago Closed 2 years ago

FindFrameForContentSibling gets display: contents pseudos wrong

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files, 1 obsolete file)

Mats noticed this while reviewing bug 979782 (see bug 979782 comment 19). I'm writing a test-case now...
Attached file testcase
Blocks: 1251799, 1359656
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
Attachment #8924240 - Flags: review?(mats) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee243422ed36
Fix insertion point computation when display: contents pseudos are involved. r=mats
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b130c004f81
Backed out changeset ee243422ed36 for test failures.
Gah, seems like a couple tests are failing, I'll look into those...
Flags: needinfo?(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.
Flags: needinfo?(emilio)
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]
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).
Hm, I am lying and that last bit is wrong, so time to debug caption insertions I guess... :(
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.
Attachment #8924240 - Flags: review?(mats)
Attachment #8924240 - Flags: review+
Attachment #8924240 - Flags: feedback?(bzbarsky)
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.
Attachment #8924240 - Flags: review?(mats) → review-
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 :-))
Attachment #8924365 - Flags: review?(mats)
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.
Attachment #8924240 - Attachment is obsolete: true
Attachment #8924240 - Flags: feedback?(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"
Attachment #8924365 - Flags: review+
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?
Attachment #8924365 - Flags: review?(mats) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/be1d69e97c5a
Fix insertion point computation when display: contents pseudos are involved. r=mats,bz
https://hg.mozilla.org/mozilla-central/rev/be1d69e97c5a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.