Remove AdjustFrameForAfterContent.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

It's very complex and bogus in the Shadow DOM case. Let's replace with something we have already and handles it correctly, like FindNextSibling.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Sorry for the mess, tried to put one patch on top of another as I self-reviewed my patch, and ended up messing up.

The order of the patches doesn't really matter in practice.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8931158 [details]
Bug 1419964: Remove AdjustParentFrameForAfterContent.

https://reviewboard.mozilla.org/r/202244/#review207684

::: layout/base/nsCSSFrameConstructor.cpp:6481
(Diff revision 1)
>   * This function will get the previous sibling to use for an append operation.
>   * it takes a parent frame (must not be null) and its :after frame (may be
>   * null).
>   */
>  static nsIFrame*
> -FindAppendPrevSibling(nsIFrame* aParentFrame, nsIFrame* aAfterFrame)
> +FindAppendPrevSibling(nsIFrame* aParentFrame, nsIFrame* aNextSibling)

Now the function argument name doesn't match what it's documented to be right above the function.

I guess the point is that in the display:contents case it might not be an actual ::after frame?  In that case, we should fix the comments to describe what it represents.

::: layout/base/nsCSSFrameConstructor.cpp:7532
(Diff revision 1)
>      RecreateFramesForContent(parentFrame->GetContent(), aInsertionKind);
>      LAYOUT_PHASE_TEMP_REENTER();
>      return;
>    }
>  
> +  nsContainerFrame* containingBlock = GetFloatContainingBlock(parentFrame);

Why is it ok to put this before the {ib} handling?

::: layout/base/nsCSSFrameConstructor.cpp:7578
(Diff revision 1)
>               "Parent frame should not be fieldset or details!");
>  
> -  // Deal with possible :after generated content on the parent
> -  nsIFrame* parentAfterFrame;
> -  nsContainerFrame* preAdjustedParentFrame = parentFrame;
> -  parentFrame =
> +  // Deal with possible :after generated content on the parent, or display:
> +  // contents.
> +  nsIFrame* nextSibling = nullptr;
> +  if (GetDisplayContentsStyleFor(insertion.mContainer) ||

Why do we no longer need to worry about display:contents children on the container?  AdjustAppendParentForAfterContent used to worry about them...
Attachment #8931158 - Flags: review?(bzbarsky)
Assignee

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8931158 [details]
Bug 1419964: Remove AdjustParentFrameForAfterContent.

https://reviewboard.mozilla.org/r/202244/#review207684

> Now the function argument name doesn't match what it's documented to be right above the function.
> 
> I guess the point is that in the display:contents case it might not be an actual ::after frame?  In that case, we should fix the comments to describe what it represents.

Right. I'll fix the docs.

> Why is it ok to put this before the {ib} handling?

parentFrame in practice is already the last ib sibling, modulo the trailing inline, which comes from `insertion.mParentFrame`. But it doesn't really matter to put it after I think, so I'll move it.

> Why do we no longer need to worry about display:contents children on the container?  AdjustAppendParentForAfterContent used to worry about them...

I don't think we needed to worry about it in the first place. We're appending content to a DOM node. The only way there may be something after that DOM node's last child is if it has display: contents or if it has ::after content.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8931156 - Attachment is obsolete: true
Attachment #8931156 - Flags: review?(mats)
Comment hidden (mozreview-request)
Priority: -- → P3

Comment 10

2 years ago
mozreview-review
Comment on attachment 8931158 [details]
Bug 1419964: Remove AdjustParentFrameForAfterContent.

https://reviewboard.mozilla.org/r/202244/#review208556

LGTM, r=mats

::: layout/base/nsCSSFrameConstructor.cpp:6477
(Diff revision 3)
> -  return aParentFrame;
> -}
> -
> -/**
>   * This function will get the previous sibling to use for an append operation.
> - * it takes a parent frame (must not be null) and its :after frame (may be
> + * it takes a parent frame (must not be null) and the next insertion sibling, if

nit: s/it/It/ while we're here...
Attachment #8931158 - Flags: review?(mats) → review+

Comment 11

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada567096efb
Remove AdjustParentFrameForAfterContent. r=mats
Assignee

Updated

2 years ago
Blocks: 1424094

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ada567096efb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 13

2 years ago
mozreview-review
Comment on attachment 8931158 [details]
Bug 1419964: Remove AdjustParentFrameForAfterContent.

https://reviewboard.mozilla.org/r/202244/#review212766
Attachment #8931158 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.