Closed Bug 1377648 Opened 7 years ago Closed 7 years ago

HTMLSummaryElement::IsSummary() check fails on removing the element

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files)

Bug 1377648 - Check summary frame instead of summary element on removing the summary.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1377648 - Use inFlowFrame to check the target frame is summary and its parent is details.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1377648 - Test case for reconstruction details element when removing summary element.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Currently we basically return reconstruction damage if we get to fail old style context.  

I tried two ways to make it more accurate in bug 1374175.

1) Calc style differences with servo's computed values
2) Return no damage if there is no display property changes

With either ways, layout/reftests/details-summary/mouse-click-move-summary-to-different-details.html failed.  So I did give up making it accurate in that bug.
Summary: Make compute_style_difference more accurate → stylo: Make compute_style_difference more accurate
OK. I think I am getting closer to the root cause of the failure for mouse-click-move-summary-to-different-details.html.

In the test case, we try to move a summary element into another details element using insertBefore().  At the moment, we call MaybeRecreateContainerForFrameRemoval for the summary frame and it seems to be expected that RecreateFramesForContent() is called for the summary frame [1]. But unfortunately |insertionFrame| there is not the parent details element's frame, it's the same summary frame itself.
So I guess making GetContentInsertionFrame() return the parent details frame for the summary frame fixes the failure.
I don't yet understand why gecko does not have the same problem, I guess gecko does produce reconstructFrame change hint somewhere else (or being conservative just like stylo currently is?).

[1] https://hg.mozilla.org/mozilla-central/file/6f8f10f48ace/layout/base/nsCSSFrameConstructor.cpp#l9724
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> OK. I think I am getting closer to the root cause of the failure for
> mouse-click-move-summary-to-different-details.html.
> 
> In the test case, we try to move a summary element into another details
> element using insertBefore().  At the moment, we call
> MaybeRecreateContainerForFrameRemoval for the summary frame and it seems to
> be expected that RecreateFramesForContent() is called for the summary frame
> [1]. But unfortunately |insertionFrame| there is not the parent details
> element's frame, it's the same summary frame itself.
> So I guess making GetContentInsertionFrame() return the parent details frame
> for the summary frame fixes the failure.

Doh! Idiot!  I was wrong. IsSummary() returns false somehow.. I will investigate the reason in detail.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > OK. I think I am getting closer to the root cause of the failure for
> > mouse-click-move-summary-to-different-details.html.
> > 
> > In the test case, we try to move a summary element into another details
> > element using insertBefore().  At the moment, we call
> > MaybeRecreateContainerForFrameRemoval for the summary frame and it seems to
> > be expected that RecreateFramesForContent() is called for the summary frame
> > [1]. But unfortunately |insertionFrame| there is not the parent details
> > element's frame, it's the same summary frame itself.
> > So I guess making GetContentInsertionFrame() return the parent details frame
> > for the summary frame fixes the failure.
> 
> Doh! Idiot!  I was wrong. IsSummary() returns false somehow.. I will
> investigate the reason in detail.

IsMainSummary(), I mean.
Changing title to reuse this bug for fixing the IsMainSummary() issue.
Blocks: 1374175
Component: CSS Parsing and Computation → Layout: Misc Code
Summary: stylo: Make compute_style_difference more accurate → HTMLSummaryElement::IsSummary() check fails on removing the element
Unfortunately I can't provide any test cases that fail on gecko.  For stylo mouse-click-move-summary-to-different-details.html is a test case when we apply the patch for bug 1374175.
As per today's stylo meeting, I sent review request to Cameron. Thank you Cameron.
Comment on attachment 8882962 [details]
Bug 1377648 - Check summary frame instead of summary element on removing the summary.

https://reviewboard.mozilla.org/r/153934/#review159218

::: layout/generic/DetailsFrame.cpp:130
(Diff revision 2)
>      aElements.AppendElement(mDefaultSummary);
>    }
>  }
>  
> +bool
> +DetailsFrame::IsMainSummaryFrame(nsIFrame* aSummaryFrame)

Nit: I think "HasMainSummaryFrame" might be a better name, since otherwise it looks like the frame that you are calling the method on (the DetailsFrame) is being checked for whether it is a summary frame.

::: layout/generic/DetailsFrame.cpp:132
(Diff revision 2)
>  }
>  
> +bool
> +DetailsFrame::IsMainSummaryFrame(nsIFrame* aSummaryFrame)
> +{
> +  return aSummaryFrame == mFrames.FirstChild();

I think this won't work if we make the <summary> element out of flow, with e.g. float:left or position:absolute.  Perhaps we can check if mFrames.FirstChild() is a placeholder frame, and if it is, follow it to find the (potential) summary frame.

Can you add a test case for this too?
Thanks for the quick review.

(In reply to Cameron McCormack (:heycam) from comment #10)

> ::: layout/generic/DetailsFrame.cpp:132
> (Diff revision 2)
> >  }
> >  
> > +bool
> > +DetailsFrame::IsMainSummaryFrame(nsIFrame* aSummaryFrame)
> > +{
> > +  return aSummaryFrame == mFrames.FirstChild();
> 
> I think this won't work if we make the <summary> element out of flow, with
> e.g. float:left or position:absolute.  Perhaps we can check if
> mFrames.FirstChild() is a placeholder frame, and if it is, follow it to find
> the (potential) summary frame.
> 
> Can you add a test case for this too?

Sure, I will do it.
It turns out HasMainSummeryFrame() is not called in the case where the summary element is out of flow, and it seems to me that the case is processed somewhere else.  Anyway, I will add a test case that the summary element is out of flow and make HasMainSummaryFrame safe to be called for such out of flow summary elements, but the new test case will succeed with/without the patch.
More precisely, 'position:absolute' case seems to be processed somewhere, 'float:left' case calls HasMainSummaryFrame() but in the case we seems to pass place holder frame for the summary frame to the function.
OK, in the case of position:absolute, we need special handling in MaybeRecreateContainerForFrameRemoval() or ContentRemoved().
Thank you Cameron for making me realize the fact.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> OK, in the case of position:absolute, we need special handling in
> MaybeRecreateContainerForFrameRemoval() or ContentRemoved().
> Thank you Cameron for making me realize the fact.

It seems to me that fieldset has the same problem here [1] in case of position:absolute. But in case of fieldset the problem I have been seeing in bug 1374175 does not happen on stylo.  The reason I am guessing is that we pass aFlags (actually it was REMOVE_CONTENT) to RecreateFramesForContent instead of REMOVE_FOR_RECONSTRUCTION so that it does not rely on the reconstruction damage from compute_style_difference in the first place.

Anyway, I am going to include the fix for fieldset here in this bug too.

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

[1] https://hg.mozilla.org/mozilla-central/file/acbd9a64c0fa/layout/base/nsCSSFrameConstructor.cpp#l9714
Hmm the fieldset part caused crash when removing legend element on the try.  In the crash case RecreateFramesForContent() returns nullptr for aDestroyedFramesFor. Though I haven't dug into detail, I guess with the fix, RecreateFramesForContent() starts being called on removing legend, but still needs to be modified.  So, I will leave the fieldset part in a later bug.

A new try without the fieldset parts;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=308a1a772d87a88e139e3862137f997ad0c03fd7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7343e0dd66a82bc7b3240ae7d16533880ef0150
I made a silly mistake in HasMainSummaryFrame(), I did compare |aSummaryFrame| with |aSummaryFrame| there!
Comment on attachment 8882962 [details]
Bug 1377648 - Check summary frame instead of summary element on removing the summary.

https://reviewboard.mozilla.org/r/153934/#review159422

r=me with the second GetRealFrameFor call removed, or an explanation of why we do need it.

::: layout/generic/DetailsFrame.h:59
(Diff revision 3)
>    void AppendAnonymousContentTo(nsTArray<nsIContent*>& aElements,
>                                  uint32_t aFilter) override;
> +  // Returns true if |aSummaryFrame| is the main summary (i.e. the first child
> +  // of this details frame).
> +  // This function is used when the summary element is removed from the parent
> +  // details element since at the moment the summary element has been already

Nit: s/at the moment/at that moment/

::: layout/generic/DetailsFrame.cpp:134
(Diff revision 3)
> +bool
> +DetailsFrame::HasMainSummaryFrame(nsIFrame* aSummaryFrame)
> +{
> +  nsIFrame* firstChild =
> +    nsPlaceholderFrame::GetRealFrameFor(mFrames.FirstChild());
> +  nsIFrame* summary = nsPlaceholderFrame::GetRealFrameFor(aSummaryFrame);

Do we need this second call to GetRealFrameFor?  I think if we are under nsCSSFrameConstructor::ContentRemoved, aSummaryFrame will be the primary frame for the <summary>, and so it will be the actual summary frame and not the placeholder.
Attachment #8882962 - Flags: review?(cam) → review+
Comment on attachment 8883441 [details]
Bug 1377648 - Use inFlowFrame to check the target frame is summary and its parent is details.

https://reviewboard.mozilla.org/r/154340/#review159446

OK, I think I satisified myself that this is doing the right thing when e.g. <summary> is overflow:scroll (and thus aFrame is the HTMLScrollFrame rather than the SummaryFrame).

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
>                                 aDestroyedFramesFor);
>        return true;
>      }
>    }
>  
> -  // Now check for possibly needing to reconstruct due to a pseudo parent

This comment should remain here.
Attachment #8883441 - Flags: review?(cam) → review+
Comment on attachment 8883442 [details]
Bug 1377648 - Test case for reconstruction details element when removing summary element.

https://reviewboard.mozilla.org/r/154342/#review159468
Attachment #8883442 - Flags: review?(cam) → review+
Comment on attachment 8882962 [details]
Bug 1377648 - Check summary frame instead of summary element on removing the summary.

https://reviewboard.mozilla.org/r/153934/#review159422

> Do we need this second call to GetRealFrameFor?  I think if we are under nsCSSFrameConstructor::ContentRemoved, aSummaryFrame will be the primary frame for the <summary>, and so it will be the actual summary frame and not the placeholder.

Oh, right. We don't need to call GetReadFrameFor for aSummary. Thanks!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b4c22da480e
Use inFlowFrame to check the target frame is summary and its parent is details. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e746c1abfea9
Check summary frame instead of summary element on removing the summary. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6be9781ed937
Test case for reconstruction details element when removing summary element. r=heycam
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: