Closed Bug 1245424 Opened 9 years ago Closed 9 years ago

Fix crash on display:none summary and add test cases described in description

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

MozReview Request: Bug 1245424 Part 1 - Fix assert for a display:none summary on debug build.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
MozReview Request: Bug 1245424 Part 2 - Stop checking the existence of details frame before toggling.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
File this bug per bug 591737 comment 222. bz: Also of interest are cases in which a capturing event listener higher up the DOM sets the <details> or <summary> to display:none. Or moves the <summary> to a different <details> element, for that matter. Please check what Chrome/Safari do in all three of those situations? Currently, display:none summary will crash. Block bug 1226455.
Summary: Add test cases for display:none details and summary → Fix crash on display:none summary and add test cases described in description
No longer blocks: ship-details-summary
Assignee: nobody → tlin
Status: NEW → ASSIGNED
If the summary element has 'display: none' style, no summary frame will be generated. So we only need to check the summary frame is generated when the summary element doesn't have 'display: none'. Review commit: https://reviewboard.mozilla.org/r/41415/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41415/
Attachment #8732887 - Flags: review?(bzbarsky)
These tests modify details or summary elements in the 'click' event listener in capturing phase higher up in the DOM tree. Review commit: https://reviewboard.mozilla.org/r/41435/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41435/
Comment on attachment 8732887 [details] MozReview Request: Bug 1245424 Part 1 - Fix assert for a display:none summary on debug build. https://reviewboard.mozilla.org/r/41415/#review37881 ::: layout/generic/DetailsFrame.cpp:76 (Diff revision 1) > - nsIFrame* realFrame = > - nsPlaceholderFrame::GetRealFrameFor(isOpen ? > - aChildList.FirstChild() : > - aChildList.OnlyChild()); > + HTMLSummaryElement* summary = > + HTMLSummaryElement::FromContentOrNull(details->GetFirstSummary()); > + if (summary) { > + nsIPresShell* presShell = > + nsComputedDOMStyle::GetPresShellForContent(summary); > + if (!nsGenericHTMLElement::IsOrHasAncestorWithDisplayNone(summary, I don't think it's worth going to all these lengths. If display:none summary means that we have no kids, then the assert that we have kids is bogus and we should just take it out instead of trying to hedge it with complicated detection of the case when we might not have kids.
Attachment #8732887 - Flags: review?(bzbarsky)
https://reviewboard.mozilla.org/r/41435/#review37885 Before asking review for part 2, I want to have a feedback about whether we need to persue the compactness with Chrome, i.e. we check whether details is display: none before toggle the 'open' attribute when summary receives a click event. https://hg.mozilla.org/mozilla-central/annotate/a2fe8535e63b/dom/html/HTMLSummaryElement.cpp#l63 These tests might rarely occur in real web pages, but they reveal a fact that we might never compact with Chrome in every corner cases. Per discussion in [html spec issue](https://github.com/whatwg/html/issues/517#issuecomment-198460384), Olli also feels this behavior is weird. How risky if we do not compact in this case? ::: layout/reftests/details-summary/mouse-click-change-details-to-display-none.html:8 (Diff revision 1) > + - http://creativecommons.org/publicdomain/zero/1.0/ --> > + > +<html class="reftest-wait"> > + <script> > + function runTest() { > + // Both Chrome and Safari add the 'open' attribute to the details element, In this test case, I think our implmentation make sense because we correctly flush the frame in https://hg.mozilla.org/mozilla-central/annotate/a2fe8535e63b/dom/html/HTMLSummaryElement.cpp#l67 , so we don't toggle the details. ::: layout/reftests/details-summary/mouse-click-move-summary-to-different-details.html:8 (Diff revision 1) > + - http://creativecommons.org/publicdomain/zero/1.0/ --> > + > +<html class="reftest-wait"> > + <script> > + function runTest() { > + // Both Chrome and Safari do not add the 'open' attribute to details1 This case is more interesting. Again, I think our implementations make sense.
Need some advice for comment 4.
Flags: needinfo?(bzbarsky)
https://reviewboard.mozilla.org/r/41415/#review37881 > I don't think it's worth going to all these lengths. If display:none summary means that we have no kids, then the assert that we have kids is bogus and we should just take it out instead of trying to hedge it with complicated detection of the case when we might not have kids. display:none summary does not mean a details frame have no child. It just means it does not have a summary frame as its first child. If we can find a non-bogus way for this, I would still like to assert a fact that a details frame's first child should be a summary frame (either \<summay\> or a generated one) assuming the summary is not display:none. This catch many bugs when I did the work in nsCSSFrameConstructor.
> How risky if we do not compact in this case? As in, if we toggle for a synthetic click on a display:none summary? I think that should be fine, personally... I expect it not working in Chrome is a side-effect of part of their impl living in the layout object or something. But it's worth checking with the Chrome folks explicitly to see why their behavior is as it is and whether they are open to changing it. Same for the two tests you mention.
> display:none summary does not mean a details frame have no child. It just > means it does not have a summary frame as its first child. Yes, I understand that. But my point is that once we have this exception, things get complicated. In particular, are we 100% sure that display:none is the only way for the <summary> to not have a frame? I'm about 90% sure it's not (e.g. I expect you can get there with XBL bindings, and there might be other stuff I'm just not thinking of right now)... You _could_ assert that there is no summary frame anywhere in the child list if it's not in the right position. That seems like a valid assert to me, and much more robust. It has the drawback of being slow, but that would only matter if there is no summary frame in the right position, right? One other thing, now that I'm thinking about this. Should all this "have a summary child" stuff be operating on the light DOM or the flattened tree, when shadow DOM is involved? And if the former, then what should happen for display:none on a flattened tree, but not DOM, ancestor of the <summary>?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #8) > > display:none summary does not mean a details frame have no child. It just > > means it does not have a summary frame as its first child. > > Yes, I understand that. > > But my point is that once we have this exception, things get complicated. > In particular, are we 100% sure that display:none is the only way for the > <summary> to not have a frame? I'm about 90% sure it's not (e.g. I expect > you can get there with XBL bindings, and there might be other stuff I'm just > not thinking of right now)... Yes. There might be other cases that I don't know of which could make summary to not have a frame. > You _could_ assert that there is no summary frame anywhere in the child list > if it's not in the right position. That seems like a valid assert to me, > and much more robust. It has the drawback of being slow, but that would > only matter if there is no summary frame in the right position, right? Yes. A "display:none" summary isn't a normal case. Your approach should work, and I'll use it in my next patch set. > One other thing, now that I'm thinking about this. Should all this "have a > summary child" stuff be operating on the light DOM or the flattened tree, > when shadow DOM is involved? And if the former, then what should happen for > display:none on a flattened tree, but not DOM, ancestor of the <summary>? Per html spec [1] , summary can be used only as the first child of a details element. We follow Chrome's behavior to extend it a bit to allow summary to have meaning if it's the direct child of an details element. So I think details/summary relationship only works in light DOM. Assume a details element is in document tree as the shadow host the and the summary is in a shadow tree, we should add a default summary to the details, and any click event dispatches to the summary in shadow tree should do nothing. (I just learn shadow DOM today, so I might miss something :) ) [1] https://html.spec.whatwg.org/multipage/forms.html#the-summary-element
> So I think details/summary relationship only works in light DOM. Is that what happens in Chrome? Because if we want it to work this way, then the summary frame thing is totally wrong: a <summary> that's a child in the light DOM may well be a grandchild in the flattened tree. > Assume a details element is in document tree as the shadow host the and the > summary is in a shadow tree, we should add a default summary to the details OK, that sounds like you want to use the parent/child relationships in the flattened tree, not the light DOM....
(In reply to Boris Zbarsky [:bz] from comment #11) > OK, that sounds like you want to use the parent/child relationships in the > flattened tree, not the light DOM.... Yes, that's what I meant. Sorry for being confused by the terminology. File bug 1258948 for investigating details/summary involving with Shadow DOM. It seems that Chrome has a different thought about details being a shadow root.
Attachment #8732887 - Flags: review?(bzbarsky)
Comment on attachment 8732887 [details] MozReview Request: Bug 1245424 Part 1 - Fix assert for a display:none summary on debug build. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41415/diff/1-2/
Attachment #8732888 - Attachment description: MozReview Request: Bug 1245424 Part 2 - Add reftest for click events with actions in capturing phase. → MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase.
Attachment #8732888 - Flags: review?(bzbarsky)
Comment on attachment 8732888 [details] MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41435/diff/1-2/
Re comment 16: Olli, thank you for the information!
Attachment #8732888 - Flags: review?(bzbarsky) → review+
Comment on attachment 8732888 [details] MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase. https://reviewboard.mozilla.org/r/41435/#review38551 ::: layout/reftests/details-summary/mouse-click-change-details-to-display-none.html:20 (Diff revision 2) > + }, true); > + > + summary.dispatchEvent(new MouseEvent("click")); > + > + details.style.display = "block"; > + document.documentElement.removeAttribute("class"); document.documentElement.className = ""; is probably simpler. ::: layout/reftests/details-summary/mouse-click-move-summary-to-different-details.html:15 (Diff revision 2) > + // moved to details1 before receiving the 'click' event. > + var details1 = document.getElementById("details1"); > + var summary2 = document.getElementById("summary2"); > + > + document.body.addEventListener("click", function () { > + // Move summary2 into details1 to be the main summary in capturing phase. You mean in the default action phase, right?
Comment on attachment 8732887 [details] MozReview Request: Bug 1245424 Part 1 - Fix assert for a display:none summary on debug build. https://reviewboard.mozilla.org/r/41415/#review38555 ::: layout/generic/DetailsFrame.cpp:77 (Diff revision 2) > - aChildList.FirstChild() : > - aChildList.OnlyChild()); > - MOZ_ASSERT(realFrame, "Principal list of details should not be empty!"); > - nsIFrame* summaryFrame = realFrame->GetContentInsertionFrame(); > - MOZ_ASSERT(summaryFrame->GetType() == nsGkAtoms::summaryFrame, > - "The frame should be summary frame!"); > + HTMLSummaryElement* summary = > + HTMLSummaryElement::FromContent(firstChild->GetContent()); > + > + if (!summary || !summary->IsMainSummary()) { > + for (nsIFrame* child = firstChild->GetNextSibling(); > + child; Please use an nsFrameList::Enumerator or range-based iteration over aChildList instead of a manual GetNextSibling walk.
Attachment #8732887 - Flags: review?(bzbarsky) → review+
Comment on attachment 8733748 [details] MozReview Request: Bug 1245424 Part 2 - Stop checking the existence of details frame before toggling. https://reviewboard.mozilla.org/r/41937/#review38557
Attachment #8733748 - Flags: review?(bzbarsky) → review+
Comment on attachment 8732887 [details] MozReview Request: Bug 1245424 Part 1 - Fix assert for a display:none summary on debug build. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41415/diff/2-3/
Comment on attachment 8733748 [details] MozReview Request: Bug 1245424 Part 2 - Stop checking the existence of details frame before toggling. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41937/diff/1-2/
Comment on attachment 8732888 [details] MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41435/diff/2-3/
https://reviewboard.mozilla.org/r/41415/#review38555 > Please use an nsFrameList::Enumerator or range-based iteration over aChildList instead of a manual GetNextSibling walk. Thanks for the suggestion. I use range-based for-loop to rewrite it, and it becomes simpler!
https://reviewboard.mozilla.org/r/41435/#review38551 > document.documentElement.className = ""; > > is probably simpler. Indeed, but other details & summary tests already used removeAttribute. I would like to keep it as it is, so it would be easier to search and replace when we migrate these tests to web-platform-test. > You mean in the default action phase, right? The click event handler will move the summary2 to details1 while the click event is in capturing phase, and the details1 will be toggled is in default action phase.
> I would like to keep it as it is OK. > The click event handler will move the summary2 to details1 while the click event is > in capturing phase, and the details1 will be toggled is in default action phase. Right. I guess the thing that confused me was whether "in capturing phase" referred to "move" or to "be the main summary". The idea is that the move happens in the capturing phase, so that by the default action phase we're the main summary.
(In reply to Boris Zbarsky [:bz] from comment #27) > Right. I guess the thing that confused me was whether "in capturing phase" > referred to "move" or to "be the main summary". The idea is that the move > happens in the capturing phase, so that by the default action phase we're > the main summary. Ah. Now I see why you get confused. Sorry for my English. I can reword the sentence as "Move summary2 into details1 at capture phase, and summary2 will be the main summary of details1 at target phase."
That sounds perfect, thanks!
Comment on attachment 8732888 [details] MozReview Request: Bug 1245424 Part 3 - Add reftest for click events with actions in capturing phase. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41435/diff/3-4/
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: